Problem/Motivation
In #3233521: Remove unnecessary call to checkForUpdates() in testTableLooksCorrect() in UpdaterFormTest we found this error that people can start the update even if there is an error that the current version is one minor version like 9.2.6 and the recommended version is a different minor version like 9.3.0.
Another potential problem is the updater does not give a warning if you start from a -dev version.
Proposed resolution
If the current version is one minor version like 9.2.6 and the recommended version is a different minor version like 9.3.0 then do not show the download updates button.
The UpdateVersionValidator subscribes to the AutomaticUpdatesEvents::PRE_START event but it should also subscribe to AutomaticUpdatesEvents::READINESS_CHECK.
Issue fork automatic_updates-3236299
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #4
tedbowOk. I see the problem with the latest test failure.
The tests fail if we test against core 9.3.x but pass if we test against 9.2.x
This is because the latest version of Drupal is 9.2.6. So our new validation throws an error because if test against Drupal core 9.3.x this is a different minor than 9.2.6.
This failure points to the fact that we are not always using our test XML fixture
drupal.0.0.xml
and we are not always faking our version of core. We should always be doing this. otherwise we are requesting real to drupal.org. This actually a problem in core itself too #3095755: Tests should not do requests to updates.drupal.orgOur tests should never depend on what actual real current version of core is according to Drupal.org xml and the test should never fail if we can't make a request to drupal.org because of network issues.
In
\Drupal\Tests\automatic_updates\Functional\ReadinessValidationTest
we can use our existing code\Drupal\Tests\automatic_updates\Functional\UpdaterFormTest::setCoreVersion
which we will need to move to AutomaticUpdatesTestBase or actually probably to trait so we can use both with our Functional and kernel tests.We will also need move this code in
\Drupal\Tests\automatic_updates\Functional\UpdaterFormTest::setUp()
which sets the test so it uses our test XML fixturedrupal.0.0.xml
to a trait to so we can use it in our kernel tests also. I am not sure if there would be anything else we need to do to get the kernel test to pass.
Comment #6
tedbow@kunal.sachdev and I spent a bunch of time debugging the remaining errors in
\Drupal\Tests\automatic_updates\Functional\ReadinessValidationTest
We narrowed it down to
\Drupal\automatic_updates_test\Datetime\TestTime::getRequestTime()
not being called in the test so inautomatic_updates_cron()
when we have haveour method in
\Drupal\Tests\automatic_updates\Functional\ReadinessValidationTest::delayRequestTime()
for faking delay so$checker_manager->run();
will run during cron not working.I finally figured out what is happening:
AutomaticUpdatesTestBase
UpdateVersionValidator
wasn't subscribing toAutomaticUpdatesEvents::READINESS_CHECK
update_test.module
\Drupal\update_test\Datetime\TestTime
which tries to replace thedatetime.time
service just like we are with\Drupal\automatic_updates_test\Datetime\TestTime
update_test
andautomatic_update_test
are enabled\Drupal\update_test\Datetime\TestTime
is winning out to replace thedatetime.time
serviceSo basically we could fix this by having our
\Drupal\automatic_updates_test\Datetime\TestTime
be a service decorator instead replacing thedatetime.time
service altogether.The long term solution in core is probably #3216087: Create test module to easily alter request time in tests so we have 1 way in core to alter the request time for tests.
Comment #7
tedbowComment #8
tedbowcrediting myself for my work on this issue
Comment #9
tedbowthe last 2 fails on 9.3 are in our kernel and they are pretty tricky to fix in a kernel test.
For example
\Drupal\Tests\automatic_updates\Kernel\ReadinessValidation\ComposerExecutableValidatorTest::testErrorIfComposerNotFound()
assert that the only error from
Is the 1 error it creates. this is true in 9.2 but in 9.3 the validator
UpdateVersionValidator
which now runs onAutomaticUpdatesEvents::READINESS_CHECK
will also produce an error.We get around this in functional tests by faking the XML which involves
The problem is the kernel tests don't have
$this->baseUrl
. This is set up for functional tests in\Drupal\Core\Test\FunctionalTestSetupTrait::setupBaseUrl()
. I am not sure if setting something like this up in kernel tests make sense because they are not meant to make browser request. In the Update xml test though we need a URL to request our own fixture XML. I don't think we shouldcurrently the kernel test is actually making a request to https://updates.drupal.org/release-history but this is not what is failing the tests. Core has the same problem #3095755: Tests should not do requests to updates.drupal.org
So I am not sure if we can properly fake the XML in kernel test and core should solve #3095755 not us.
So if we can't fake the XML for a kernel test I think it is fine remove
UpdateVersionValidator
for kernel tests as this is not what the kernel tests are testing. We could do this by removing the service via an alter in our test module, like https://www.drupal.org/docs/drupal-apis/services-and-dependency-injectio.... core/modules/media/tests/modules/media_test_oembed/src/MediaTestOembedServiceProvider.php is an example of altering a service in a test module where as we would remove it.Comment #10
tedbowComment #11
phenaproximaThe actual fix here seems straightforward enough, but the test changes are quite extensive and I'm not certain they're all in scope here.
I feel like we should merge #3239889: Add an offline kernel test of UpdateRecommender in first and then refactor some of the tests here, since that issue provides some helpful infrastructure for both kernel and functional tests to deal with release metadata. If I'm understanding this issue correctly, it would benefit from that.
Comment #12
phenaproximaComment #13
tedbow@phenaproxima I addressed on all your questions. If you are good with the explanations than I think this is good to merge.
I think having a separate
\Drupal\automatic_updates\Validator\UpdateVersionValidator::checkReadinessUpdateVersion
makes cleaner code but I am not married to it if you want to change it.Comment #14
phenaproximaComment #16
phenaproximaI think we've iterated on this one enough; merged into 8.x-2.x. Thanks all!