Problem/Motivation

Discovered at #3111929-31: If no recommended update is found, Update Status recommends the latest release, even if it is unsupported ... the patch that was already RTBC would have broken this block of code from update.compare.inc:

    // First, if this is the existing release, check a few conditions.
    if ($project_data['existing_version'] === $version) {
      if (isset($release['terms']['Release type']) &&
          in_array('Insecure', $release['terms']['Release type'])) {
        $project_data['status'] = UpdateManagerInterface::NOT_SECURE;
      }
      elseif ($release['status'] == 'unpublished') {
        $project_data['status'] = UpdateManagerInterface::REVOKED;
        if (empty($project_data['extra'])) {
          $project_data['extra'] = [];
        }
        $project_data['extra'][] = [
          'class' => ['release-revoked'],
          'label' => t('Release revoked'),
          'data' => t('Your currently installed release has been revoked, and is no longer available for download. Disabling everything included in this release or upgrading is strongly recommended!'),
        ];
      }
      elseif (isset($release['terms']['Release type']) &&
              in_array('Unsupported', $release['terms']['Release type'])) {
        $project_data['status'] = UpdateManagerInterface::NOT_SUPPORTED;
        if (empty($project_data['extra'])) {
          $project_data['extra'] = [];
        }
        $project_data['extra'][] = [
          'class' => ['release-not-supported'],
          'label' => t('Release not supported'),
          'data' => t('Your currently installed release is now unsupported, and is no longer available for download. Disabling everything included in this release or upgrading is strongly recommended!'),
        ];
      }
    }

% grep -ri revoke core/modules/update/tests is empty. We have no test coverage of the above code block.

Proposed resolution

Add tests.

Remaining tasks

  1. Add tests:
    • NOT_SECURE See #11 and #16
    • REVOKED patch #4
    • NOT_SUPPORTED patch #10
  2. Review.
  3. RTBC.
  4. Commit.

User interface changes

N/A

API changes

N/A

Data model changes

N/A

Release notes snippet

N/A

CommentFileSizeAuthor
#66 3113292-66-8.8.patch30.15 KBtedbow
#61 3113292-58-8.8.x-reroll.patch29.46 KBtedbow
#58 3113292.55_58.interdiff.txt422 bytesdww
#58 3113292-58.88x.patch30.42 KBdww
#55 3113292.44_55.rawdiff.txt790 bytesdww
#55 3113292-55-88x.patch30.46 KBdww
#44 3113292-44.patch30.57 KBdww
#42 3113292.35_41.interdiff.txt15.15 KBdww
#42 3113292.40_41.interdiff.txt3.28 KBdww
#41 3113292-41.patch33.46 KBdww
#40 3113292.35_40.interdiff.txt14.89 KBdww
#40 3113292-40.do-not-test.patch30.57 KBdww
#35 3113292-35.patch29.45 KBtedbow
#35 interdiff-19-35.txt8.77 KBtedbow
#30 3111929-fix-in-34.diff1.24 KBtedbow
#26 3113292.19_25.interdiff.txt13.31 KBdww
#26 3113292-25.FAIL-with-3111929-29.patch30.78 KBdww
#19 3113292-19.patch25.67 KBtedbow
#19 interdiff-17-19.txt1.67 KBtedbow
#17 3113292-17.patch25.45 KBtedbow
#17 interdiff-15-17.txt625 bytestedbow
#15 3113292.14_15.interdiff.txt19.68 KBdww
#15 3113292-15.patch26.67 KBdww
#14 3113292-14.patch24.65 KBtedbow
#14 interdiff-10-14.txt1.15 KBtedbow
#13 Screen Shot 2020-02-18 at 2.32.26 PM.png19.44 KBtedbow
#10 3113292.9_10.interdiff.txt19.1 KBdww
#10 3113292.4_10.interdiff.txt21.69 KBdww
#10 3113292-10.patch25.81 KBdww
#9 3113292.4_9.rawdiff.txt5.53 KBdww
#9 3113292.4_9.interdiff.txt13.5 KBdww
#9 3113292-9.do-not-test.patch19.86 KBdww
#5 3113292-4-with-3111929-bug-FAIL.patch21.42 KBtedbow
#5 interdiff-4-5.txt3.95 KBtedbow
#4 3113292-4.patch17.47 KBtedbow
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dww created an issue. See original summary.

dww’s picture

dww’s picture

Title: Update module has no tests for 'revoked' releases » Update module has no tests for changes to status of the installed release (revoked, etc)
Issue summary: View changes

Not just revoked. Better scope / title.

tedbow’s picture

Status: Active » Needs review
FileSize
17.47 KB

Here are some tests that will break with bug on #3111929-31: If no recommended update is found, Update Status recommends the latest release, even if it is unsupported.

These test make sure "Revoked" and "Security Update Required" show even if the branch you are on is currently unsupported.

tedbow’s picture

dww’s picture

Status: Needs review » Needs work

