Migrate UI upgrade tests should provide the complete log

Saving a CSV file seems like a work-around, not a real solution.

Can we do drupalGet() in tearDown() and save the browser output of /admin/reports/dblog along with the rest of the UI pages?

Hi,

Creating a patch as suggested in #8 comment.

Please review the patch.

Thanks.

Have not seen the issue in detail but drupalGet is undefined in migrate_drupal_ui/tests/src/Functional/ - Reason for failing of test,
Without its fix patch will not work

1. I like this approach a lot better!

    +    $this->drupalGet('/admin/reports/dblog');
    +    while (strstr($this->getSession()->getPage()->getContent(), 'Next page') !== FALSE) {
    +      $this->clickLink('Next page');
    +    }

   Can we be a little more specific? We might follow the wrong link if we just search for the text "Next page". (Does clickLink('Next page') look for a link whose text contains "Next page" or does it only match if that is the complete link text?)

   Slight refinement: do not search for "Next page" twice.

    $page = $this->getSession()->getPage();
    while ($next_link = $page->findLink(...)) {
      $next_link->click();
    }

   Or does this work?

    $next_link = $this->getSession()->getPage()->findLink(...);
    while ($next_link) {
      $next_link->click();
    }

2. What we are doing here is a little unusual, so maybe we should add a code comment.

3. MigrateUpgradeTestBase extends BrowserTestBase, which already uses UiHelperTrait. Why do we need this?

    +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeTestBase.php
    @@ -5,6 +5,7 @@
    ...
    +use Drupal\Tests\UiHelperTrait;
    @@ -16,6 +17,7 @@ abstract class MigrateUpgradeTestBase extends BrowserTestBase {
    ...
    +  use UiHelperTrait;

   I know that #12 claims that drupalGet() is not available. I see: the patch in #9 tried drupalGet() as a global function. I meant $this->drupalGet() as in #15.

The test failed with

1) Drupal\Tests\migrate_drupal_ui\Functional\d6\Upgrade6Test::testMigrateUpgradeExecute
Failed asserting that '33' matches expected 37.
...
/var/www/html/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeExecuteTestBase.php:126

If you update outputLogs() from

    $this->drupalGet('/admin/reports/dblog');

to

    $this->drupalGet('/admin/reports/dblog', ['query' => ['severity[]' => '3']]);

and run the test locally, then you can see the 37 expected errors on one page (no pagination needed). Also one error from the node module.

Staring at the list, can we guess which of the errors are not logged when the testbot runs the test? I see exactly 4 copies of

Invalid translation language (und) specified. (/app/core/lib/Drupal/Core/Entity/ContentEntityBase.php:955)

(Get the full text of the message from the title attribute.)

I've whacked in couple of assertions on the expected watchdog errors (interdiff attached), these pass my local test, but the way it looks the 4 errors about A translation already exists for the specified language are not in the CI -environment.

See test-result here

@Spokje: Thanks! Certainly it is better to use RfcLogLevel::ERROR instead of '3' as in #22.

Looking at the results of my local test, these are /admin/reports/deblog/972 through ...975. The full text (from the title attributes):

• Source ID 1: A translation already exists for the specified language (zu). (/app/core/lib/Drupal/Core/Entity/ContentEntityBase.php:810)
• A translation already exists for the specified language (zu). (/app/core/lib/Drupal/Core/Entity/ContentEntityBase.php:810)
• Source ID 2: A translation already exists for the specified language (fr). (/app/core/lib/Drupal/Core/Entity/ContentEntityBase.php:810)
• A translation already exists for the specified language (fr). (/app/core/lib/Drupal/Core/Entity/ContentEntityBase.php:810)

By hacking Drupal\Tests\migrate_drupal_ui\Functional\MigrateUpgradeTestBase to extend WebDriverTestBase I was able to get a screenshot from the Watchdog log on Drupal CI.
(Very Evil Patch that made the screenshot here)

Indeed the 4 A translation already exists for the specified language errors mentioned by @benjifisher in #24 are not present on the Drupal CI environment.

