Add comprehensive test coverage for update status for security releases

@tedbow I am testing the patch.

Can you please elaborate this second point

No security release available for site minor release 0 but release not marked as insecure. Security release available for next minor.

@tedbow Thanks for explaination.

+++ b/core/drupalci.yml
@@ -3,53 +3,13 @@
 build:
   assessment:
-    validate_codebase:
-      phplint:
-      # Re-run composer install to ensure the dependencies resolve for the
-      # containerized PHP version.
-      container_composer:
-        options: ' install --prefer-dist --no-suggest --no-progress --no-interaction'
-        halt-on-fail: true
-      csslint:
-        halt-on-fail: false
-      eslint:
-        # A test must pass eslinting standards check in order to continue processing.
-        halt-on-fail: false
-      phpcs:
-        # phpcs will use core's specified version of Coder.
-        sniff-all-files: false
-        halt-on-fail: false
     testing:
       # run_tests task is executed several times in order of performance speeds.
       # halt-on-fail can be set on the run_tests tasks in order to fail fast.
       # suppress-deprecations is false in order to be alerted to usages of
       # deprecated code.
-      run_tests.phpunit:
-        types: 'PHPUnit-Unit'
-        testgroups: '--all'
-        suppress-deprecations: false
-        halt-on-fail: false
-      run_tests.kernel:
-        types: 'PHPUnit-Kernel'
-        testgroups: '--all'
-        suppress-deprecations: false
-        halt-on-fail: false
-      run_tests.simpletest:
-         types: 'Simpletest'
-         testgroups: '--all'
-         suppress-deprecations: false
-         halt-on-fail: false
       run_tests.functional:
         types: 'PHPUnit-Functional'
-        testgroups: '--all'
-        suppress-deprecations: false
-        halt-on-fail: false
-      run_tests.javascript:
-        concurrency: 15
-        types: 'PHPUnit-FunctionalJavascript'
-        testgroups: '--all'
+        testgroups: '--module update'
         suppress-deprecations: false
         halt-on-fail: false
-      # Run nightwatch testing.
-      # @see https://www.drupal.org/project/drupal/issues/2869825
-      nightwatchjs:

I don't think these changes should be here?

Nice work so far!

I don't see test coverage for going from alpha -> beta, or beta -> beta, or alpha -> rc. Also don't see a test for when there's a security update for a previous branch, i.e. if you're on 8.x-2.0 and there's a security update in 8.x-1.2, there should be no expected security release.

Also don't see a test for when there's a security update for a previous branch, i.e. if you're on 8.x-2.0 and there's a security update in 8.x-1.2, there should be no expected security release.

So this is an interesting case now that the "insecure" metadata has been populated. 8.x-2.0 might be secure, or it might be insecure. If it's insecure and there's no secure version on 8.x-2.x, then what do we expect to happen? What is the current behavior in HEAD?

1. Re: #16, yeah, let's exclude failing cases from this issue and put them in their own issues. Not sure #2989616: The update report does not honor drupal.org's info about what releases are secure or not is the right place though unless we rescope it; that sounds like maybe a regression for contrib?

