Create contrib update module test cases that use semantic versioning

Approach here from #3 sounds great. No time to do a careful review right now, but I hope to look soon. This would be fabulous to land ASAP!

Thanks,
-Derek

Starting a review now...

A lot of this looks good. But it's a huge patch to try to review. I mostly skipped over all the new fixture files.

Not all of these points require work, but I'm setting to NW for at least some of them. ;)

1. --- /dev/null
   +++ b/core/modules/update/tests/modules/semantic_test/semantic_test.info.yml
   
   +++ b/core/modules/update/tests/modules/semantic_test/semantic_test.info.yml
   @@ -0,0 +1,6 @@
   
   @@ -0,0 +1,6 @@
   +name: 'Semantic Test'
   

   Can we call this semver_test* instead of semantic_test*?

   Name could be "Semantic Version Test" if desired.

   "Semantic" is true, but not specific enough. We're not testing the semantics of updates. We're testing semver / semantic version.

   Hopefully easy to automate this change if it's agreeable.

2. +++ b/core/modules/update/tests/modules/update_test/semantic_test.0.0-alpha1.xml
   @@ -0,0 +1,41 @@
   +    <tag>SEMANTIC_TEST-8-2-0</tag>
   

   Really wish we could agree on #3113798: Remove unused (and generally wrong) <tag> markup from Update module test XML fixtures before we add a bazillion more of these bogus tags. :(

3. +++ b/core/modules/update/tests/modules/update_test/semantic_test.0.0-alpha1.xml
   @@ -0,0 +1,41 @@
   +     <term><name>Release type</name><value>New features</value></term>
   
   --- /dev/null
   +++ b/core/modules/update/tests/modules/update_test/semantic_test.0.0-beta1.xml
   

   Eyes are glazing over trying to carefully review all these fixtures. I see the script that's auto-generating them. I think if I actually look at all this and say anything, it's going to be punted to "out of scope", so I don't really want to spend much energy on it.

   That said, the naming conventions for these files are weird. What does "0.0-beta1" mean, given there's an 8.2.0 release in here?

   Before we bulk copy/paste all of this, I'd love to do a few cleanups if possible:

   #3115435: Make clear why each XML update.module fixture is created the way it is
   #3121020: Move Update Manager XML test fixtures into a fixtures/release-history directory

   Probably not going to happen, but I can at least wish outloud. ;)

4. +++ b/core/modules/update/tests/src/Functional/UpdateCoreTest.php
   --- /dev/null
   +++ b/core/modules/update/tests/src/Functional/UpdateSemanticContribTest.php
   

   s/UpdateSemanticContribTest/UpdateSemverContribTest/?

5. +++ b/core/modules/update/tests/src/Functional/UpdateSemanticContribTest.php
   @@ -0,0 +1,91 @@
   +    // Ensure Drupal core on the same version for all test runs.
   +    if ($this->updateProject !== 'drupal') {
   +      $system_info['drupal'] = [
   +        'project' => 'drupal',
   +        'version' => '8.0.0',
   +        'hidden' => FALSE,
   +      ];
   +    }
   

   This is slightly weird to do in a shared helper method, since it only applies to the contrib side of life. But having different implementations of this in each child class is maybe not worth the effort. Kind of a DRY vs. Smell trade-off. ;)
   /shrug

