Split from #3087644: Remove Drupal 8 updates up to and including 88**:

IMO the next step is to move the tests in that issue to a sub-namespace, i.e. system\Functional\Update\UpdateSystem, so that it's very easy to review the mega-patch here to see that they haven't been removed.

CommentFileSizeAuthor
#7 updatesystem-tests-3106395-7.patch13.99 KBBerdir
#2 updatesystem-tests-3106395-2.patch14.14 KBBerdir
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir created an issue. See original summary.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Active » Needs review
FileSize
updatesystem-tests-3106395-2.patch14.14 KB

This moves all tests that have "use UpdatePathTestTrait;", including the one in workspaces, as well as as UpdatePathTestBaseFilledTest.

Wasn't sure about, but that's already in a special place together with the base class, so I left that.

Some of the remaining ones I'm not 100% sure whether they could be useful:

\Drupal\Tests\system\Functional\Update\WarmCacheUpdateFrom8dot6Test: Was added for a specific bug, but having a test for updates might be useful later on too, even though we now have the special update cache backend.

\Drupal\Tests\system\Functional\Update\NoDependenciesUpdateTest: was also added for a specific bug, and I think \Drupal\FunctionalTests\Update\UpdatePathTestBaseTest is pretty minimal too with drupal-8.8.0.bare.standard.php.gz? ( Didn't investigate why \Drupal\Tests\system\Functional\Update\UpdatePathRC1TestBaseTest wasn't enough to cover this which I'd assume is equally bare.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

Not quite sure if this is the namespace that @catch had in mind, in a previous comment, he said:

Put new tests for actual updates, in an additional Updates namespace (i.e. ones added from now onwards).

That way in Drupal 10 we only need to rm -rf the Updates folder and we'll know we're not removing important coverage. Moving the updates system tests before doing other removals here will also make the removals patch easier to review.

With system\Functional\Update\UpdateSystem, we can't do the rm -rf Update as that includes UpdateSystem, should we move UpdateSystem out so it's next to Update and not inside?

Status: Needs review » Needs work

The last submitted patch, 2: updatesystem-tests-3106395-2.patch, failed testing. View results

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

Ah, this breaks the relative path of course for that test. Will wait on feedback on the namespace before fixing that. Moving it one level up would fix it too.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented

@Berdir yeah I think core/modules/system/tests/src/Functional/Update/UpdateSystem should be core/modules/system/tests/src/Functional/UpdateSystem

I think removing NoDependenciesUpdateTest is fine. That is about testing without the block module installed. Very rare case. Also I think removing WarmCacheUpdateFrom8dot6Test is okay too. That was for a very specific error around extension objects.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Needs work » Needs review
FileSize
updatesystem-tests-3106395-7.patch13.99 KB

Sounds good to me. Moved the classes. I also removed @group legacy from two tests that still had it. Not much point in an interdiff.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Needs review » Reviewed & tested by the community

I was thinking Update/UpdateSystem and Update/Updates but moving them sideways instead of downwards is a much better idea...

Looks solid to me.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Version: 9.0.x-dev » 8.9.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

@catch was about to commit this but I think we need to land #3102059: Make tests of the update system use UpdatePathTestTrait instead of UpdatePathTestBase too because then maintaining both 8.9.x and 9.0.x is simpler because these tests are in the same place.

I guess we can commit to 9.0.x and wait on that issue.

Committed 46eaa9b and pushed to 9.0.x. Thanks!

  • alexpott committed 46eaa9b on 9.0.x 
    Issue #3106395 by Berdir, catch: Move tests that test the update system...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented

#3102059: Make tests of the update system use UpdatePathTestTrait instead of UpdatePathTestBase is in, so this is ready to go now.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Patch (to be ported) » Needs review

I requested an 8.9.x test.

  • catch committed e3b4d5e on 8.9.x authored by alexpott 
    Issue #3106395 by Berdir, catch: Move tests that test the update system...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Needs review » Fixed

8.9.x is green so I've gone ahead and cherry-picked to 8.9.x.

Status: Fixed » Closed (fixed)

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