Excellent start! Very glad to see these test cases added...

  1. +++ b/core/modules/update/tests/modules/update_test/aaa_update_test.1_0-supported.xml
    @@ -0,0 +1,66 @@
    +    <tag>DRUPAL-8--2-0</tag>
    

    These are driving me nuts. ;) I'm going to open a follow-up to purge this from all our fixtures. <tag> is totally ignored by all our tests, and all of Update module.

  2. +++ b/core/modules/update/tests/modules/update_test/aaa_update_test.1_0-supported.xml
    @@ -0,0 +1,66 @@
    +    <version>8.x-2.0-beta1</version>
    +    <tag>DRUPAL-8--2-0</tag>
    

    Yeah, more of same. :/

  3. +++ b/core/modules/update/tests/src/Functional/UpdateTestBase.php
    @@ -204,4 +204,46 @@ protected function assertVersionUpdateLinks($label, $version, $download_version
    +   * Confirms messages are correct when messages are correct.
    

    @todo ;)

  4. +++ b/core/modules/update/tests/src/Functional/UpdateTestBase.php
    @@ -204,4 +204,46 @@ protected function assertVersionUpdateLinks($label, $version, $download_version
    +   * @param string $newer_version
    +   *   The expected new_version version.
    

    "The expected newer version"?

  5. +++ b/core/modules/update/tests/src/Functional/UpdateTestBase.php
    @@ -204,4 +204,46 @@ protected function assertVersionUpdateLinks($label, $version, $download_version
    +   * @param $new_version_label
    +   *   The ex
    

    @todo

  6. +++ b/core/modules/update/tests/src/Functional/UpdateTestBase.php
    @@ -204,4 +204,46 @@ protected function assertVersionUpdateLinks($label, $version, $download_version
    +  /**
    +   * Asserts that the update table text does contain the specified text.
    +   *
    +   * @param string $text
    +   *   The expected text.
    +   */
    +  protected function assertUpdateTableTextContains($text) {
    +    $this->assertSession()->elementTextContains('css', $this->updateTableLocator, $text);
    +  }
    +
    +  /**
    +   * Asserts that the update table does contain the specified text.
    +   *
    +   * @param string $text
    +   *   The expected text.
    +   */
    +  protected function assertUpdateTableContains($text) {
    +    $this->assertSession()
    +      ->elementContains('css', $this->updateTableLocator, $text);
    +  }
    +
    

    I don't understand the difference between these. Not clear why we need both. Let's either standardize on one, or add better comments explaining the difference.

Thanks,
-Derek

dww’s picture

Priority: Normal » Major
Issue tags: -Needs tests

Re: #6 1 and 2: #3113798: Remove unused (and generally wrong) <tag> markup from Update module test XML fixtures

Also, removing 'Needs tests' tag here, since #4 includes the test coverage we were missing.
Edit: I was wrong, see below.

Finally, bumping to major since this is/should be blocking a critical.

dww’s picture

Issue summary: View changes
Issue tags: +Needs tests

Crossing off some of the needed tests from the summary.

However, closer review shows we're still not testing this case / message:

      elseif (isset($release['terms']['Release type']) &&
              in_array('Unsupported', $release['terms']['Release type'])) {
        $project_data['status'] = UpdateManagerInterface::NOT_SUPPORTED;
        if (empty($project_data['extra'])) {
          $project_data['extra'] = [];
        }
        $project_data['extra'][] = [
          'class' => ['release-not-supported'],
          'label' => t('Release not supported'),
          'data' => t('Your currently installed release is now unsupported, and is no longer available for download. Disabling everything included in this release or upgrading is strongly recommended!'),
        ];
      }

So leaving that one not crossed off, and restoring the 'Needs tests' tag.

Thanks!
-Derek

dww’s picture

This fixes all of #6 except point 6.
Using do-not-test since it's still incomplete...
Interdiff is again a bit confused, so I'm also attaching a raw diff of the 2 patch files. /shrug

dww’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
25.81 KB
21.69 KB
19.1 KB

This adds test coverage for individual releases marked unsupported. This all passes locally, so I'm going to let the bot chew on it, too.

Interdiffs (such as they are) from both #4 and #9.

Upon even closer inspection, I don't see where we're testing the case of the currently-installed release being marked as 'Insecure'. We've got lots of coverage of various cases where a newer release is a security update, which should basically always happen if an older release is marked 'Insecure'. But still, for completeness, maybe we want to add some explicit coverage of that here?

tedbow’s picture

  1. re #10

    don't see where we're testing the case of the currently-installed release being marked as 'Insecure'.

    We are testing this in \Drupal\Tests\update\Functional\UpdateCoreTest::testSecurityUpdateAvailability() and \Drupal\Tests\update\Functional\UpdateContribTest::testSecurityUpdateAvailability()

    They both use \Drupal\Tests\update\Functional\UpdateTestBase::assertSecurityUpdates() which explicitly checks

    if ($expected_update_message_type === static::SECURITY_UPDATE_REQUIRED) {
       $assert_session->elementTextNotContains('css', $update_element_css_locator, 'Update available');
       $assert_session->elementTextContains('css', $update_element_css_locator, 'Security update required!');
        $assert_session->responseContains('error.svg', 'Error icon was found.');
    }
    

    The 2nd to last assert is based on the site being an insecure release. Just because there is newer security release it doesn't mean the installed release is in security. For instance

    • a site is 8.x-1.2
    • 8.x-1.3 introduces a security bug
    • 8.x-1.4 is a security fix

    In this case 8.x-1.2 may still not be Insecure

    testSecurityUpdateAvailability() covers many variations of this.

    In update_calculate_project_update_status() if you comment out

    if (isset($release['terms']['Release type']) &&
        in_array('Insecure', $release['terms']['Release type'])) {
       $project_data['status'] = UpdateManagerInterface::NOT_SECURE;
    }
    

    Multiple testcases for this method will fail.

tedbow’s picture

re #6.6

This is following the existing pattern in a lot of the update tests where we use assertText() to specify text that should be on the and user should see and $this->assertRaw('check.svg', 'Check icon was found.'); for when we are checking for markup that should be on the page but is not text the user should be reading.

This assertText() and assertRaw() work fines for core because there is only 1 update table on the page so we don't have worry that expected text or markup being on the page but being in the other update table and therefore giving a false pass. So tests that need to apply to both core and contrib we are mostly not using assertText() and assertRaw().

The test methods added here are going against contrib and core so I think it safer to test expliciting that the expected text/marked is on the element in \Drupal\Tests\update\Functional\UpdateTestBase::$updateTableLocator rather than just on the page.

tedbow’s picture

  1. +++ b/core/modules/update/tests/src/Functional/UpdateContribTest.php
    @@ -707,6 +707,60 @@ public function securityUpdateAvailabilityProvider() {
    +    $this->confirmUnsupportedStatus('8.x-1.1', '8.x-2.0', 'Also available:');
    +
    +    $this->refreshUpdateStatus([
    +      'drupal' => '0.0',
    +      $this->updateProject => '1_0-unsupported',
    +    ]);
    +    $this->confirmUnsupportedStatus('8.x-1.1', '8.x-2.0', 'Recommended version:');
    

    The fact that 8.x-2.0 is shown as "Also available" points to another problem.

    In this case there is no other update shown to the user so it should be recommended

    Created #3114408: Update module labels updates as "Also available" even when there are no other possible updates

  2. +++ b/core/modules/update/tests/src/Functional/UpdateContribTest.php
    @@ -707,6 +707,60 @@ public function securityUpdateAvailabilityProvider() {
    +    $this->confirmRevokedStatus('8.x-1.0', '8.x-2.0', 'Also available:');
    +
    +    $this->refreshUpdateStatus([
    +      'drupal' => '0.0',
    +      $this->updateProject => '1_0-unsupported',
    +    ]);
    +    $this->confirmRevokedStatus('8.x-1.0', '8.x-2.0', 'Recommended version:');
    

    Here 2 it should not be shown as "also available" 8.x-1.1 is not shown to the user because it is "unsupported"

    Here is screenshot
    next major as also available

tedbow’s picture

  1. +++ b/core/modules/update/tests/src/Functional/UpdateTestBase.php
    @@ -204,4 +204,69 @@ protected function assertVersionUpdateLinks($label, $version, $download_version
    +   * @param $new_version_label
    

    Needs string parameter type

  2. +++ b/core/modules/update/tests/src/Functional/UpdateTestBase.php
    @@ -204,4 +204,69 @@ protected function assertVersionUpdateLinks($label, $version, $download_version
    +   * @param $new_version_label
    

    Needs string parameter type

dww’s picture

Re: #11: I'll have to dig closer when I have more time. Regardless, I don't think it's necessary to hold this up any more for that.

Re: #12: All makes sense, thanks. I just think we need better method names / comments to clarify the difference. See attached. Also more consistently formatting these two methods so neither one wraps more than 80 chars.

Re: #13: Good catch. Following. ;)

Re: #14: Good catches! Test bot also complained about an indentation bug in #10. Also fixed here.

Finally, based on Slack exchange, @tedbow and I agreed to restore/fix the <tag> entries in the fixtures for this patch, and leave removing them entirely to #3113798: Remove unused (and generally wrong) <tag> markup from Update module test XML fixtures. We might as well try to remain consistent, and if this lands but that doesn't, we'd be in a slightly weird state. Thankfully, that's trivial to re-roll with a perl 1-liner, so it's easy to refresh that patch after this lands.

p.s. Interdiff is again being weird. *sigh*

tedbow’s picture

+++ b/core/modules/update/tests/src/Functional/UpdateCoreTest.php
@@ -305,6 +305,15 @@ public function securityUpdateAvailabilityProvider() {
+      // No security release available for site minor release 0.
+      // Site minor is not a supported branch.
+      // Security release available for next minor.
+      '0.0, 1.2, insecure-unsupported' => [
+        'site_patch_version' => '0.0',
+        'expected_security_releases' => ['1.2'],
+        'expected_update_message_type' => static::SECURITY_UPDATE_REQUIRED,
+        'fixture' => 'sec.1.2_insecure-unsupported',
+      ],

I forgot to mention this in #11 when I was explaining that we do have test coverage for a site having a insecure release installed.

I add this test case which basically the test case before it but with installed release not only insecure but also on a unsupported branch. We still want to display "Security Update Required". This test case confirms it.

This cass would fail with the patch in #3111929-31: If no recommended update is found, Update Status recommends the latest release, even if it is unsupported

tedbow’s picture

+++ b/core/modules/update/tests/src/Functional/UpdateTestBase.php
@@ -204,4 +204,74 @@ protected function assertVersionUpdateLinks($label, $version, $download_version
+   * Asserts that the update table visible text contains the specified text.

1 final nit I think.

\Behat\Mink\WebAssert::elementTextContains() which this method uses doesn't actually check if the text is visible.

There are various reasons text could be not visible such as if it is out of the viewport, if the element is hidden via CSS, etc.

I don't think our function tests take this into account but we do in Javascript tests. But not to confuse developers into thinking we actually do/can check visibility here I think we should remove "visible".

dww’s picture

Issue summary: View changes

Re: #16 Duly noted. I'll dig later. Waist-deep in other (non-Drupal) muck right now. Happy for this to proceed as-is.

Re: #17 fair enough. I was trying to find the right adjective to concisely describe "human readable output text" and went with "visible", but I agree that potentially confuses the matter for all the reasons you laid out. +1 to #17. I believe my changes to the name and docs for assertUpdateTableElementContains() from #15 are sufficient to avoid confusion between the two methods now.

Minor updates to remaining tasks. Since @tedbow and I have reviewed each other's stuff closely, I think it's safe to cross that out. Sadly, neither of us feel qualified to RTBC at this point, so leaving that for someone else...

Thanks!
-Derek

tedbow’s picture

  1. +++ b/core/modules/update/tests/modules/update_test/drupal.1.0-unsupported.xml
    @@ -3,7 +3,7 @@
    -<supported_branches>8.0.,8.1.</supported_branches>
    +<supported_branches>8.1</supported_branches>
    

    @tim.plunkett pointed this out
    The last period was left off the branch. Should be 8.1..

    It still passes because of this is how determine if a version is in a branch

    $is_in_supported_branch = function ($version) use ($supported_branches) {
      foreach ($supported_branches as $supported_branch) {
       if (strpos($version, $supported_branch) === 0) {
        return TRUE;
       }
     }
     return FALSE;
    };
    

    So version still starts with the branch string.

  2. +++ b/core/modules/update/tests/src/Functional/UpdateContribTest.php
    @@ -706,6 +706,60 @@ public function securityUpdateAvailabilityProvider() {
    +    $this->confirmRevokedStatus('8.x-1.0', '8.x-2.0', 'Also available:');
    ...
    +    $this->confirmUnsupportedStatus('8.x-1.1', '8.x-2.0', 'Also available:');
    

    These need a todo to change to "Recommended version" in https://www.drupal.org/project/drupal/issues/3114408

xjm’s picture

Priority: Major » Critical
xjm’s picture

This is blocked on #3111929: If no recommended update is found, Update Status recommends the latest release, even if it is unsupported, because our changes to that issue introduced a regression to this functionallity.

dww’s picture

Re: #19 Interdiff all looks good. Nice fixes. RTBC+1, but I don't think I should actually change the status...

Re: #20, #21: Cool, agreed. ;)

Cheers,
-Derek

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

As pointed out in #19 I discussed this issue with @tedbow. I've reviewed all the patches and interdiffs going back to #14, and I agree with #22 that this is ready to go!

xjm’s picture

Status: Reviewed & tested by the community » Needs review

Overall this looks great. Just a few questions, some of which I answered myself in the course of review:

  1. Can we attach a failing patch (using the bug from the other issue) to prove the coverage here, or is the goal to add additional coverage in the other issue?

  2. +++ b/core/modules/update/tests/modules/update_test/aaa_update_test.1_0-supported.xml
    @@ -0,0 +1,80 @@
    +      <term><name>Release type</name><value>Bug fixes</value></term>
    +    </terms>
    ...
    +  <release>
    +    <name>aaa_update_test 8.x-1.0</name>
    +    <version>8.x-1.0</version>
    +    <tag>8.x-1.0</tag>
    +    <status>unpublished</status>
    +    <release_link>http://example.com/aaa_update_test-8-x-1-0-release</release_link>
    +    <download_link>http://example.com/aaa_update_test-8.x-1.0.tar.gz</download_link>
    +    <date>1250404521</date>
    +    <terms>
    +      <term><name>Release type</name><value>New features</value></term>
    +      <term><name>Release type</name><value>Bug fixes</value></term>
    +    </terms>
    +  </release>
    

    In this scenario, the unpublished release has a lower version number than the latest supported release. Should we also have a case foe when it is newer? Found it, see point 5.

  3. +++ b/core/modules/update/tests/modules/update_test/aaa_update_test.1_0-unsupported.xml
    @@ -0,0 +1,80 @@
    +  <release>
    +    <name>aaa_update_test 8.x-1.0</name>
    +    <version>8.x-1.0</version>
    +    <tag>8.x-1.0</tag>
    +    <status>unpublished</status>
    +    <release_link>http://example.com/aaa_update_test-8-x-1-0-release</release_link>
    +    <download_link>http://example.com/aaa_update_test-8.x-1.0.tar.gz</download_link>
    +    <date>1250404521</date>
    +    <terms>
    +      <term><name>Release type</name><value>New features</value></term>
    +      <term><name>Release type</name><value>Bug fixes</value></term>
    +    </terms>
    +  </release>
    

    Wait... isn't this an identical scenario to the previous fixture? What did I miss?

  4. +++ b/core/modules/update/tests/modules/update_test/drupal.1.0-unsupported.xml
    @@ -49,10 +49,37 @@
    + <release>
    +   <name>Drupal 8.0.2</name>
    +   <version>8.0.2</version>
    +   <tag>8.0.2</tag>
    +   <status>unpublished</status>
    +   <release_link>http://example.com/drupal-8-0-2-release</release_link>
    

    Same question here (where it's the oldest release that's unpublished). I think we should have scenarios where the newer releases are unpublished as well.

  5. +++ b/core/modules/update/tests/modules/update_test/drupal.1.0.xml
    @@ -49,6 +49,33 @@
    + <release>
    +   <name>Drupal 8.0.2</name>
    +   <version>8.0.2</version>
    +   <tag>8.0.2</tag>
    +   <status>unpublished</status>
    +   <release_link>http://example.com/drupal-8-0-2-release</release_link>
    +   <download_link>http://example.com/drupal-8-0-2.tar.gz</download_link>
    +   <date>1250424581</date>
    +   <terms>
    +     <term><name>Release type</name><value>New features</value></term>
    +     <term><name>Release type</name><value>Bug fixes</value></term>
    +   </terms>
    + </release>
      <release>
        <name>Drupal 8.0.1</name>
        <version>8.0.1</version>
    

    Ah, here we go, this is the scenario I was asking for. An unpublished release between two published ones.

  6. Do we have coverage for a revoked release between two supported releases?

  7. +++ b/core/modules/update/tests/modules/update_test/drupal.sec.1.2_insecure-unsupported.xml
    @@ -0,0 +1,98 @@
    +  <tag>8.1.0</tag>
    +  <status>published</status>
    +  <release_link>http://example.com/drupal-8-1-0-release</release_link>
    +  <download_link>http://example.com/drupal-8-1-0.tar.gz</download_link>
    +  <date>1250424521</date>
    +  <terms>
    +   <term><name>Release type</name><value>New features</value></term>
    +   <term><name>Release type</name><value>Bug fixes</value></term>
    +   <term><name>Release type</name><value>Insecure</value></term>
    +  </terms>
    + </release>
    + <release>
    +  <name>Drupal 8.0.2</name>
    +  <version>8.0.2</version>
    +  <tag>8.0.2</tag>
    +  <status>published</status>
    +  <release_link>http://example.com/drupal-8-0-2-release</release_link>
    +  <download_link>http://example.com/drupal-8-0-2.tar.gz</download_link>
    +  <date>1250424641</date>
    +  <terms>
    +   <term><name>Release type</name><value>New features</value></term>
    +   <term><name>Release type</name><value>Bug fixes</value></term>
    +   <term><name>Release type</name><value>Insecure</value></term>
    +  </terms>
    + </release>
    

    There's no scenario here for the extended security coverage scenario where 8.0.2 is secure but 8.1.0 is not. I hope though that we added coverage for that originally when we first started supporting this.

    If we don't have further test coverage for that it can be a followup as it's not related to the bug from the issue.

tedbow’s picture

re #24

  1. Yes I tried to do that in #5 but I realize I got the drupalci changes wrong. Here is new 1 with the bug and #19
  2. ok
  3. No
    aaa_update_test.1_0-supported.xml has <supported_branches>8.x-1.,8.x-2.</supported_branches>

    aaa_update_test.1_0-unsupported.xml has <supported_branches>8.x-2.</supported_branches>

    So in 1 case the branch they are on is supported and 1 it is not. The tests are to make sure the "Revoked" and "Release not supported:" messages show up in both cases.

  4. see above
  5. ok
  6. The only bug that was introduced in #3111929: If no recommended update is found, Update Status recommends the latest release, even if it is unsupported was when the currently installed release is either unpublished, unsupported or insecure. The only checks it caused to be skipped were checks on the currently installed release no others.
  7. I am not sure what you are asking about. This test is not dealing with "extended security coverage".

    The extra test case added to securityUpdateAvailabilityProvider() was '0.0, 1.2, insecure-unsupported'. It uses the new fixture drupal.sec.1.2_insecure-unsupported.xml.
    The test case is identical to the existing case ''0.0, 1.2, insecure" except that the new fixture has different support_branches value. So the installed version is not in a supported branch. The new test case ensures that "Security Update Required" message still shows in that case.

    There's no scenario here for the extended security coverage scenario where 8.0.2 is secure but 8.1.0 is not.

    I would have to know the full details of the scenario you are describing to know if it is covered by securityUpdateAvailabilityProvider(). There are 17 test cases in that provider

dww’s picture

@xjm re: #24:

Thanks for reviewing!

1: Here you go. This is patch #19 from here with the changes to update.compare.inc from #3111929-29: If no recommended update is found, Update Status recommends the latest release, even if it is unsupported, plus drupalci.yml hacks to only run update tests to save cycles.

Local results:

There were 3 errors:

1) Drupal\Tests\update\Functional\UpdateContribTest::testCoreCompatibilityMessage
Error: Call to a member function getParent() on null

/.../drupal-8_9/core/modules/update/tests/src/Functional/UpdateContribTest.php:779
/.../htdocs/drupal-8_9/core/modules/update/tests/src/Functional/UpdateContribTest.php:568

2) Drupal\Tests\update\Functional\UpdateContribTest::testRevokedRelease
Behat\Mink\Exception\ElementTextException: The text "Revoked!" was not found in the text of the element matching css "table.update:nth-of-type(2)".

/.../htdocs/drupal-8_9/vendor/behat/mink/src/WebAssert.php:821
/.../htdocs/drupal-8_9/vendor/behat/mink/src/WebAssert.php:467
/.../htdocs/drupal-8_9/core/modules/update/tests/src/Functional/UpdateTestBase.php:261
/.../htdocs/drupal-8_9/core/modules/update/tests/src/Functional/UpdateTestBase.php:222
/.../htdocs/drupal-8_9/core/modules/update/tests/src/Functional/UpdateContribTest.php:735

3) Drupal\Tests\update\Functional\UpdateContribTest::testUnsupportedRelease
Behat\Mink\Exception\ElementTextException: The text "Release not supported: Your currently installed release is now unsupported, and is no longer available for download. Disabling everything included in this release or upgrading is strongly recommended!" was not found in the text of the element matching css "table.update:nth-of-type(2)".

/.../htdocs/drupal-8_9/vendor/behat/mink/src/WebAssert.php:821
/.../htdocs/drupal-8_9/vendor/behat/mink/src/WebAssert.php:467
/.../htdocs/drupal-8_9/core/modules/update/tests/src/Functional/UpdateTestBase.php:261
/.../htdocs/drupal-8_9/core/modules/update/tests/src/Functional/UpdateTestBase.php:247
/.../htdocs/drupal-8_9/core/modules/update/tests/src/Functional/UpdateContribTest.php:764

ERRORS!
Tests: 19, Assertions: 379, Errors: 3.

So yes, that really was a bug. :p If the revoked release is in an unsupported branch, we wouldn't mark it revoked with the original "fix" over there. ;)

p.s. Interdiff continues to be weird for me on this issue. I guess it's getting confused with the new files being added. /shrug

2: 👍

3:

Wait... isn't this an identical scenario to the previous fixture? What did I miss?
diff -up aaa_update_test.1_0-supported.xml aaa_update_test.1_0-unsupported.xml
@@ -3,7 +3,7 @@
 <title>AAA Update test</title>
 <short_name>aaa_update_test</short_name>
 <dc:creator>Drupal</dc:creator>
-<supported_branches>8.x-1.,8.x-2.</supported_branches>
+<supported_branches>8.x-2.</supported_branches>
 <project_status>published</project_status>
 <link>http://example.com/project/aaa_update_test</link>
   <terms>

So aaa_update_test.1_0-unsupported.xml is all the same release history, only with the 8.x-1. branch unsupported.

This speaks to something that's been concerning me, too. I'd love it if we had clear comments at the top of each update test XML fixture explaining the scenarios that fixture provides... Trying to track all this stuff in your head is next to impossible.

4.

I think we should have scenarios where the newer releases are unpublished as well.

Probably wise. @todo

5. 👍

6. Nope. Also @todo

7. Not sure, @tedbow would know better. Sounds like this isn't a blocker for this issue, but I'll let Ted either explain the existing coverage or add the appropriate follow-up.

So, NW for 4 and 6. I'll let the bot mark it as such when this FAIL patch fails. ;)

Cheers,
-Derek

The last submitted patch, , failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 26: 3113292-25.FAIL-with-3111929-29.patch, failed testing. View results

tedbow’s picture

re #29

This speaks to something that's been concerning me, too. I'd love it if we had clear comments at the top of each update test XML fixture explaining the scenarios that fixture provides... Trying to track all this stuff in your head is next to impossible.

I agree though maybe we should open an issue to this to all the XML at once. Also it can be tricky because the same XML can be used in multiple test method and the scenarios are different based on the installed version.

Re

So, NW for 4 and 6. I'll let the bot mark it as such when this FAIL patch fails. ;)

This refers to review in #24

I don't think we need more test coverage. The bug that was introduced in the original was pretty simple. I checked if the released was not in supported branch and continue onto the next release in the loop too earlier. I skipped this logic.

// First, if this is the existing release, check a few conditions.
if ($project_data['existing_version'] === $version) {
  if (isset($release['terms']['Release type']) &&
      in_array('Insecure', $release['terms']['Release type'])) {
    $project_data['status'] = UpdateManagerInterface::NOT_SECURE;
  }
  elseif ($release['status'] == 'unpublished') {
    $project_data['status'] = UpdateManagerInterface::REVOKED;
    if (empty($project_data['extra'])) {
      $project_data['extra'] = [];
    }
    $project_data['extra'][] = [
      'class' => ['release-revoked'],
      'label' => t('Release revoked'),
      'data' => t('Your currently installed release has been revoked, and is no longer available for download. Disabling everything included in this release or upgrading is strongly recommended!'),
    ];
  }
  elseif (isset($release['terms']['Release type']) &&
          in_array('Unsupported', $release['terms']['Release type'])) {
    $project_data['status'] = UpdateManagerInterface::NOT_SUPPORTED;
    if (empty($project_data['extra'])) {
      $project_data['extra'] = [];
    }
    $project_data['extra'][] = [
      'class' => ['release-not-supported'],
      'label' => t('Release not supported'),
      'data' => t('Your currently installed release is now unsupported, and is no longer available for download. Disabling everything included in this release or upgrading is strongly recommended!'),
    ];
  }
}

So the logic that was skipped only dealt with the installed version, no other versions. And the checks skipped dealt with 3 specific cases, unpublished, release type == unsupported and release type == insecure.

In the fix that @dww figure out we still continued on to the next release just after these checks.
I will attach a diff that goes from original bug to the fix.

If we need more tests cases I think we need explanation for why and what the exact scenario is.

tedbow’s picture

FileSize
1.24 KB

Here is the diff between the original bug and the fix

tedbow’s picture

+++ b/core/modules/update/update.compare.inc
@@ -388,6 +382,7 @@ function update_calculate_project_update_status(&$project_data, $available) {
     if ($release['status'] == 'unpublished' ||
+        !$is_in_supported_branch($release['version']) ||
         (isset($release['terms']['Release type']) &&
          (in_array('Insecure', $release['terms']['Release type']) ||
           in_array('Unsupported', $release['terms']['Release type'])))) {

Inside this if block we have another continue.

So we are still not doing any more consideration of the release.

xjm’s picture

So in 1 case the branch they are on is supported and 1 it is not. The tests are to make sure the "Revoked" and "Release not supported:" messages show up in both cases.

Can we put comments in the test to this effect, including comments that describe the contents of the fixtures? For all the scenarios added. (Not in the fixtures, in the tests.) Otherwise it's really hard to parse out what is supposed to be tested. Usually we'd put comments in a data provider to check it, but in this case that's not an option. However, "regardless of whether ____ is ____ or not" doesn't tell you which of those scenarios is the one being checked and could imply either. So the comments aren't giving the needed info there.

Regarding:

I am not sure what you are asking about. This test is not dealing with "extended security coverage".
The extra test case added to securityUpdateAvailabilityProvider() was '0.0, 1.2, insecure-unsupported'. It uses the new fixture drupal.sec.1.2_insecure-unsupported.xml.
The test case is identical to the existing case ''0.0, 1.2, insecure" except that the new fixture has different support_branches value. So the installed version is not in a supported branch. The new test case ensures that "Security Update Required" message still shows in that case.

There's no scenario here for the extended security coverage scenario where 8.0.2 is secure but 8.1.0 is not.

I would have to know the full details of the scenario you are describing to know if it is covered by securityUpdateAvailabilityProvider(). There are 17 test cases in that provider

Weird, I thought you worked on that with me? #2804155: If the next minor version of core has a security release, status still says "Security update required!" even if the site is on an equivalent, secure release already Under our policy, the typical pattern for core releases is that (say) 8.7.11 and 8.7.12 will be secure, 8.8.0 will be insecure, and 8.8.1 through 8.8.3 will be secure. I confirmed the original issue already introduced the correct test coverage for that, so no followup is needed.

I don't think we need more test coverage. The bug that was introduced in the original was pretty simple. I checked if the released was not in supported branch and continue onto the next release in the loop too earlier. I skipped this logic.

[...]

So the logic that was skipped only dealt with the installed version, no other versions. And the checks skipped dealt with 3 specific cases, unpublished, release type == unsupported and release type == insecure.

The title of the issue implied this issue was adding test coverage for revoked releases "etc.". It didn't imply it was "adding test coverage only for cases covered by a specific regression in the other issue". I'm fine with it as-is too I guess, but I am not sure I agree with the pushback.

Leaving NW for better comments inside the tests.

tedbow’s picture

Assigned: Unassigned » tedbow

working on this

dww’s picture

Re: #29:

I agree though maybe we should open an issue to this to all the XML at once. Also it can be tricky because the same XML can be used in multiple test method and the scenarios are different based on the installed version.

Completely agree. I don't think we want to do it here, just raising it as a concern.

Re: #32:

The title of the issue implied this issue was adding test coverage for revoked releases "etc.". It didn't imply it was "adding test coverage only for cases covered by a specific regression in the other issue".

I added "(revoked, etc)" in #3 because it's not just unpublished, but also "Not supported" and "Insecure" release types. Seemed clumsy to enumerate them all in the title. But in all cases, all update.module cares about is if any of these conditions apply to your currently-installed version (which is what @tedbow is getting at).

That said, I'm not opposed to Moar Tests(tm) for the other scenarios you're asking for, I just don't think they're critical right now. The critical thing is that we don't have this coverage for an important feature / behavior, and we almost committed a change that would have broken it. The feature in question is 100% about your currently-installed version, which is why the coverage in this issue is sufficient to cover the code blocks mentioned in the summary.

The other tests would be nice for end-to-end testing to make sure the whole system is working as expected, but they're not going to exercise the code block in the summary.

Thanks,
-Derek

tedbow’s picture

Status: Needs work » Needs review
FileSize
8.77 KB
29.45 KB

Here is some hopefully better comments. I have talked with @xjm about #32 and will comment next on why I think the existing test cases are sufficient.

tedbow’s picture

  1. re #32
    Can we put comments in the test to this effect, including comments that describe the contents of the fixtures? For all the scenarios added

    I added this in #35

  2. Under our policy, the typical pattern for core releases is that (say) 8.7.11 and 8.7.12 will be secure, 8.8.0 will be insecure, and 8.8.1 through 8.8.3 will be secure.

    I read back through #2804155: If the next minor version of core has a security release, status still says "Security update required!" even if the site is on an equivalent, secure release already
    The problem before that issue was that we had

    if (!empty($project_data['security updates'])) {
        // If we found security updates, that always trumps any other status.
        $project_data['status'] = UPDATE_NOT_SECURE;
      }
    

    basically if we had any security updates newer than the installed version we considered the project insecure. But in that issue we solved this by relying on "Release type" of "insecure". So we have no logic around extended security coverage. If a site was on 8.1.10 and the xml didn't have

    <term>
    <name>Release type</name>
    <value>Insecure</value>
    </term>
    

    on that release you still would not have see "Security Update Required"

    In \Drupal\Tests\update\Functional\UpdateCoreTest::securityUpdateAvailabilityProvider() we have cases for when the site is on a secure release for their minor and there is an newer security release for the next minor. In addition we have cases where there is newer security release for the existing minor. We have 17 cases in all.

    The only new thing we are testing here is the variation of when the release that is currently installed in not in a supported_branch.

    Of the 17 cases 6 involve a site being in a lower minor and have expected update in the next minor. I don't think we need to add a test case variation for each of these 6 cases where in additionally test the same exact situation except with the current version being in a branch that is not in supported_minors. I think the 1 case to confirm this enough. but we could add them or we could add them in follow up. In a follow up we could duplicate all 17 cases with a variations this would include case were the site is on the latest minor but it is an unsupported branch. This is very unlikely for Drupal core but I guess could happen in contrib. \Drupal\Tests\update\Functional\UpdateContribTest::securityUpdateAvailabilityProvider() currently only has 7 cases partly because it doesn't yet have minor branches.

tedbow’s picture

+++ b/core/modules/update/tests/src/Functional/UpdateCoreTest.php
@@ -62,6 +62,13 @@ protected function setSystemInfo($version) {
+   *
+   * The XML fixture file 'drupal.1.0.xml' which is one of the XML files this
+   * test uses also contains 2 extra releases that are newer than '8.1.0'.
+   * These releases will not show as available updates because of the following
+   * reasons:
+   * - '8.0.2' is an unpublished release.
+   * - '8.0.3' is marked as 'Release type' 'Unsupported'.
    */

I added this comment because we updated the XML fixture this test was using for the new test methods.

It doesn't change any assertions in the test because the 2 releases we added should never be listed as updates and this test is only confirming that there are no updates

dww’s picture

Status: Needs review » Needs work

Thanks for the updates and clarifications on test coverage!

A few nits with the new comments:

  1. +++ b/core/modules/update/tests/modules/update_test/aaa_update_test.1_0-unsupported.xml
    @@ -1,4 +1,9 @@
    +This XML file is the exact same as the file aaa_update_test.1_0-supported.xml except 'supported_branches' in this file
    +does not contain the '8.x-1.' branch. Therefore all the the releases that start with '8.x-1.' are in an unsupported
    +branch.
    

    Seems we should wrap these to 80 chars, no?

    Also: s/Therefore all/Therefore, all/

    s/the the/the/

  2. +++ b/core/modules/update/tests/modules/update_test/drupal.1.0-unsupported.xml
    @@ -1,4 +1,8 @@
    +<!--
    +This XML file is the exact same as the file drupal.1.xml except 'supported_branches' in this file does not contain the
    +'8.0.' branch. Therefore all the the releases that start with '8.0.' are in an unsupported branch.
    +-->
    

    Almost copy/paste of above comment, with the same problems.

  3. +++ b/core/modules/update/tests/modules/update_test/drupal.sec.1.2_insecure-unsupported.xml
    @@ -1,4 +1,8 @@
    +<!--
    +This XML file is the exact same as the file drupal.sec.1.2_insecure.xml except 'supported_branches' in this file does
    +not contain the '8.0.' branch. Therefore all the the releases that start with '8.0.' are in an unsupported branch.
    +-->
    

    And here.

  4. +++ b/core/modules/update/tests/src/Functional/UpdateContribTest.php
    @@ -707,7 +707,18 @@ public function securityUpdateAvailabilityProvider() {
    +   * The both have an '8.x-1.0' release that is unpublished and an '8.x-2.0'
    

    s/The both/They both/

  5. +++ b/core/modules/update/tests/src/Functional/UpdateContribTest.php
    @@ -736,7 +745,19 @@ public function testRevokedRelease() {
    +   * The both have an '8.x-1.1' release that has the 'Release type' value of
    

    s/The both/They both/

  6. +++ b/core/modules/update/tests/src/Functional/UpdateCoreTest.php
    @@ -62,6 +62,13 @@ protected function setSystemInfo($version) {
    +   * test uses also contains 2 extra releases that are newer than '8.1.0'.
    +   * These releases will not show as available updates because of the following
    +   * reasons:
    +   * - '8.0.2' is an unpublished release.
    +   * - '8.0.3' is marked as 'Release type' 'Unsupported'.
    

    Something's not right.
    8.0.2 is not "newer than '8.1.0'"...

  7. +++ b/core/modules/update/tests/src/Functional/UpdateCoreTest.php
    @@ -802,10 +814,19 @@ public function testLocalActions() {
    +   * The both have an '8.0.2' release that is unpublished and an '8.1.0' release
    

    s/The both/They both/

    In this case, that'll cause "release" to exceed 80 chars, so we'll need to wrap that differently, too.

  8. +++ b/core/modules/update/tests/src/Functional/UpdateCoreTest.php
    @@ -816,10 +837,20 @@ public function testRevokedRelease() {
    +   * The both have an '8.0.3' release that that has the 'Release type' value of
    

    "The both"

dww’s picture

Assigned: tedbow » dww

@tim.plunkett pinged me in Slack asking me to re-roll this today since Ted's traveling. Should be easy fixes. Stay tuned.

dww’s picture

This fixes everything from #38.

However, interdiff is still being annoying, and part of the problem is that in spite of the new comments, drupal.1.0.xml and drupal.1.0-unsupported.xml also differ because of legacy CVS values in the <tag> entries for the drupal.1.0.xml. So I'll upload another patch that cleans that up, too.

dww’s picture

I'd rather all this went away via #3113798: Remove unused (and generally wrong) <tag> markup from Update module test XML fixtures

Until then, we should at least keep these consistent between the two "identical" fixtures, and closer to the actual updates.d.o release history feeds...

dww’s picture

Whoops, meant to attach these, too. Interdiffs for #41 relative to both #40 and #35...

xjm’s picture

+++ b/core/modules/update/tests/modules/update_test/drupal.1.0.xml
@@ -13,7 +13,7 @@
-   <tag>DRUPAL-8-1-0</tag>
+   <tag>8.1.0</tag>
    <status>published</status>
    <release_link>http://example.com/drupal-8-1-0-release</release_link>
    <download_link>http://example.com/drupal-8-1-0.tar.gz</download_link>
@@ -26,7 +26,7 @@

@@ -26,7 +26,7 @@
  <release>
    <name>Drupal 8.1.0-beta1</name>
    <version>8.1.0-beta1</version>
-   <tag>DRUPAL-8-1-0-beta1</tag>
+   <tag>8.1.0-beta1</tag>
    <status>published</status>
    <release_link>http://example.com/drupal-8-1-0-beta1-release</release_link>
    <download_link>http://example.com/drupal-8-1-0-beta1.tar.gz</download_link>
@@ -39,7 +39,7 @@

@@ -39,7 +39,7 @@
  <release>
    <name>Drupal 8.1.0-alpha1</name>
    <version>8.1.0-alpha1</version>
-   <tag>DRUPAL-8-0-1-alpha1</tag>
+   <tag>8.1.0-alpha1</tag>

I think these changes are out of scope for this issue.

dww’s picture

Re: #43:

Okay, then here's patch #40 again, without the do-not-test suffix. ;)

Status: Needs review » Needs work

The last submitted patch, 44: 3113292-44.patch, failed testing. View results

dww’s picture

Status: Needs work » Needs review

Random fail:

There were 2 failures:

1) Drupal\KernelTests\Core\Entity\ContentEntityChangedTest::testChanged
Changed time of the German translation is newer then the original language.
Failed asserting that false is true.

2) Drupal\KernelTests\Core\Entity\ContentEntityChangedTest::testRevisionChanged
Changed time of original language has been updated by new revision.
Failed asserting that false is true.

Requeued...

Status: Needs review » Needs work

The last submitted patch, 44: 3113292-44.patch, failed testing. View results

tim.plunkett’s picture

Status: Needs work » Reviewed & tested by the community

#3112295: Replace REQUEST_TIME in rest of OO code (except for tests) was just reverted in between those test runs. That caused the fail and isn't in the way anymore.

I agree with #43, those keep popping in and out of the patch over the course of the issue, glad they're gone again :)

This looks great, thanks to @tedbow, @dww, and @xjm for their extremely thorough reviews and explanations since #24.

dww’s picture

FYI: #3115435: Make clear why each XML update.module fixture is created the way it is now exists for #26.3 / #29 etc.

Thanks for opening that, @tedbow!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed 831d6bccd9 to 9.0.x and 8015e8e3fc to 8.9.x. Thanks!

  • alexpott committed 831d6bc on 9.0.x
    Issue #3113292 by dww, tedbow, xjm, tim.plunkett: Update module has no...

  • alexpott committed 8015e8e on 8.9.x
    Issue #3113292 by dww, tedbow, xjm, tim.plunkett: Update module has no...
dww’s picture

Version: 9.0.x-dev » 8.8.x-dev
Status: Fixed » Patch (to be ported)

Yay, thanks!

Is this eligible for a backport to 8.8.x? It's adding test coverage of something we almost broke in another critical bug fix that I believe we're going to backport. Patch doesn't cleanly apply, but I could re-roll if appropriate. Please advise.

Thanks,
-Derek

alexpott’s picture

Backporting tests to 8.8.x seems valuable.

dww’s picture

Status: Patch (to be ported) » Needs review
FileSize
30.46 KB
790 bytes

Re-rolled. Let's see what the bot says about this.

Status: Needs review » Needs work

The last submitted patch, 55: 3113292-55-88x.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

tedbow’s picture

Title: Update module has no tests for changes to status of the installed release (revoked, etc) » [PP-1] Update module has no tests for changes to status of the installed release (revoked, etc)
Status: Needs work » Postponed
Related issues: +#3074993: Use "current" in update URLs instead of CORE_COMPATIBILITY to retrieve all future updates

This at least needs #3074993: Use "current" in update URLs instead of CORE_COMPATIBILITY to retrieve all future updates because the new fixtures don't have version_major etc.

dww’s picture

Re: #57: Oh, right. Bummer. I was hoping this could escape issue spaghetti, but I guess not. ;) No sense complicating the fixtures here for the backport, only to have to undo that when #3074993: Use "current" in update URLs instead of CORE_COMPATIBILITY to retrieve all future updates lands. Agreed on postponed.

Meanwhile, this fixes the extra newline the bot found.

tedbow’s picture

Status: Postponed » Patch (to be ported)

This should be backported

tedbow’s picture

Status: Patch (to be ported) » Postponed

This needs #3074993: Use "current" in update URLs instead of CORE_COMPATIBILITY to retrieve all future updates to be backported to 8.8.x also.

I have 8.8.x running tests now on that issue

tedbow’s picture

Status: Postponed » Needs review
FileSize
29.46 KB

new commits on 8.8.x here is reroll.

tedbow’s picture

Title: [PP-1] Update module has no tests for changes to status of the installed release (revoked, etc) » Update module has no tests for changes to status of the installed release (revoked, etc)

Status: Needs review » Needs work

The last submitted patch, 61: 3113292-58-8.8.x-reroll.patch, failed testing. View results

dww’s picture

Status: Needs work » Needs review

Random fail. Re-queued #61. Will review shortly.

tedbow’s picture

Assigned: Unassigned » tedbow
Status: Needs review » Needs work

Assigning to myself to reroll

tedbow’s picture

Assigned: tedbow » Unassigned
Status: Needs work » Needs review
FileSize
30.15 KB

Another reroll

dww’s picture

Status: Needs review » Reviewed & tested by the community

Confirmed the re-roll is clean and accurate. Only changes relative to #58 are the order of a few functions in a few files. But #66 has the order matching what we've got in the 8.9.x branch, so that's better than #58. RTBC.

Thanks!
-Derek

  • Gábor Hojtsy committed d3d2a5c on 8.8.x
    Issue #3113292 by dww, tedbow, xjm, tim.plunkett: Update module has no...
Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Thanks!

Status: Fixed » Closed (fixed)

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