6. +++ b/core/modules/update/tests/src/Functional/UpdateSemanticContribTest.php
   @@ -0,0 +1,91 @@
   +    $install_versions = [
   

   s/install_versions/installed_versions/?

7. +++ b/core/modules/update/tests/src/Functional/UpdateSemanticContribTest.php
   @@ -0,0 +1,91 @@
   +      '8.x-7.1',
   

   Since it's a potentially error-prone edge case, would love to see some '8.x-8.*' test cases in here, too.

8. +++ b/core/modules/update/tests/src/Functional/UpdateSemanticContribTest.php
   @@ -0,0 +1,91 @@
   +    $this->refreshUpdateStatus([$this->updateProject => '1.0']);
   

   Don't we also need to specify the fixture we're using for core here? I found it later, ignore this.

9. +++ b/core/modules/update/tests/src/Functional/UpdateSemanticContribTest.php
   @@ -0,0 +1,91 @@
   +      $this->drupalGet('admin/reports/updates');
   +      $this->clickLink(t('Check manually'));
   +      $this->checkForMetaRefresh();
   ...
   +        $this->assertVersionUpdateLinks('Recommended version:', '8.x-7.1');
   

   Isn't all this done by refreshUpdateStatus()? Why aren't we just doing the refreshUpdateStatus() inside the loop?

10. +++ b/core/modules/update/tests/src/Functional/UpdateSemanticContribTest.php
    @@ -0,0 +1,91 @@
    +      $this->assertUpdateTableTextNotContains(t('Security update required!'));
    

    Don't we want a case where an update to a semver release coincides with a required security update?

11. +++ b/core/modules/update/tests/src/Functional/UpdateSemanticContribTest.php
    @@ -0,0 +1,91 @@
    +      // All installed versions should indicate that there is update available
    +      // for the next major version of the module.
    

    s/there is update/there is an update/

    Then this will need to re-wrap.

12. +++ b/core/modules/update/tests/src/Functional/UpdateSemanticContribTest.php
    @@ -0,0 +1,91 @@
    +      $this->assertVersionUpdateLinks('Also available:', '8.1.0');
    

    This is the key assertion in this whole method. However, it's a bit confusing:

    a) I'd expect "8.0.0", not "8.1.0" as the expected release.

    b) This looks a lot like a core version string, so it's easy to miss that this is the main thing to be asserting.

13. +++ b/core/modules/update/tests/src/Functional/UpdateSemanticContribTest.php
    @@ -0,0 +1,91 @@
    +        // All installed versions besides 8.x-7.1 should recommend 8.x-7.1
    +        // because it is the latest full release for the major.
    +        $this->assertVersionUpdateLinks('Recommended version:', '8.x-7.1');
    

    Can we add a test case for where all the legacy versions are unsupported and the semver release is recommended?

14. +++ b/core/modules/update/tests/src/Functional/UpdateSemanticTestBase.php
    @@ -0,0 +1,445 @@
    +   * The project title.
    

    "The title of the project being tested" or something?

15. +++ b/core/modules/update/tests/src/Functional/UpdateSemanticTestBase.php
    @@ -0,0 +1,445 @@
    +    $admin_user = $this->drupalCreateUser(['administer site configuration', 'administer modules', 'administer themes']);
    

    Why do we need 'administer modules' and 'administer themes' for any of this?

16. +++ b/core/modules/update/tests/src/Functional/UpdateSemanticTestBase.php
    @@ -0,0 +1,445 @@
    +        $this->refreshUpdateStatus([$this->updateProject => "$minor_version.1" . $extra_version]);
    ...
    +        $this->drupalGet('admin/reports/updates');
    +        $this->clickLink(t('Check manually'));
    +        $this->checkForMetaRefresh();
    

    Again, I thought all this was the point of refreshUpdateStatus()...

17. +++ b/core/modules/update/tests/src/Functional/UpdateSemanticTestBase.php
    @@ -0,0 +1,445 @@
    +            // Both stable and unstable releases are available.
    +            // A stable release is the latest.
    

    These comments don't fully make sense. What does it mean that an unstable release is available if it's not the latest?

18. +++ b/core/modules/update/tests/src/Functional/UpdateSemanticTestBase.php
    @@ -0,0 +1,445 @@
    +            if ($extra_version == '') {
    

    === ?

19. +++ b/core/modules/update/tests/src/Functional/UpdateSemanticTestBase.php
    @@ -0,0 +1,445 @@
    +            // Only unstable releases are available.
    +            // An unstable release is the latest.
    

    The former implies the later. ;)

20. +++ b/core/modules/update/tests/src/Functional/UpdateSemanticTestBase.php
    @@ -0,0 +1,445 @@
    +            break;
    

    newline after break;

