Problem/Motivation

in #3085717: [PP-1] Do not rely on version_* tags it was determined that release information version_patch and version_extra are not fully tested.

In that issue we want to remove reliance on this metadata from the update xml and instead get the information directly from version number instead.

We should first make sure we have test coverage for the current functionality. Currently these are taken into account for contrib test and may be under tested for core

Proposed resolution

Determine what test coverage we are missing and create it

Remaining tasks

Duplicate \Drupal\Tests\update\Functional\UpdateCoreTest::testNormalUpdateAvailable() which test version_patch and version_extra for core in \Drupal\Tests\update\Functional\UpdateContribTest for contrib

User interface changes

none

API changes

none

Data model changes

none

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tedbow created an issue. See original summary.

tedbow’s picture

Here is patch that removes the version_patch and version_extra information for releases.

This patch will cause just 1 test fail in \Drupal\Tests\update\Functional\UpdateCoreTest and none in \Drupal\Tests\update\Functional\UpdateContribTest

At the very least we need make sure this patch would cause fails in UpdateContribTest

We should also look at what other test coverage we are missing for the version_* metadata

tedbow’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 2: no-version_patch-version_extra.patch, failed testing. View results

dww’s picture

I don't really understand why we want to add new test coverage for functionality we're about to remove.

Seems like we shouldn't add new test coverage relying on items from the XML feed that we're about to deprecate and stop using entirely. New tests for about-to-be-deprecated stuff isn't going to help us make the new code work better. All these tests would do is slow us down, since we're going to have to modify them or remove them entirely once all the <version_X> tags are gone from the updates.d.o feeds we're consuming.

I'd rather see the effort go into adding test coverage around parsing the version string and using it properly.

IMHO, this issue sounds like a waste of time and energy, and will only make more work for us moving forward.

Strong vote for "won't fix"...

tedbow’s picture

Title: Create tests that cover version_patch and version_extra » Create tests that cover contrib non-full releases and contrib patch versions
Status: Needs work » Needs review
FileSize
14 KB

@dww sorry I titled/framed this issue incorrectly.

We don't need to test particularly test coverage for the particularly these elements version_extra and version_patch.
We need test coverage for contrib releases like 8.x-1.0-alpha1 and the differences between having multiple updates with different patch versions like 8.x-1.1 and 8.x-1.2.

Right now we are getting that information from version_extra and version_patch. In #3085717: [PP-1] Do not rely on version_* tags we get that information from the version string itself, which is good idea.

But #3085717 how will we know that we didn't break the current functionality around alpha, beta, rc releases? We should know that we didn't break the current functionality around those type of releases because our test coverage should break if we break current functionality. #3085717 should not change the functionality of which updates are displayed to the user just how the information is obtained from the XML.

I am uploading patch that will remove in occurrence of beta, alpha, rc or dev(only beta is actually used) from XMl files that supply the test releases for the contrib modules tested in \Drupal\Tests\update\Functional\UpdateContribTest. This removes beta from version_extra but also from the version strings. If we actually had current test coverage for this type of functionality this should break some test coverage in UpdateContribTest but my local testing show it will not.

So I still think we should be clear about current functionality by the way of comprehension test when we do an issue like #3085717 which in itself should not change end user functionality just how it is achieved. Otherwise there is much more of risk that we will break current functionality.

dww’s picture

@tedbow re: #6: Okay, thanks. That makes more sense. Carry on. ;)

Cheers,
-Derek

Gábor Hojtsy’s picture

tedbow’s picture

@Gábor Hojtsy Yep I expected #6 to pass because we currently don't have test coverage for contrib modules to the extent we do core. For instance we don't have anything like \Drupal\Tests\update\Functional\UpdateCoreTest::testNormalUpdateAvailable() in \Drupal\Tests\update\Functional\UpdateContribTest() so we don't have the basics test currently for contrib.

tedbow’s picture

Here a few patches

  1. 3094304-10.patch is the patch that needs review. It introduces a new test \Drupal\Tests\update\Functional\UpdateContribTest::testNormalUpdateAvailable()
    This test will fail if version_patch or version_extra is removed from the XML it uses.

    This test pretty much copies \Drupal\Tests\update\Functional\UpdateCoreTest::testNormalUpdateAvailable() which is the core version

    #6 proves that currently you can remove version_patch and version_extra in the test XML and no current tests fail. so we have no test coverage.

  2. 3094304-10-no_version_extra-FAIL.patch is the same as above but without version_extra. The new test method should fail.
  3. 3094304-10-no_version_patc-FAIL.patch is the same as above but without version_patch. The new test method should fail.

This proves that new test need both of these to pass just like the core version