2. +++ b/core/modules/update/tests/src/Functional/UpdateContribTest.php
   @@ -438,4 +438,110 @@ public function testHookUpdateStatusAlter() {
   +      // Security release available for module major release 2.
   +      // Security release also available for previous minor.
   +      '8.x-2.0, 8.x-1.2 8.x-2.2' => [
   +        'module_patch_version' => '8.x-2.0',
   +        'security_releases' => ['8.x-1.2', '8.x-2.2'],
   +        'expected_security_release' => '8.x-2.2',
   +        'fixture' => '8.x-1.2_8.x-2.2-sec',
   +      ],
   

   The second inline comment here looks incorrect; should it be "Security release also available for the previous major"?

3. +++ b/core/modules/update/tests/src/Functional/UpdateContribTest.php
   @@ -438,4 +438,110 @@ public function testHookUpdateStatusAlter() {
   +      // No security release available for module major release 1 but release
   +      // not marked as insecure.
   +      // Security release available for next major.
   

   That's a lot of negatives. :)

   Does this imply that in the prior cases, all security releases besides the latest were marked as insecure? (The default behavior, but only since a couple months ago.)

   Maybe it'd be helpful to just list a quick summary the releases in the fixture in the docs, e.g.:

   // Fixture 8.2_1.2_1.1.0.11 contains:
   // - 8.x-1.0, insecure,
   // - 8.x-1.1, security release, insecure 
   // - 8.x-1.2, security release
   // - 8.x-2.0, insecure
   // - 8.x-2.1, security release
   

1. +++ b/core/modules/update/tests/src/Functional/UpdateCoreTest.php
   @@ -160,20 +160,234 @@ public function testMajorUpdateAvailable() {
   +      // Site on 2.0-beta1 which is insecure.2.0-beta2 is not insecure but
   +      // because beta/alpha/RC releases are not supported the latest security
   +      // from the previous major is the expected security release.
   +      '2.0-beta1, 0.2 1.2 2.0-beta2' => [
   +        'site_patch_version' => '2.0-beta1',
   +        'expected_security_release' => '1.2',
   +        'update_available' => TRUE,
   +        'fixture' => 'sec.0.2-rc2',
   +      ],
   

   This doesn't make sense to me - we should never suggest that users downgrade. Is this just how the current behavior is? I have the same comment for the other new tests that do this.

This looks like a good amount of tests to start from when working on test logic changes. Marking as RTBC assuming the full test run passes.

Thanks @tedbow and @samuel.mortenson! I'll finish reviewing this tomorrow. Meanwhile:

+++ b/core/modules/update/tests/src/Functional/UpdateContribTest.php
@@ -487,6 +487,30 @@ public function testSecurityUpdateAvailability($module_version, $security_releas
+   *     8.x-3.0-beta1 1nsecure

1nsecure would be an excellent name for a boyband.

+++ b/core/modules/update/tests/src/Functional/UpdateContribTest.php
@@ -438,4 +438,144 @@ public function testHookUpdateStatusAlter() {
+   *   Contains releases
+   *     8.x-1.2 Security Update
+   *     8.x-1.1, 8.x-1.2 insecure

Last line also looks to contain a typo here.

Back to RTBC assuming tests pass.

This still looks good to me - hopefully my credibility isn't getting dinged! :-)

Really great work on this. Thanks @tedbow for your careful attention to detail here. I am really glad we split this issue out.

I finally read through this carefully from the top. I want to read over the providers one more time but here are my notes from the rest (don't want to lose the review).

1. Regarding:
   

   This links are set up incorrectly and aren't in the format that test would look for them.

   Why didn't this cause a fail before? (Or did it -- is that the source of the fail in #20?)

2. +++ b/core/modules/update/tests/modules/update_test/aaa_update_test.sec.8.x-1.2_8.x-2.2.xml
   @@ -0,0 +1,169 @@
   + <recommended_major>1</recommended_major>
   + <supported_majors>1</supported_majors>
   + <default_major>1</default_major>
   

   Hmm. Is this correct? This fixture has stable releases for 8.x-2.x too. Wouldn't this include both major versions? Why is the test passing if so? Maybe this doesn't actually affect the part of the behavior that we're testing? Need to double-check against the provider.

3. +++ b/core/modules/update/tests/modules/update_test/aaa_update_test.sec.8.x-2.2_1.x_secure.xml
   @@ -0,0 +1,123 @@
   + <recommended_major>1</recommended_major>
   + <supported_majors>1</supported_majors>
   + <default_major>1</default_major>
   

   Same question here.

4. +++ b/core/modules/update/tests/modules/update_test/drupal.sec.0.2-rc2-b.xml
   @@ -0,0 +1,263 @@
   +  <name>drupal 8.2.0-rc1</name>
   +  <version>8.2.0-rc1</version>
   +  <tag>8.2.0-rc1</tag>
   ...
   +  <security>
   +   Beta releases are not covered by Drupal security advisories.
   +  </security>
   

   Is this <security> message a c&p error? This is actually an RC if I'm reading the XML right.

   This probably has no impact on the test results and I'm not even sure if/how it's used in core, but could cause confusion with the fixture in the future. Maybe we should check what d.o actually outputs and verify this is still there as such.

   Aside: I have trouble typing "XML" because it's too close to my username. :P

5. +++ b/core/modules/update/tests/modules/update_test/drupal.sec.0.2-rc2-b.xml
   @@ -0,0 +1,263 @@
   +  <name>drupal 8.2.0-alpha2</name>
   +  <version>8.2.0-alpha2</version>
   +  <tag>8.2.0-alpha2</tag>
   ...
   +  <security>
   +   Beta releases are not covered by Drupal security advisories.
   +  </security>
   

   Same thing, except this one is an alpha.

6. +++ b/core/modules/update/tests/src/Functional/UpdateContribTest.php
   @@ -438,4 +438,149 @@ public function testHookUpdateStatusAlter() {
   +   * @param string $expected_security_release
   +   *   The expected security release. If NULL then no security release is
   +   *   expected.
   
   +++ b/core/modules/update/tests/src/Functional/UpdateCoreTest.php
   @@ -160,20 +160,208 @@ public function testMajorUpdateAvailable() {
   +   * @param string $expected_security_release
   +   *   The expected security release.
   

   I still think "expected security release" is unclear. Should this maybe say something like:

   The security release, if any, that the status report should recommend.

   Is that accurate?

   Also, this implies that only one security release is "expected". That's the default behavior with the changes to d.o's metadata and will likely be the preferred behavior based on UX review on #2865920: Add tests for when a single supported branch has 2 security updates that are both not insecure, but previously, multiple security releases were "expected". Are we sure we want to hardcode that there is only one? I think it's still technically possible for d.o to output multiple ones under certain circumstances. Should it be a string[] instead, single-value everywhere for now, and add a reference/@todo for #2865920: Add tests for when a single supported branch has 2 security updates that are both not insecure? That way we can keep that out of the scope here.

   The other option is just calling it something like $expected_latest_security_release.

7. Also, the copy should be the same for both of these, right? Which makes me wonder if we should have a trait or something for the code semi-duplication to avoid unintended divergence/errors/etc.

   OTOH, since there are only two cases (core and contrib) the duplication might be NBD or something that a normal followup could address. What do you think?

8. +++ b/core/modules/update/tests/src/Functional/UpdateContribTest.php
   @@ -438,4 +438,149 @@ public function testHookUpdateStatusAlter() {
   +   *     8.x-1.2 Security Update
   +   *     8.x-1.1 Security Update, Insecure
   
   +++ b/core/modules/update/tests/src/Functional/UpdateCoreTest.php
   @@ -160,20 +160,208 @@ public function testMajorUpdateAvailable() {
   +   *     8.0.2 Security Update
   +   *     8.0.1 Security Update, Insecure
   ...
   +   *     8.0.2 Security Update
   ...
   +   *     8.2.0-rc2 Security Update
   ...
   +   *     8.1.2 Security Update
   ...
   +   *     8.0.2 Security Update
   ...
   +   *     8.1.2 Security Update
   ...
   +   *     8.1.2 Security Update
   

   Nit: I think the correct case is Security update. Either that or the other ones are cased incorrectly. :)

Those changes all look pretty good; thanks @tedbow.

1. +++ b/core/modules/update/tests/modules/update_test/drupal.sec.0.2-rc2-b.xml
   @@ -54,7 +54,7 @@
   -   Beta releases are not covered by Drupal security advisories.
   +   RC releases are not covered by Drupal security advisories.
   

   Can we confirm that this is what d.o actually outputs for RCs? I would expect "Release candidates" but I don't know what, if anything, is actually there in the XML.

2. +++ b/core/modules/update/tests/src/Functional/UpdateContribTest.php
   @@ -453,8 +452,7 @@ public function testHookUpdateStatusAlter() {
   -  public function testSecurityUpdateAvailability($module_version, $expected_security_release, $update_available, $fixture) {
   ...
   +  public function testSecurityUpdateAvailability($module_version, $expected_security_releases, $update_available, $fixture) {
   

   This one could have the array typehint too, right?

3. +++ b/core/modules/update/tests/src/Functional/UpdateTestBase.php
   @@ -81,4 +81,42 @@ protected function standardTests() {
   +      foreach ($expected_security_releases as $expected_security_release) {
   +        $assert_session->elementTextNotContains('css', $update_element_css_locator, 'Up to date');
   +        $assert_session->elementTextNotContains('css', $update_element_css_locator, 'Update available');
   +        $assert_session->elementTextContains('css', $update_element_css_locator, 'Security update required!');
   +        $expected_url_version = str_replace('.', '-', $expected_security_release);
   +        $assert_session->linkByHrefExists("http://example.com/$project-$expected_url_version-release");
   +        $assert_session->linkByHrefExists("http://example.com/$project-$expected_url_version.tar.gz");
   +        $assert_session->responseContains('error.svg', 'Error icon was found.');
   +      }
   

   Now that we have an array, should we also be asserting that no security releases other than the expected security releases are shown?

+++ b/core/modules/update/tests/src/Functional/UpdateContribTest.php
@@ -438,4 +438,131 @@ public function testHookUpdateStatusAlter() {
+   * - aaa_update_test.sec.8.x-1.1_8.x-1.2.xml
+   *   Contains releases
+   *     8.x-1.2 Security update
+   *     8.x-1.1 Security update, Insecure
+   *     8.x-1.2 Insecure

For these to be formatted correctly as a nested list, the releases should have a - at the beginning too. I think we can get rid of the line "contains releases" and instead say something like:

These test cases rely on the following fixtures containing the following releases:

+++ b/core/modules/update/tests/src/Functional/UpdateContribTest.php
@@ -438,4 +438,131 @@ public function testHookUpdateStatusAlter() {
+   *     8.x-1.2 Security update
+   *     8.x-1.1 Security update, Insecure
+   *     8.x-1.2 Insecure

Looks like a typo as the same release is listed twice?

1. +++ b/core/modules/update/tests/src/Functional/UpdateContribTest.php
   @@ -438,4 +438,131 @@ public function testHookUpdateStatusAlter() {
   +   * - aaa_update_test.sec.8.x-1.1_8.x-1.2.xml
   ...
   +   * - aaa_update_test.sec.8.x-1.2_8.x-2.2.xml
   +   *   Contains releases
   ...
   +   * - aaa_update_test.sec.8.x-2.2_1.x_secure.xml
   ...
   +        'fixture' => 'sec.8.x-1.2',
   

   This fixture name doesn't appear in the docblock above?

2. +++ b/core/modules/update/tests/src/Functional/UpdateContribTest.php
   @@ -438,4 +438,131 @@ public function testHookUpdateStatusAlter() {
   +      // 2 security releases available for module major release 1.
   
   +++ b/core/modules/update/tests/src/Functional/UpdateCoreTest.php
   @@ -160,20 +160,185 @@ public function testMajorUpdateAvailable() {
   +      // 2 security releases available for site minor release 0.
   

   Nit: When "2" is a word in a sentence (as in the quantity of something) rather than a string, we should spell it out as "two".

3. +++ b/core/modules/update/tests/src/Functional/UpdateCoreTest.php
   @@ -160,20 +160,185 @@ public function testMajorUpdateAvailable() {
   +      // Security release available for site minor release 1.
   +      // Security release also available for previous minor.
   ...
   +      // Site on latest security release for minor. Previous minor has security
   +      // release.
   

   Nit: Should the second example look more like the first for easier scanning of the test cases?

4. +++ b/core/modules/update/tests/src/Functional/UpdateCoreTest.php
   @@ -160,20 +160,185 @@ public function testMajorUpdateAvailable() {
   +    $pre_releases = [
   +      '2.0-alpha1',
   +      '2.0-alpha2',
   +      '2.0-beta1',
   +      '2.0-beta2',
   +      '2.0-rc1',
   +    ];
   ...
   +    $pre_releases[] = '2.0-rc2';
   

   Adding an item to the array after the fact is a bit brittle. I'd rather just have a separate test case for 8.2.0-rc2 after the foreach. That would also allow combining the two foreach loops into one.

5. +++ b/core/modules/update/tests/src/Functional/UpdateCoreTest.php
   @@ -160,20 +160,185 @@ public function testMajorUpdateAvailable() {
   +    // Add test cases to confirm that all alpha/beta/RC releases will
   +    // have the expected security update of 8.2.0-rc2 which is the only
   +    // alpha/beta/RC release in sec.0.2-rc2 that is not insecure and is a
   +    // security release.
   

   Hm, quite the run-on. How about:

   If:
   - 8.2.0-rc2 is a security release.
   - All other pre-release milestones are marked insecure.
   Then any 8.2.x release prior to 8.2.0-rc2 should show 8.2.0-rc2 as a security update.

6. +++ b/core/modules/update/tests/src/Functional/UpdateCoreTest.php
   @@ -160,20 +160,185 @@ public function testMajorUpdateAvailable() {
   +    // Add test cases to confirm that all alpha/beta/RC releases will have no
   ...
   +    foreach ($pre_releases as $pre_release) {
   

   I had to read this a couple times to understand it on account of the double negative. Maybe:

   If the site is on an alpha/beta/RC of an upcoming minor and none of the alpha/beta/RC versions are marked insecure, no security update should be required.

   This is interesting especially as a passing test case because it raises that release managers/sec team members need to remember to manually mark such pre-release milestones as insecure as I did when I rolled 8.6.0-beta2. By default, I think d.o only presents previous releases to be marked insecure on the security release publishing form. That's not relevant to this issue, but definitely something we need to keep in mind going forward.

Going to add some headers to Ted's review above just to make sure I'm not mixing stuff together.

• I think "Contrib fail case 1" makes sense as explained.
• "Contrib fail case 2" sounds like whatever's left over of #2865920: Add tests for when a single supported branch has 2 security updates that are both not insecure
• "Contrib fail case 3" is a harder call. Since there is no stable release version of 8.x-3.0 and since pre-release versions do not receive security coverage, it makes sense for the site to recommend a known secure version. So I believe that one is probably works as designed.
• "Core fail case 1" seems to have the same bug as Contrib fail case 2.
• "Core fail case 3" might arguably have the same issue as "Contrib fail case 3", where our pre-release milestones don't receive security coverage, and so it makes sense to show security releases that aren't marked insecure for older branches.

Back to RTBC

Can we attach a patch with the removed test cases to #2865920: Add tests for when a single supported branch has 2 security updates that are both not insecure? It won't apply yet but "Test with" can be set to "Do not test" on upload.

+++ b/core/modules/update/tests/src/Functional/UpdateContribTest.php
@@ -558,10 +550,14 @@ public function securityUpdateAvailabilityProvider() {
       // 8.x-3.0-beta2 is not insecure but is not a security release.
       '8.x-3.0-beta1, 8.x-1.2 8.x-2.2 8.x-3.0-beta2' => [
         'module_patch_version' => '8.x-3.0-beta1',
-        'expected_security_releases' => [],
+        'expected_security_releases' => ['8.x-1.2', '8.x-2.2'],
         'update_available' => TRUE,
         'fixture' => 'sec.8.x-1.2_8.x-2.2',

Hm, so even if it shows 2.2 it definitely should not be showing 1.2. So let's remove this test cases as well, insert the @todo, and include it in the patch for #2865920: Add tests for when a single supported branch has 2 security updates that are both not insecure.

Recent changes look good to me.

+++ b/core/modules/update/tests/src/Functional/UpdateCoreTest.php
@@ -160,20 +160,169 @@ public function testMajorUpdateAvailable() {
+        'fixture' => 'sec.0.2-rc2',
+
+      ],

Nit: extra blank line.

Saving issue credit.

I fixed the blank line on commit. Committed and pushed to 8.7.x, 8.6.x, and 8.5.x since we may need to backport code related to this for security coverage.

Thanks everyone for your patience on this issue! I feel a lot safer now about fixing the various bugs and changes needed with the update status report.

@xjm I updated the 'Version' to the earliest branch this commit is available in. This information relevant for projects that are built with patches from Drupal.org and that update to a new minor version (8.5.x -> 8.6.x), since it indicates the issue has been fixed in the project itself and the patch can be removed.