21. +++ b/core/modules/update/tests/src/Functional/UpdateSemanticTestBase.php
    @@ -0,0 +1,445 @@
    +            // Both stable and unstable releases are available.
    +            // A stable release is the latest.
    +            if ($extra_version == '') {
    +              $this->assertUpdateTableTextNotContains(t('Up to date'));
    +              $this->assertUpdateTableTextContains(t('Update available'));
    +              $this->assertVersionUpdateLinks('Recommended version:', $full_version);
    +              $this->assertUpdateTableTextNotContains(t('Latest version:'));
    +              $this->assertUpdateTableElementContains('warning.svg', 'Warning icon was found.');
    +            }
    

    Isn't this entirely duplicate with the case 0 version of this above? DRY?

22. +++ b/core/modules/update/tests/src/Functional/UpdateSemanticTestBase.php
    @@ -0,0 +1,445 @@
    +            break;
    

    Newline after break.

23. +++ b/core/modules/update/tests/src/Functional/UpdateSemanticTestBase.php
    @@ -0,0 +1,445 @@
    +          $this->drupalGet('admin/reports/updates');
    +          $this->clickLink(t('Check manually'));
    +          $this->checkForMetaRefresh();
    

    Again -- why do we need these after refreshUpdateStatus()?

24. +++ b/core/modules/update/tests/src/Functional/UpdateSemanticTestBase.php
    @@ -0,0 +1,445 @@
    +          $this->assertUpdateTableElementContains(Link::fromTextAndUrl('9.0.0', Url::fromUri("http://example.com/{$this->updateProject}-9-0-0-release"))->toString(), 'Link to release appears.');
    +          $this->assertUpdateTableElementContains(Link::fromTextAndUrl(t('Download'), Url::fromUri("http://example.com/{$this->updateProject}-9-0-0.tar.gz"))->toString(), 'Link to download appears.');
    +          $this->assertUpdateTableElementContains(Link::fromTextAndUrl(t('Release notes'), Url::fromUri("http://example.com/{$this->updateProject}-9-0-0-release"))->toString(), 'Link to release notes appears.');
    

    That's a lot of Url and Link instantiation inside a deeply nested loop. These never change across the test cases. Can we do this once outside any of the loops and re-use the results inside the assertions? Will also keep these from being 250 chars wide. ;)

25. +++ b/core/modules/update/tests/src/Functional/UpdateSemanticTestBase.php
    @@ -0,0 +1,445 @@
    +   * @param string $site_patch_version
    +   *   The patch version to set the site to for testing.
    

    "site" seems a bit off. Implies core, but this could be for anything. Maybe "installed_patch_version" or something?

26. +++ b/core/modules/update/tests/src/Functional/UpdateSemanticTestBase.php
    @@ -0,0 +1,445 @@
    +    $this->setProjectInstalledVersion("8.$site_patch_version");
    

    I'd expect a minor version. Oh, I see below, this isn't just "site_patch_version", it's really "installed_minor_patch_versions" or something. #confusing

27. +++ b/core/modules/update/tests/src/Functional/UpdateSemanticTestBase.php
    @@ -0,0 +1,445 @@
    +    $this->assertSecurityUpdates("{$this->updateProject}-8", $expected_security_releases, $expected_update_message_type, $this->updateTableLocator);
    

    Would love an inline comment about why we append "-8" to the updateProject name for this. /shrug

28. +++ b/core/modules/update/tests/src/Functional/UpdateSemanticTestBase.php
    @@ -0,0 +1,445 @@
    +   * These test cases rely on the following fixtures containing the following
    +   * releases:
    

    YAY! Update manager tests need so much more of this. Thank you, Thank You, THANK YOU!

29. +++ b/core/modules/update/tests/src/Functional/UpdateSemanticTestBase.php
    @@ -0,0 +1,445 @@
    +   * - drupal.sec.0.1_0.2.xml
    

    Aren't all of these filenames wrong for the contrib case? Instead of "drupal" for all these, don't we want (drupal|semver_test) or something to indicate that the fixture name changes depending on who's calling this? Or just "[project_name]..."

