Problem/Motivation

follow up to #3100386: Create contrib update module test cases that use semantic versioning
\Drupal\Tests\update\Functional\UpdateCoreTest mostly has test methods that deal with updates to the core Drupal project has it's name implies.

Certain test methods such as \Drupal\Tests\update\Functional\UpdateCoreTest::testLocalActions() are just general update module tests.

Proposed resolution

Move test methods from \Drupal\Tests\update\Functional\UpdateCoreTest that don't deal with Drupal core update releases to their own class.

Remaining tasks

Review MR 1358, https://git.drupalcode.org/project/drupal/-/merge_requests/1358

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#17 raw_diff_MR1358-MR2255.txt775 bytesspokje

Issue fork drupal-3127177

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

tedbow created an issue. See original summary.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Issue summary: View changes

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

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

kunal.sachdev made their first commit to this issue’s fork.

kunal.sachdev opened merge request !1358

kunal.sachdev’s picture

kunal.sachdev commented
Status: Active » Needs review

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Status: Needs review » Needs work

@kunal.sachdev this is looking good but I think we can move a few more methods over.

The following methods in UpdateSemverCoreTest to require a core version to be set but they don't really test which version is recommended for core they just need something set so they can test regular functionality(non core specific) so they should also be moved to UpdateTest

  1. testModulePageUpToDate
  2. testModulePageSecurityUpdate
  3. testModulePageRunCron
  4. testModulePageRegularUpdate
  5. testLanguageModuleUpdate (although this is testing the "Language" module appears it really is just testing that a module within a project shows up(we should make a follow-up to generalize this and make sure a contrib one shows up too)

The problem is that all these methods use $this->setProjectInstalledVersion('8.0.0'); and setProjectInstalledVersion is defined in UpdateSemverTestBase. We can solve this by not calling setProjectInstalledVersion() and instead just doing 

$setting = [
      '#all' => [
        'version' => '8.0.0',
      ],
    ];
    $this->config('update_test.settings')->set('system_info', $setting)->save();

In \Drupal\Tests\update\Functional\UpdateTest::setUp()

kunal.sachdev’s picture

kunal.sachdev commented
Status: Needs work » Needs review
tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Status: Needs review » Reviewed & tested by the community

This looks good. Assuming it passes tests RTBC!

quietone’s picture

quietone commented

The new UpdateMiscTest class contains 4 tests about the ModulePage, (testModulePageRunCron, testModulePageSecurityUpdate, testModulePageRegularUpdate, testModulePageUpToDate). Would they be better off and easier to find in their own file?

quietone’s picture

quietone commented
Version: 9.4.x-dev » 10.0.x-dev
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

This needs to be on 10.0.x, changing version. Also, tests are not running, maybe needs a rebase, adding reroll tag.

spokje’s picture

spokje
Primary language Dutch
commented
Assigned: Unassigned » spokje

Spokje opened merge request !2255

spokje’s picture

spokje
Primary language Dutch
commented
Issue tags: -Needs reroll
StatusFileSize
new raw_diff_MR1358-MR2255.txt775 bytes

MR!2255 is currently MR!1358 1-on-1 against 10.0.x-dev

Attached raw_diff between the 2 MRs

spokje’s picture

spokje
Primary language Dutch
commented
Assigned: spokje » Unassigned
Status: Needs work » Needs review

Moved Module Page related tests to a new separate Test Class (addressing #14)

smustgrave made their first commit to this issue’s fork.

smustgrave’s picture

smustgrave commented

Rebased for 10.0.x. May need to update MR for 10.1.x

but lets see if there is green.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Appears to have test failures.

spokje’s picture

spokje
Primary language Dutch
commented
Version: 10.0.x-dev » 10.1.x-dev
Assigned: Unassigned » spokje

I think @smustgrave is right and this should be targeting 10.1.x now.

Spokje opened merge request !3042

Spokje closed merge request !2255

spokje’s picture

spokje
Primary language Dutch
commented

Closed the 10.0.x MR and opened a new 10.1.x MR.

Rebasing was a nightmare and there were no comments on the old MR anyway.

spokje’s picture

spokje
Primary language Dutch
commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

I see all green now. https://git.drupalcode.org/project/drupal/-/merge_requests/3042 appears to be good to merge so feel comfortable marking

spokje’s picture

spokje
Primary language Dutch
commented
Assigned: spokje » Unassigned
xjm’s picture

xjm
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

Thanks everyone for working on this and for trying to update it for 10.1.x.

The issue title says that the issue will move test methods, but nothing is being moved in the current MR. Instead, the methods are just being copied, and are still present in UpdateSemverCoreTest after the MR is applied.

Can we correctly move them as the issue summary and title describe, and also verify that nothing else from the original MR has gone missing in the process? It's always important to check the diffstat and read the whole change set when resolving merge conflicts. Thanks!

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone’s picture

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

Rebase the old MR to 11.x and then moved the functions. Review MR 1358

smustgrave changed the visibility of the branch 10.1.x to hidden.

smustgrave changed the visibility of the branch 3127177-10.1.-move-test-methods to hidden.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Bunch of super small stuff. Probably find to self RTBC after that.

quietone’s picture

quietone commented
Status: Needs work » Reviewed & tested by the community

The latest changes were to apply suggestions for adding void returns to new methods. So, I am setting to RTBC per #34.

  • catch committed b4d8219a on 10.3.x 
    Issue #3127177 by kunal.sachdev, Spokje, quietone, smustgrave, tedbow,...
catch’s picture

catch
he/him
Primary language English
commented
Version: 11.x-dev » 10.3.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 11.x and cherry-picked to 10.3.x, thanks!

  • catch committed 820d76b2 on 11.x 
    Issue #3127177 by kunal.sachdev, Spokje, quietone, smustgrave, tedbow,...

quietone closed merge request !1358

Status: Fixed » Closed (fixed)

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