The last submitted patch, 10: 3094304-10-no_version_patch-FAIL.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 10: 3094304-10-no_version_extra-FAIL.patch, failed testing. View results

tedbow’s picture

Issue summary: View changes
Status: Needs work » Needs review

The fails in 11 and 12 were expected

Updating the summary

bnjmnm’s picture

Status: Needs review » Needs work

Two small things

  1. +++ b/core/modules/update/tests/src/Functional/UpdateContribTest.php
    @@ -226,6 +226,87 @@ public function testUpdateBaseThemeSecurityUpdate() {
    +    $this->assertResponse(403, 'Accessing admin/reports/updates/check without a CSRF token results in access denied.');
    

    assertResponse() does not accept a second argument, and is deprecated in Drupal 10 in favor of $this->assertSession()->statusCodeEquals() (which also does not accept a second argument).

  2. Should this include a test scenario that results in an "Also available:" appearing, since that is also behavior that responds to version_extra?
tedbow’s picture

Status: Needs work » Needs review
FileSize
757 bytes
30.28 KB
2.9 KB

@bnjmnm thanks for the review!

  1. fixed.
  2. Good point but it made me check if "Also available is tested add all for core or contrib updates. I don't think so. I am also including a patch to check this.

    Not if I should add that test coverage to this issue. Maybe because lines near this will be affected in #3074993: Use "current" in update URLs instead of CORE_COMPATIBILITY to retrieve all future updates.

    but lets see if this patch passes or if it is tested elsewhere

tedbow’s picture

  1. Re #14.2.

    so the remove_also_releases.patch passed which "Also available" is not test at all for contrib or core ☹️

    "Also available" doesn't actually deal with version_extra it shows the latest update on the next major version

    \Drupal\Tests\update\Functional\UpdateCoreTest::testMajorUpdateAvailable() tests for the next major update for core but in test XML only Drupal 9 is supported so it is not under "also available"

  2. Added a test case for having a 8.x-2.0 being available as the next major version while the current major version is still supported. This puts it under "Also available"
  3. +++ b/core/modules/update/tests/src/Functional/UpdateContribTest.php
    @@ -227,6 +227,87 @@ public function testUpdateBaseThemeSecurityUpdate() {
    +        $modules_table_locator = 'table.update:nth-of-type(2)';
    +        $assert_session->elementContains('css', $modules_table_locator, Link::fromTextAndUrl("8.x-1.$patch$extra_version", Url::fromUri("http://example.com/$url_version-release"))->toString());
    +        $assert_session->elementContains('css', $modules_table_locator, Link::fromTextAndUrl('Download', Url::fromUri("http://example.com/aaa_update_test-8.x-1.$patch$extra_version.tar.gz"))->toString());
    +        $assert_session->elementContains('css', $modules_table_locator, Link::fromTextAndUrl('Release notes', Url::fromUri("http://example.com/$url_version-release"))->toString());
    ...
    +              $assert_session->elementTextContains('css', $modules_table_locator, 'Recommended version:');
    +              $assert_session->elementTextContains('css', $modules_table_locator, 'Latest version:');
    

    So the current test, and \Drupal\Tests\update\Functional\UpdateCoreTest::testNormalUpdateAvailable() which this was copied from has this problem where it tests for the link to a version and then checks there are actually to versions with update links.

    One is "Recommended" and one is "Latest" but it doesn't actually test that link is the right one and does test the other at all.

    Fixing that. Which also means for the new 8.x-2.0 test case mentioned above this tests for both the "Recommended" and "Also available" links.

  4. Not addressing in this patch but \Drupal\Tests\update\Functional\UpdateCoreTest::testNormalUpdateAvailable() needs to a new test for a new major being listed in "Also available" it needs to check the links it is currently testing for actually are under the correct heading, Recommend and Latest
tedbow’s picture

  1. +++ b/core/modules/update/tests/src/Functional/UpdateContribTest.php
    @@ -571,4 +664,27 @@ public function securityUpdateAvailabilityProvider() {
    +  protected function assertVersionUpdateLinks($version, $modules_table_locator, $label) {
    

    I am fixing the core version of testNormalUpdateAvailable() so I am moving this method to \Drupal\Tests\update\Functional\UpdateTestBase

    It need a couple other changes so that class can provide the update table CSS selector and project name. But also for some reason the drupal.*.xml fixtures don't use the version number directly in the download links link the contrib test fixtures do. 🤷‍♂️

    So this interdiff just uses new assert on the core test method which adds the extra check that not only the label such as "Recommended" is in the text but the expected download is there.

  2. The core test also had this
          // Both stable and unstable releases are available.
          // An unstable release is the latest.
          else {
            $this->assertNoText(t('Up to date'));
            $this->assertText(t('Update available'));
            $this->assertText(t('Recommended version:'));
            $this->assertText(t('Latest version:'));
            $this->assertRaw('warning.svg', 'Warning icon was found.');
         }
    

    So was testing that "Recommended" and "Latest" update label where there but only actually check for 1 link and not which label that 1 link was under.

    fixed that.

  3. The only thing I think left to cover would be the that when new major core update is available and the installed major is still supported that the new major goes under "Also available".

    I think this could be done in a follow because it will make the patch much bigger and shouldn't block #3074993: Use "current" in update URLs instead of CORE_COMPATIBILITY to retrieve all future updates

bnjmnm’s picture

Status: Needs review » Needs work
Issue tags: +Needs followup

I agree that addressing #17.3 in a followup is fine. It should get addressed but does not have to happen here.

All I could find were pretty minor things. I'm going to dig around with Xdebug a little more to be sure of it, but addressing the items below + creating the followup may be all that's left to do.

  1. +++ b/core/modules/update/tests/src/Functional/UpdateCoreTest.php
    @@ -565,4 +572,13 @@ public function testLocalActions() {
    +    $download_version = str_replace('.', '-', $version);
    

    Would be good to add a comment explaining why the str_replace() is needed.

  2. +++ b/core/modules/update/tests/src/Functional/UpdateTestBase.php
    @@ -168,4 +182,29 @@ protected function assertSecurityUpdates($project_path_part, array $expected_sec
    +    $this->assertTrue($update_element->has('css', "a[href=\"http://example.com/{$this->updateProject}-$url_version-release\"]"));
    +
    +    $this->assertTrue($update_element->hasLink('Download'));
    +    $this->assertTrue($update_element->has('css', "a[href=\"http://example.com/{$this->updateProject}-$download_version.tar.gz\"]"));
    +
    +    $this->assertTrue($update_element->hasLink('Release notes'));
    +    $this->assertTrue($update_element->has('css', "a[href=\"http://example.com/{$this->updateProject}-$url_version-release\"]"));
    

    The same assertion is repeated at the beginning and end of the snippet here.

  3. +++ b/core/modules/update/tests/src/Functional/UpdateContribTest.php
    @@ -227,6 +237,99 @@ public function testUpdateBaseThemeSecurityUpdate() {
    +            // When next major release is available and the current major is
    +            // still supported the next major will be under "Also available".
    +            // In this case it does not matter if the next major release is
    

    An 'and' and a few commas will make this a little easier to read

    // When the next major release is available and the current major is
    // still supported, the next major will be under "Also available".
    
    // In this case, it does not matter if the next major release is
    // unstable or stable.
    
tedbow’s picture

Status: Needs work » Needs review
Issue tags: -Needs followup
FileSize
3.81 KB
59.81 KB

re #17.3 here is the follow-up #3099825: Test available updates when the current major and next major of core are supported

re #18

  1. fixed
  2. Good catch. There are actually 2 links with different text. Fixing the assertion for all these links to make the text and href for the same link.
  3. fixed
bnjmnm’s picture

+++ b/core/modules/update/tests/src/Functional/UpdateCoreTest.php
@@ -576,7 +576,9 @@ public function testLocalActions() {
+  protected function assertVersionUpdateLinks($label, $version, $download_version = NULL) {
+    // Test XML files for Drupal core use '-' in the version number for the
+    // download link.
     $download_version = str_replace('.', '-', $version);
     parent::assertVersionUpdateLinks($label, $version, $download_version);

I only noticed this because the default value of $download_version was changed -- but realized it would be a problem regardless of the default value.

str_replace will return a string even if $subject is NULL, which means the null coalesce in the parent function will never find a null

    $download_version = $download_version ?? $version;

. (obviously not a problem for the current tests, but could be for future ones)

bnjmnm’s picture

Status: Needs review » Reviewed & tested by the community

#20 was a misdiagnosis on my part, there's nothing wrong there at all. This is RTBC.

  • Gábor Hojtsy committed ae75f06 on 9.0.x
    Issue #3094304 by tedbow, bnjmnm, dww: Create tests that cover contrib...

  • Gábor Hojtsy committed 3c114e3 on 8.9.x
    Issue #3094304 by tedbow, bnjmnm, dww: Create tests that cover contrib...
Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

It is lovely to see more test coverage for what we already do. Seeing this being a UI test was a bit surprising but since we want to ensure we provide the same feedback to the user, I am fine with that. Committed! Thanks all!

Gábor Hojtsy’s picture

Adjusting credits (again).

tedbow’s picture

@Gábor Hojtsy thanks for committing!

Status: Fixed » Closed (fixed)

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

xjm’s picture

Version: 8.9.x-dev » 8.8.x-dev

Backported to 8.8.x also.