Problem/Motivation

\Drupal\FunctionalTests\Installer\InstallerTestBase tests currently enable the update module. This causes tests to fail - see https://www.drupal.org/pift-ci-job/2409126 - the fail message is:

Exception: Warning: Undefined array key "date"
update_calculate_project_update_status()() (Line: 528)

BUT the update module should not be installed according to \Drupal\Core\Test\FunctionalTestSetupTrait::installParameters(). So what's going on here?

The code in \Drupal\Core\Test\FunctionalTestSetupTrait::installParameters() is:

          // form_type_checkboxes_value() requires NULL instead of FALSE values
          // for programmatic form submissions to disable a checkbox.
          'enable_update_status_module' => NULL,
          'enable_update_status_emails' => NULL,

and while this works for a non-interactive install, it does not work in an interactive install. We need to set it to FALSE there. This is due to how we process the form input using \Drupal\Tests\BrowserTestBase::translatePostValues() - it removes NULL values completely :) - it's a feature of http_build_query() - see https://3v4l.org/7NHaH.

So how come this suddenly broke? It appears that this is due to the recent 10.1.x release. It is not a valid release because it could not be packaged - because it only contains the first commit from drupal. See https://www.drupal.org/project/drupal/releases/10.1.x-dev - so the update module can't work out the available release.

Steps to reproduce

Run \Drupal\FunctionalTests\Installer\InstallerTest on 10.0.x - it will fail - like lots of other tests.

Proposed resolution

Don't install the update module as part of a regular InstallerTestBase test. Override the install parameters to work for an interactive install.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

Status: Active » Needs review
FileSize
2.34 KB
alexpott’s picture

Issue summary: View changes
alexpott’s picture

alexpott’s picture

Fixing the config tests. The efforts to remove update module status checkbox there where wrong... this is down to http_build_query() - see issue summary.

We're still going to have test fails but I think we should commit this to have way less noise.

The last submitted patch, 2: 3291877-2.patch, failed testing. View results

longwave’s picture

  1. +++ b/core/tests/Drupal/FunctionalTests/BrowserTestBaseTest.php
    @@ -555,6 +555,9 @@ public function testCronRun() {
    +    $this->assertFalse(\Drupal::moduleHandler()->moduleExists('update'), 'The Update module is not installed.');
    
    +++ b/core/tests/Drupal/FunctionalTests/Installer/InstallerTest.php
    @@ -131,6 +131,9 @@ public function testInstalled() {
    +    $this->assertFalse($module_handler->moduleExists('update'), 'The Update module is not installed.');
    

    No real need to bother with custom assertion messages given that PHPUnit is usually quite clear already.

  2. +++ b/core/tests/Drupal/FunctionalTests/Installer/InstallerExistingConfigTestBase.php
    @@ -105,7 +105,8 @@ protected function installParameters() {
    -    unset($parameters['forms']['install_configure_form']['update_status_module']);
    

    Wonder if we should assert existence of the things we are removing first, to catch changes to the form in the future?

alexpott’s picture

Re #7.1 - I think assertTrue / assertFalse should always have messages. If this is TRUE PHPUnit will just say TRUE is not FALSE.
Re #7.2 - The form will error if we try to set a none existing field - this was all due to http_build_query() unexpectedly removing stuff. I don't think we need to worry about that.

Status: Needs review » Needs work

The last submitted patch, 5: 3291877-5.patch, failed testing. View results

alexpott’s picture

Status: Needs work » Needs review

This will still fail on 10.0.x but it reduces the fails from 70 to 6 so I think this is good approach irrespective of the fix in #3291893: Stub release for 10.1.x breaks 10.0.x tests in HEAD. which will fix the last 6 fails, as well as the other 64.

longwave’s picture

Status: Needs review » Reviewed & tested by the community

70 fails down to 6 is a vast improvement.

#8.1 is fine by me, for #7.2 I meant that previously we were unsetting a non-existent update_status_module instead of enable_update_status_module but the fix here is good enough.

  • catch committed 3e4df3a on 10.0.x
    Issue #3291877 by alexpott, longwave: Fix \Drupal\FunctionalTests\...

  • catch committed 740a7a8 on 9.5.x
    Issue #3291877 by alexpott, longwave: Fix \Drupal\FunctionalTests\...

  • catch committed 8aee240 on 9.4.x
    Issue #3291877 by alexpott, longwave: Fix \Drupal\FunctionalTests\...
catch’s picture

Version: 10.0.x-dev » 9.4.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 10.0.x, cherry-picked to 9.5.x and 9.4.x, thanks!

Status: Fixed » Closed (fixed)

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