On my local install they _are_ present (and it seems on @benjifisher's install as well).

Hope Bigger Brains than mine are able to figure out why those errors are not present on CI, but present when running local.

1. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeExecuteTestBase.php
   @@ -22,6 +23,14 @@ protected function setUp() {
   +  /**
   +   * Gets the expected number of error messages.
   +   *
   +   * @return int
   +   *   The number of expected logged errors of type migrate_drupal_ui.
   +   */
   +  abstract protected function getLogCountsError();
   +
      /**
   

   Need the return typehint that is in the docblock to be added to the method declaration

2. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d6/IdConflictTest.php
   @@ -67,6 +67,12 @@ protected function getAvailablePaths() {
   +  /**
   +   * {@inheritdoc}
   +   */
   +  protected function getLogCountsError() {
   +  }
   +
      /**
   

   Which brings up the issue that you've declared the return value to be an int, but you are returning void here. Should these `return 0;`?

I patched and ran Drupal\Tests\migrate_drupal_ui\Functional\d7\Upgrade7Test locally, and I get all the logs pages as html.

While you can't click into the links (since we don't scrape those pages), you can read the un-truncated message by hovering over the link since the link title holds the un-truncated message.

I'm confused about something, though: if the test fails, I don't see browser output for all the log pages.

I tested this by setting $context['results']['failures'] = 1; in MigrateUpgradeImportBatch::run(). The result is that I get a test failure like this:

Drupal\Tests\migrate_drupal_ui\Functional\d7\Upgrade7Test::testUpgradeAndIncremental
Behat\Mink\Exception\ResponseTextException: The text "Congratulations, you upgraded Drupal!" was not found anywhere in the text of the current page.

And that's where my browser output stops. I can't get to the log pages because they apparently aren't scraped. So the logs aren't there when I actually need them!

Is this something wrong or different with my local test setup? Or is there something I'm misunderstanding? Or is this a real problem with the approach in the issue?

Setting to NW to get response. Thanks!

Thanks, @quietone. That makes sense and I like the fixes.

Now I get the logs even if the test fails.

I only have one suggestion for the patch, and I'm open to a rebuttal on it. Regarding the new abstract function:

+++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeExecuteTestBase.php
@@ -22,6 +37,25 @@ protected function setUp() {
+  abstract protected function getLogCountsError(): int;

What would you think about changing that to a protected property just like the new ones you added in the last patch. It could be protected $logCountsError = 0; or even leave it null protected $logCountsError;.

And then that default value could be overridden as needed. That way we wouldn't have to duplicate this all over:

  /**
   * {@inheritdoc}
   */
  protected function getLogCountsError(): int {
    return 0;
  }

Setting to NW to get a response. Once again, open to rebuttal!

I somehow lost track of this one! The latest patch looks really nice. I'm going trigger tests and then RTBC when they pass.

Darn, apparently this needs a reroll. Tagging and setting to NW.

I also noticed that we all missed that the logs were not tested in Upgrade6Test so I added that as well.

Oops. Well this time I applied the patch and tested with Upgrade6Test, but otherwise just like I did in #45. The logs were available when the test passed as well as when the test failed. I'm happy.

Back to RTBC as per comment #57.

For the record, #57 was not the latest failure. The failure in #58 is in Drupal\Tests\views\Functional\Plugin\DisplayTest which is a different random failure.

Most recent random failure was in Drupal\Tests\layout_builder\FunctionalJavascript\AjaxBlockTest

Back to RTBC.

Reroll looks correct. RTBC. Thanks, @quietone!

This makes sense to me. RTBC.

This got me wondering if all the tests we've been moving to contrib are going to start failing, but it looks like they'll be fine. Those tests don't assert the number of log errors so they shouldn't mind a few extra errors.

The patch for this issue needs a reroll after #3219959: Update standard profile so Olivero is the default theme. I am adding the Novice tag for the reroll.

Thanks for the reroll!

I compared the patches in #75 and #78. Other than metadata, the only differences were an entity count and an expected error count in Upgrade7Test. If the testbot is happy, then I am happy. Back to RTBC.

I was a bit confused why we're visiting the log pages reading the patch, but it seems like a decent way to preserve the messages and couldn't think of another idea.

Committed/pushed to 10.1.x and cherry-picked back to 9.4.x, thanks!

This issue may have broken D9 head; a revert patch is running at #2994000: Notice in logs when setting invalid translation config for a content type.

Must have been this one. We changed the block count several times as the new themes became default, all of which should be in 9.4 and 10, but we should have ran the test again against D9, there are enough differences. I would be fine with leaving this in 10.1+ and reverting 9.4,9.5, and 10.0 if that's an option.

Reverted from 9.4.x and 9.5.x for now.

This is failing on sqlite on 10.0.x and 10.1.x too, reverted from those branches as well.

Want to request an uncheck all button. Since the fix has moved to MR5272 hiding the patches for clarity.

Rebased MR5272 to run test-only feature

1) Drupal\Tests\migrate_drupal_ui\Functional\d6\Upgrade6Test::testUpgradeAndIncremental
Error: Call to undefined method Drupal\Tests\migrate_drupal_ui\Functional\d6\Upgrade6Test::outputLogs()
/builds/issue/drupal-3082211/core/modules/migrate_drupal_ui/tests/src/Functional/d6/Upgrade6Test.php:211
/builds/issue/drupal-3082211/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
ERRORS!
Tests: 1, Assertions: 4364, Errors: 1.
Unsilenced deprecation notices (3)
  1x: Creation of dynamic property Drupal\Tests\migrate_drupal_ui\Functional\d6\Upgrade6Test::$outputLogs is deprecated
    1x in Upgrade6Test::testUpgradeAndIncremental from Drupal\Tests\migrate_drupal_ui\Functional\d6
  1x: Creation of dynamic property Drupal\Tests\migrate_drupal_ui\Functional\d6\Upgrade6Test::$migratedAdminUserName is deprecated
    1x in Upgrade6Test::testUpgradeAndIncremental from Drupal\Tests\migrate_drupal_ui\Functional\d6
  1x: Creation of dynamic property Drupal\Tests\migrate_drupal_ui\Functional\d6\Upgrade6Test::$expectedLoggedErrors is deprecated
    1x in Upgrade6Test::testUpgradeAndIncremental from Drupal\Tests\migrate_drupal_ui\Functional\d6
Uploading artifacts for failed job

The change itself seems simple enough.

Added some questions to the MR.

Left some small nitpicky stuff, leaving in review for additional eyes.

Seems nitpicky stuff was addressed. Since this was close before believe it's fine to mark now.

Needs rebasing against 11.x, there are lots of unrelated changes in the MR.

Confirmed via GitLab CI that the tests pass on SQLite now. However, decided not to backport as this is a minimal improvement and seems slightly fragile given the hardcoded log counts.

Committed a904645 and pushed to 11.x. Thanks!