30. +++ b/core/modules/update/tests/src/Functional/UpdateSemanticTestBase.php
    @@ -0,0 +1,445 @@
    +   *   - 8.0.2
    +   *   - 8.0.1
    +   *   - 8.0.0
    

    Seems like an @todo that these are blank. Maybe something like "Normal" or "Regular" or something so we know we didn't just forget to document them?

31. +++ b/core/modules/update/tests/src/Functional/UpdateSemanticTestBase.php
    @@ -0,0 +1,445 @@
    +   *   different value for 'supported_branches' that does not contain '8.0.'.
    +   *   It is used to ensure that the "Security update required!" is displayed
    

    Technically "It" would fit on the prior line, although I actually think it's more readable this way...

32. +++ b/core/modules/update/tests/src/Functional/UpdateSemanticTestBase.php
    @@ -0,0 +1,445 @@
    +   *   - 8.2.0-rc2
    +   *   - 8.2.0-rc1
    +   *   - 8.2.0-beta2
    +   *   - 8.2.0-beta1
    +   *   - 8.2.0-alpha2
    +   *   - 8.2.0-alpha1
    

    Same for all these -- "Normal"?

33. +++ b/core/modules/update/tests/src/Functional/UpdateSemanticTestBase.php
    @@ -0,0 +1,445 @@
    +      '0.0, 0.2' => [
    

    I don't comprehend the naming convention for these array keys. I know it doesn't actually matter, but this is mostly incomprehensible noise at this point.

    If this is the best we can hope for, can we have a comment in the docblock for this function to explain what the keys are supposed to imply?

    Better yet, can we have more self-documenting keys that guide reviewers and future developers to understanding these?

34. +++ b/core/modules/update/tests/src/Functional/UpdateSemanticTestBase.php
    @@ -0,0 +1,445 @@
    +        'expected_update_message_type' => static::SECURITY_UPDATE_REQUIRED,
    

    (Out of scope) I wonder why UpdateTestBase defines its own versions of these constants instead of re-using the ones from Update manager itself. /shrug

35. +++ b/core/modules/update/tests/src/Functional/UpdateSemanticTestBase.php
    @@ -0,0 +1,445 @@
    +    //   - For 8.1.0 using fixture 'sec.0.2-rc2' to ensure that only security
    

    s/that only/that the only/

36. +++ b/core/modules/update/tests/src/Functional/UpdateSemanticTestBase.php
    @@ -0,0 +1,445 @@
    +    return $test_cases;
    

    I know we just decided dataProviders are okay in Functional tests at #3113992: The 'Update' page has no idea that some updates are incompatible, but this is *a lot* of cases, and this whole thing happens *twice* (once for core semver, once for contrib semver). I'd be curious to see how many minutes this adds to the test runs, and if we really want to keep going with such a massive dataProvider like this...

37. +++ b/core/modules/update/tests/src/Functional/UpdateSemanticTestBase.php
    @@ -0,0 +1,445 @@
    +   * - drupal.1.0.xml
    

    Again, "drupal" is only true 1/2 of the time.

38. +++ b/core/modules/update/tests/src/Functional/UpdateSemanticTestBase.php
    @@ -0,0 +1,445 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  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);
    +  }
    

    This is a slightly weird/unfortunate thing to have to override this method for. Can't we make the semver_test fixtures match the core one or vice-versa?

39. +++ b/core/modules/update/tests/src/Functional/UpdateSemanticTestBase.php
    @@ -0,0 +1,445 @@
    +    if (!isset($xml_map['drupal'])) {
    +      $xml_map['drupal'] = '0.0';
    +    }
    

    Ahah! This answers some previous review point. Phew.

40. +++ b/core/modules/update/tests/src/Functional/UpdateSemanticTestBase.php
    @@ -0,0 +1,445 @@
    +   * Sets the project installed version.
    

    "Sets the installed project version"

