Assert no errors after creating the test project in ModuleUpdateTest

Unpostponing but also not tagging with sprint because it is contrib-only

Just like I extracted 📌 Make build tests fail 1) more explicitly, 2) earlier when possible (failing StatusCheckEvent subscribers) Fixed from 📌 Limit trusted Composer plugins to a known list, allow user to add more Fixed 's https://git.drupalcode.org/project/automatic_updates/-/merge_requests/659, I believe I can do the same here.

(See #3320792-25: Make build tests fail 1) more explicitly, 2) earlier when possible (failing StatusCheckEvent subscribers) → through #3320792-27: Make build tests fail 1) more explicitly, 2) earlier when possible (failing StatusCheckEvent subscribers) → .)

First pushed a test. That should fail like this:

1) Drupal\Tests\automatic_updates_extensions\Build\ModuleUpdateTest::testApi
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-''
+'
+Drupal cannot be automatically updated from the installed version, 10.0.3-dev, because automatic updates from a dev version to any other version are not supported.'

As expected:

If I now extract/cherry-pick the solution from https://git.drupalcode.org/project/automatic_updates/-/merge_requests/65... (which finds its origin in https://git.drupalcode.org/project/automatic_updates/-/merge_requests/65...), we can see that … it does not pass tests.

Why did it work for 📌 Limit trusted Composer plugins to a known list, allow user to add more Fixed ? Probably because #3331168 did less strict assertions that assertStatusReportChecksSuccessful() (introduced by 📌 Make build tests fail 1) more explicitly, 2) earlier when possible (failing StatusCheckEvent subscribers) Fixed ) does.

So I started debugging … and found a mind-boggling rabbit hole. Put a breakpoint in update_get_available() and observe how many times it gets called. It's insane. There's call stacks like this:

update.module:324, update_get_available()
ProjectInfo.php:195, Drupal\package_manager\ProjectInfo->getAvailableProjects()
ProjectInfo.php:109, Drupal\package_manager\ProjectInfo->getInstallableReleases()
VersionPolicyValidator.php:236, Drupal\automatic_updates\Validator\VersionPolicyValidator->getAvailableReleases()
VersionPolicyValidator.php:119, Drupal\automatic_updates\Validator\VersionPolicyValidator->validateVersion()
ReleaseChooser.php:57, Drupal\automatic_updates\ReleaseChooser->Drupal\automatic_updates\{closure:/private/tmp/build_workspace_aaf970c10050f82aef4c7bae01d73e2e72cpBo/project/web/modules/contrib/automatic_updates/src/ReleaseChooser.php:56-58}()
ReleaseChooser.php:62, array_filter()
ReleaseChooser.php:62, Drupal\automatic_updates\ReleaseChooser->getInstallableReleases()
ReleaseChooser.php:84, Drupal\automatic_updates\ReleaseChooser->getMostRecentReleaseInMinor()
ReleaseChooser.php:121, Drupal\automatic_updates\ReleaseChooser->getLatestInInstalledMinor()
CronUpdater.php:135, Drupal\automatic_updates\CronUpdater->getTargetRelease()
VersionPolicyValidator.php:210, Drupal\automatic_updates\Validator\VersionPolicyValidator->getTargetVersion()
VersionPolicyValidator.php:149, Drupal\automatic_updates\Validator\VersionPolicyValidator->checkVersion()
ContainerAwareEventDispatcher.php:111, call_user_func:{/private/tmp/build_workspace_aaf970c10050f82aef4c7bae01d73e2e72cpBo/project/web/core/lib/Drupal/Component/EventDispatcher/ContainerAwareEventDispatcher.php:111}()
ContainerAwareEventDispatcher.php:111, Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch()
StatusCheckTrait.php:49, Drupal\automatic_updates\Validation\StatusChecker->runStatusCheck()
StatusChecker.php:105, Drupal\automatic_updates\Validation\StatusChecker->run()
automatic_updates.module:204, automatic_updates_modules_installed()
<snip>

