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.

Command icon 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:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kunal.sachdev created an issue. See original summary.

tedbow made their first commit to this issue’s fork.

tedbow’s picture

Status: Active » Needs work

Ok. 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.org

Our 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 fixture drupal.0.0.xml


$this->config('update_test.settings')
      ->set('xml_map', [
        'drupal' => '0.0',
      ])
      ->save();
    $this->config('update.settings')
      ->set('fetch.url', $this->baseUrl . '/automatic-update-test')
      ->save();

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.

Suresh Prabhu Parkala made their first commit to this issue’s fork.

tedbow’s picture

@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 in automatic_updates_cron() when we have have

if ($last_results === NULL || !$last_run_time || \Drupal::time()->getRequestTime() - $last_run_time > 3600) {
    $checker_manager->run();
  }

our 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:

  1. We are now extending AutomaticUpdatesTestBase
  2. This is necessary because we don't want the test to make real call to drupal.org to get the real XML. Currently in 8.x-2.x it is making real calls to drupal.org but this didn't cause a test failure because UpdateVersionValidator wasn't subscribing to AutomaticUpdatesEvents::READINESS_CHECK
  3. So to fake the XML we have to require update_test.module
  4. update_test has it's own test class \Drupal\update_test\Datetime\TestTime which tries to replace the datetime.time service just like we are with \Drupal\automatic_updates_test\Datetime\TestTime
  5. When both are update_test and automatic_update_test are enabled \Drupal\update_test\Datetime\TestTime is winning out to replace the datetime.time service

So basically we could fix this by having our \Drupal\automatic_updates_test\Datetime\TestTime be a service decorator instead replacing the datetime.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.

tedbow’s picture

tedbow’s picture

crediting myself for my work on this issue

tedbow’s picture

the 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

$results = $this->container->get('automatic_updates.readiness_validation_manager')
      ->run()
      ->getResults();

Is the 1 error it creates. this is true in 9.2 but in 9.3 the validator UpdateVersionValidator which now runs on AutomaticUpdatesEvents::READINESS_CHECK will also produce an error.

We get around this in functional tests by faking the XML which involves


$this->config('update_test.settings')
      ->set('xml_map', [
        'drupal' => '9.8.1',
      ])
      ->save();
    $this->config('update.settings')
      ->set('fetch.url', $this->baseUrl . '/automatic-update-test')
      ->save();

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 should

currently 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.

tedbow’s picture

Status: Needs work » Needs review
phenaproxima’s picture

Status: Needs review » Needs work

The 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.

phenaproxima’s picture

Status: Needs work » Needs review
tedbow’s picture

Status: Needs review » Reviewed & tested by the community

@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.

phenaproxima’s picture

Status: Reviewed & tested by the community » Needs work

phenaproxima’s picture

Status: Needs work » Fixed

I think we've iterated on this one enough; merged into 8.x-2.x. Thanks all!

Status: Fixed » Closed (fixed)

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