41. +++ b/core/modules/update/tests/src/Functional/UpdateSemanticTestBase.php
    @@ -0,0 +1,445 @@
    +   *   The version number.
    

    It's really a "version string", since there can be "extra" and all sorts of non-number stuff...

42. +++ b/core/modules/update/tests/src/Functional/UpdateTestBase.php
    @@ -274,6 +274,30 @@ protected function assertUpdateTableElementContains($text) {
    +  /**
    +   * Asserts that the update table element HTML contains the specified text.
    +   *
    +   * @param string $text
    +   *   The expected text.
    +   *
    +   * @see \Behat\Mink\WebAssert::elementNotContains()
    +   */
    +  protected function assertUpdateTableElementNotContains($text) {
    +    $this->assertSession()
    +      ->elementNotContains('css', $this->updateTableLocator, $text);
    +  }
    +
    +
    +  /**
    +   * Asserts that the update table text does not contain the specified text.
    +   *
    +   * @param string $text
    +   *   The expected text.
    +   */
    +  protected function assertUpdateTableTextNotContains($text) {
    +    $this->assertSession()->elementTextNotContains('css', $this->updateTableLocator, $text);
    +  }
    +
    

    My mind's capacity is starting to overflow with this review, apologies. But why is it in scope to be adding both of these to UpdateTestBase? Aren't we only using them from children of UpdateSemverTestBase?

Thanks!
-Derek

Re: #19:

1. Thanks, I think that'll be better long term.

2. I already wrote a perl 1-liner to rip out <tag> in #3113798: Remove unused (and generally wrong) <tag> markup from Update module test XML fixtures. That's not the point. It's just a lot of churn to duplicate all this bogus data and then have to remove it all later. I was stating my preference that we fix that first, since it'd be trivial and easy if only we agreed. But I know it's not going to happen.

Since this feature is already released, and we already saw passing test results, what's the hurry? ;) Why rush in 200+ kb of tests and fixtures if they're hard to understand and maintain? Why not spend the effort to make this worth committing, by the standards applied to every other core issue (even critical bugs that have been fixed for weeks but we shipped multiple versions of core with them while we argued about perfect test coverage)?

3. /shrug
4. 👍
5. 👍
6. 👍
7. 👍
8. 👍
9. 👍-- indeed, the assertVersionUpdateLinks() stuff was a dreditor mis-fire, sorry about that.
10. Sure, #NeedsFollowup is fine with me. This patch and issue are already huge.
11. 👍
12a. 👍
12b. Sure, more comments is fine. It's slightly too bad, but pragmatism++ for this. These tests are hard enough to read as-is.
13. 👍
14. 👍
15. See 2 above. ;) I don't know why we'd want to massively expand the footprint of not-so-clearly-written-nor-easy-to-make-sense-of tests in update module. If we're refactoring code into a shared base class and making a bunch of changes so we can test core and contrib with it, why leave known problems broken? But I can rarely make sense of what's in scope when or why, so I give up.

Re: #24:

1. No, a bunch of test methods in UpdateCoreTest need 'administer modules'. Search for "admin/modules" and you'll find many hits. I was talking about UpdateSemverTestBase not needing them. Might as well keep those perms for all the tests in UpdateCoreTest, since it'll be a lot of bloat to selectively add it to each test that needs it. /shrug I find it ironic that you set #3113992-93: The 'Update' page has no idea that some updates are incompatible to NW over this exact point, but in this issue you're effectively saying "it was like this already, we're only copying code, and we shouldn't change it or we might break something.". ;)

2. Hah. ;) Play git games so the patch doesn't include the weirdness so no one can point it out and it'd be out of scope to fix anything. I'll have to remember this strategy for future issues. 😉

3. -1 to putting more functions in UpdateSemverTestBase for the sole reason of keeping the patch here smaller. Again, I think we should be optimizing for the end result code is easy to read and maintain, not the smallest number of bytes we can manage for each patch.

4. /shrug

Re: #24.2 / #25.2
I suggested using -C35 to @tedbow.
No one is playing games here.