Note how VersionPolicyValidator (which throws the error messages mentioned in #6 causing tests to fail on 10.x) ends up calling itself. And there's plenty more examples where that came from. The amount of unnecessary function calls is incredible. Now, of course, "premature optimization is the root of all evil", but at this point it interferes with debuggability and hence maintainability.

Another example:

ProjectInfo.php:149, Drupal\package_manager\ProjectInfo->getInstallableReleases()
VersionPolicyValidator.php:236, Drupal\automatic_updates\Validator\VersionPolicyValidator->getAvailableReleases()
VersionPolicyValidator.php:119, Drupal\automatic_updates\Validator\VersionPolicyValidator->validateVersion()
ReleaseChooser.php:57, Drupal\automatic_updates\ReleaseChooser->Drupal\automatic_updates\{closure:/private/tmp/build_workspace_aaf970c10050f82aef4c7bae01d73e2e72cpBo/project/web/modules/contrib/automatic_updates/src/ReleaseChooser.php:56-58}()
ReleaseChooser.php:62, array_filter()
ReleaseChooser.php:62, Drupal\automatic_updates\ReleaseChooser->getInstallableReleases()
ReleaseChooser.php:84, Drupal\automatic_updates\ReleaseChooser->getMostRecentReleaseInMinor()
ReleaseChooser.php:121, Drupal\automatic_updates\ReleaseChooser->getLatestInInstalledMinor()
CronUpdater.php:135, Drupal\automatic_updates\CronUpdater->getTargetRelease()
VersionPolicyValidator.php:210, Drupal\automatic_updates\Validator\VersionPolicyValidator->getTargetVersion()
VersionPolicyValidator.php:149, Drupal\automatic_updates\Validator\VersionPolicyValidator->checkVersion()
<snip>

ProjectInfo->getInstallableReleases() cannot return different results within the same PHP process by definition, yet it is recomputed countless times.

So I did not understand why CoreUpdateTest was not affected by this. It also has a assertStatusReportChecksSuccessful() and it passes tests, why?! What is it doing differently?

Turns out that both ModuleUpdateTest and CoreUpdateTest end up in \Drupal\automatic_updates\Validator\VersionPolicy\SupportedBranchInstalled::validate() looking at the real, actually requested Drupal core version info! 😬😳😱

Turns out that both CoreUpdateTest and ModuleUpdateTest use

    $this->setReleaseMetadata([
      'drupal' => __DIR__ . '/../../../../package_manager/tests/fixtures/release-history/drupal.9.8.1-security.xml',

… and setReleaseMetadata() writes this to settings.php: $config['update_test.settings']['xml_map'] = … → but the update_test module is never even installed! 😳😱

Reclassifying this as core-mvp because this severely undermined my trust in both of these tests

Compared to #6, the extracted/cherry-picked commit results in:

Finally found it!

Simplified further.

It looks like the edge case uncovered/investigated in #7 and #9 can also cause random failures: see #3325654-36: Improve the user experience of having your staged update deleted before it was applied → , and specifically https://www.drupal.org/pift-ci-job/2583990 → , which failed like this:

Drupal\Tests\automatic_updates_extensions\Build\ModuleUpdateTest
fail: [Other] Line 0 of sites/default/files/simpletest/phpunit-1.xml:
PHPUnit Test failed to complete; Error: PHPUnit 9.5.26 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\automatic_updates_extensions\Build\ModuleUpdateTest
.F                                                                  2 / 2 (100%)

Time: 03:13.888, Memory: 18.00 MB

There was 1 failure:

1) Drupal\Tests\automatic_updates_extensions\Build\ModuleUpdateTest::testUi
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-''
+'Error message There is a security update available for your version of Drupal. To ensure the security of your server, you should update immediately! See the available updates page for more information and to install your missing updates.'

/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:122
/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/IsIdentical.php:79
/var/www/html/modules/contrib/automatic_updates/automatic_updates_extensions/tests/src/Build/ModuleUpdateTest.php:123
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php:673
/var/www/html/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:661
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:144
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:97

FAILURES!
Tests: 2, Assertions: 162, Failures: 1.

This now blocks #3341708 — see #3341708-4: [PP-1] Update requirements for 3.x → .

Rebased the old MR onto 3.0.x and opened a new MR 👍

Wim Leers → closed merge request !683

Nope, doesn't block that anymore: #3341708-6: Update requirements for 3.x → .

@tedbow and @tim.plunkett discussed the plan for getting Automatic Updates, Project Browser and Package Manager into core. We're likely going to aim to land Package Manager first. So postponing this issue for now, until the dust settles on that.

This now blocks 🐛 Test failing on 8.x-2.x on 10.0.x and 10.1.x Fixed .

Update on #20: confirmed at #3358878-7: Test failing on 8.x-2.x on 10.0.x and 10.1.x → that the fix in this issue is essential.

I'll push this across the finish line, there's too much tricky past history at play here.

No time for this.

This issue was fixed in #3362746-7: fix 10.0.x phpstan errors → . It was needed to get the tests were passing after I fixed the phpstan issues. I probably shouldn't have committed in that issue but I think the solution is correct. '

Currently \Drupal\Tests\automatic_updates\Build\CoreUpdateTest::createTestProject calls

$this->setUpstreamCoreVersion('9.8.1');
    $this->setReleaseMetadata([
      'drupal' => __DIR__ . '/../../../package_manager/tests/fixtures/release-history/drupal.9.8.1-security.xml',
    ]);

Then later calls

$this->assertStatusReportChecksSuccessful();

To affirm all the status checks are passing after we have created the project.

Since ModuleUpdateTest extends UpdateTestBase this also fixes it for this that test.

So I think the only thing left to do on this issue add an extra call to assertStatusReportChecksSuccessful() to ensure that after \Drupal\Tests\automatic_updates_extensions\Build\ModuleUpdateTest::createTestProject has altered the XML fixtures in use that all thats status checks pass and there are no other errors before we start the tests.

I am removing core-mvp because we just have a problem left in automatic_updates_extensions

I don't see a problem with this. More checks and safety in our tests, especially our build tests, is a good thing.

🥳 Weird to see code you wrote in January landing suddenly 😄

Automatically closed - issue fixed for 2 weeks with no activity.