The default value is 50%, but lowering the threshold is a common tactic used throughout core development's history.
It's not intended to hide *anything*, but instead to highlight what is actually a change being made by the patch author, instead of obfuscating the intention by placing it among a lot of copy/paste.

Finding the right percentage threshold is conveying *more* information, not less.

Re: #27: For the record, I flagged this issue in this week's #d9readiness meeting (although the notes aren't yet posted), asking if it should be critical. That's what prompted @xjm to add comment #15. I agree it's critical, and that's why I'm spending (a lot of) my free time on it. Unlike the rest of you, jumping through these hoops isn't paying any of my bills, it's a labor of "love". I understand the desire to get this in quickly (as I wrote in #6). I just don't understand why some issues (like #3113992: The 'Update' page has no idea that some updates are incompatible) are stalled for weeks on end while we argue about perfect test coverage, while others (like this one) get the "it's cool if the tests are weird and hard to make sense of, something is better than nothing!" treatment. If we were consistent (in either direction), it wouldn't bother me (much), and I'd adapt to our standards. What's causing me to lose my cool is that we're not consistent, so I get the sense that no matter what I do, it's considered out of scope (or not enough, depending on the reviewer), unhelpful, wrong, etc.

Re: #26: So I guess it's now supposed to be on me to re-review the -C35 version and figure out how much of #17 still counts as a legitimate concern vs. off-topic comments on "obfuscated intentions"? Are we done fiddling with the threshold value? Do y'all believe that #24 represents the accurate intentions of the patch author? Is it worth spending time reviewing it yet, or do you want to keep searching for the magic value that properly represents what's reviewable?

Inventing a new issue tag "Adds technical debt" for issues like this where we know we're adding technical debt, but agree it's best due to other considerations. Tagging for "Needs followup" since a lot of #17 is being called out of scope and should be addressed elsewhere. Please define scoped issues you're all happy with so I can spend time fixing things instead of arguing about scope.

Thanks,
-Derek

This will need a (hopefully trivial) re-roll now that #3121020: Move Update Manager XML test fixtures into a fixtures/release-history directory landed.

Re-rolled patch from #35 for D9.1.x

PHP Fatal error:  Access level to Drupal\Tests\update\Functional\UpdateCoreTest::$modules must be public (as in class Drupal\Tests\update\Functional\UpdateSemverTestBase) in /var/www/html/core/modules/update/tests/src/Functional/UpdateCoreTest.php on line 13
3 coding standards messages
/var/lib/drupalci/workspace/jenkins-drupal_patches-45170/source/core/modules/update/tests/src/Functional/UpdateTestBase.php
line 288	Expected 1 blank line after function; 2 found
/var/lib/drupalci/workspace/jenkins-drupal_patches-45170/source/create-3100386-semver-xml.php
1	Missing file doc comment
1	The PHP open tag must be followed by exactly one blank line

A) The PHP fatal error needs help. Probably the base class should move $modules to be protected.
B) The patch shouldn't include create-3100386-semver-xml.php
C) The code standard violation for UpdateTestBase.php should be fixed.

Updated patch as mentioned in comment #39

Updated interdiff.txt for comment #40

Cleaning up pre-9.0.0 beta targets.

Test issues are patch-eligible.

Added reroll of patch #40 on D-9.1.x

@ravi.shankar: There is something wrong with your rerolled patch. The original has a size of 218kb and your patch is 168kb. It also needs to be rerolled against 9.2.x.

Removing credit for the problematic reroll. If you need assistance with how to reroll patches, you can reach out in the Drupal community Slack. Thanks!

Thanks @daffie
Here I have again added a reroll, please review.

Trying to fix testbot issues of patch #52

Patch is still failing the testbot.

Fixing failed tests.

Fixing failed tests of patch #55.

Just a couple of nitpicks:

1. +++ b/core/modules/update/tests/src/Functional/UpdateCoreTest.php
   @@ -12,7 +10,7 @@
   +class UpdateCoreTest extends UpdateSemverTestBase {
   

   A better class name would be UpdateSemverCoreTest as we are extending the class UpdateSemverTestBase.

2. +++ b/core/modules/update/tests/src/Functional/UpdateSemverTestBase.php
   @@ -0,0 +1,468 @@
   +          $this->assertUpdateTableTextContains(t('Up to date'));
   

   Could we not translate the text string or any other text string in this file. It happen multiple times in this class.

3. +++ b/core/modules/update/tests/src/Functional/UpdateTestBase.php
   @@ -277,6 +277,29 @@ protected function assertUpdateTableElementContains($text) {
   +  /**
   +   * Asserts that the update table text does not contain the specified text.
   +   *
   +   * @param string $text
   +   *   The expected text.
   +   */
   +  protected function assertUpdateTableTextNotContains($text) {
   +    $this->assertSession()->elementTextNotContains('css', $this->updateTableLocator, $text);
   +  }
   

   Could this method be placed directly after the method assertUpdateTableTextContains() as the two method belong together.

@nikitagupta: Thank you for the reroll.

@nikitagupta: Thank you for fixing the patch!

The patch does what the proposed solution from the IS is saying that should be done.
All code changes look goo to me.
For me it is RTBC.

I reviewed the patch again, and it looks all right. RTBC +1

@ilgnerfagundes I'm seeing quite a few single line comments where you are attaching a screenshot that provides no additional info beyond what the bot / dreditor / gitlab already gives us. So, I've removed the issue credit for that screenshot. In the future, you can get credit for issues by reading the issue to understand its purpose, and posting your review or testing of that purpose. Thank you!

+++ /dev/null
@@ -1,932 +0,0 @@
-  public function testSecurityCoverageMessage($installed_version, $fixture, $requirements_section_heading, $message, $mock_date) {
...
-  public function testDatestampMismatch() {
...
-  public function testModulePageRunCron() {
...
-  public function testClearDiskCache() {
...
-  public function testModulePageUpToDate() {
...
-  public function testModulePageRegularUpdate() {
...
-  public function testModulePageSecurityUpdate() {
...
-  public function testServiceUnavailable() {
...
-  public function testFetchTasks() {
...
-  public function testLanguageModuleUpdate() {
...
-  public function testLocalActions() {
...
-  public function testBrokenThenFixedUpdates() {

These tests are being removed by the patch with no obvious replacement. I've searched the issue for most of these tests and I can't see any reasons why they are being removed - only comments which say we need to decide where to put them (eg. testSecurityCoverageMessage). Either the issue summary needs updating to explain why they're not necessary anymore or we should add them to \Drupal\Tests\update\Functional\UpdateSemverCoreTest (or somewhere else).

Hello guys, I put the methods mentioned in comment # 64 and I'm uploading the patch, but I don't know how I do it to test it.

@Ilgner: Please adjust this patch according to @alexpott in comment #64.

sorry, i had done the wrong patch, i'm sending the new patch

Fixed custom commands issues of patch #67.

Fixing tests.

At @tedbow's behest, I just "rerolled" patch #35 into the new merge request !465. I didn't make any changes beyond resolving the conflicts (and fixing coding standards violations so that the tests would run).

In doing so, I discovered the answer to @alexpott's question in #64: the test methods are not being removed; they are simply moved wholesale out of UpdateSemverTestBase and into UpdateCoreTest.

We still need to reapply @daffie's feedback in #58 to return this to RTBC, so I'm assigning this to @tedbow in order to complete it. Since this was very close to a straight-up reroll, hopefully I will still be within issue queue etiquette if I restore RTBC once that's done.

OK, everything looks good here. The merge request is green, and it appears all feedback is addressed and follow-ups are filed. I say we've done enough iterating. Since my work on this was just a straight-up reroll (and two minor coding standards fixes required to get tests to run), I feel okay re-RTBCing it.

Committed and pushed 27070f4c91 to 9.3.x and a1822890bf to 9.2.x. Thanks!

The random xml indentation bugs me but it seems like all of the update xml has this so no reason to hold up this one.