Problem/Motivation

As discovered in #3267458-6: Deprecate aggregator module in Drupal 9.4:

Modules which are deprecated are still testing for valid config routes, thus triggering the deprecation warning, making TestBot sad.

Steps to reproduce

- Deprecate module with a config route
- Run test
- Weep

Proposed resolution

Exclude deprecated modules from being tested in \Drupal\KernelTests\Core\Extension\ModuleConfigureRouteTest::testModuleConfigureRoutes and add a legacy test that tests deprecated modules without triggering a deprecation notice.

Remaining tasks

Stuff

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#14 1986.diff2.93 KBSpokje
#12 1986.diff2.68 KBSpokje

Issue fork drupal-3270323

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
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Spokje created an issue. See original summary.

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje at Chromatic commented

Spokje opened merge request !1986

catch’s picture

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

Looks good.

xjm’s picture

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

We're losing test coverage here. Shouldn't we add a legacy test that explicitly tests this for deprecated modules, if we skip them here?

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje at Chromatic commented
Assigned: Spokje » Unassigned
Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje at Chromatic commented
Status: Needs work » Needs review

Makes sense, attempted to add that legacy test.

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje at Chromatic commented
Issue summary: View changes
catch’s picture

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

Test failures look real.

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje at Chromatic commented

Test failures look real.

I would argue that even The Struggle is real, but at the very least I've made another stab to fix the test failures

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje at Chromatic commented
Status: Needs work » Needs review
Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje at Chromatic commented
FileSize
1986.diff2.68 KB

Testing the plain diff from the current MR on 9.4.x-dev which actually has a deprecated module (HAL).

murilohp’s picture

murilohp CreditAttribution: murilohp at CI&T commented

The test looks good! Commenting here because for some reason if I comment only on the MR, the issue is not automatically followed.

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje at Chromatic commented
FileSize
1986.diff2.93 KB

Thanks for the review @murilohp!

I've changed (and resolved) one of your threads, and left an explanation in the other.
Hope the helps, if not: Just leave a remark :)

Added a new plain diff from the current state of the MR for testing against 9.4.x-dev.

murilohp’s picture

murilohp CreditAttribution: murilohp at CI&T commented
Status: Needs review » Reviewed & tested by the community

The patch looks good to me! The testbot is happy and all the threads were addressed, so it's RTBC!

  • catch committed 04b4d51 on 10.0.x 
    Issue #3270323 by Spokje, murilohp, xjm: ModuleConfigureRouteTest::...
  • catch committed 13c453a on 9.4.x
    Issue #3270323 by Spokje, murilohp, xjm: ModuleConfigureRouteTest::...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Version: 10.0.x-dev » 9.4.x-dev
Status: Reviewed & tested by the community » Fixed

I didn't know you could use @group legacy on methods that aren't the actual test, that's clever.

Committed/pushed to 10.0.x and cherry-picked to 9.4.x, thanks!

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented

I didn't know you could use @group legacy on methods that aren't the actual test, that's clever.

Me neither -- neat!

murilohp’s picture

murilohp CreditAttribution: murilohp at CI&T commented

Sorry for coming back here and say that unfortunately this will not solve the problem :(, testing here using the parent issue branch, we still see the deprecation notice.

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje at Chromatic commented
Version: 9.4.x-dev » 10.0.x-dev
Assigned: Unassigned » Spokje
Status: Fixed » Needs review

I didn't know you could use @group legacy on methods that aren't the actual test, that's clever.

Way too clever as it turns out, this is just not working, as @murilohp discovered.
The testing against 9.4.x-dev was moot, since at this point only hal is deprecated and that module doesn't have a configure route, meaning this test was skipped before installing it.
(When something seems to good to be true, it probably isn't...)

So, I've re-opened this one and made separate test methods for deprecated and non-deprecated modules.
Running it locally against #3267458: Deprecate aggregator module in Drupal 9.4 worked for me.

Writing a test to test a test seems a bit much, but I've been wrong before...
(...basically all comments in this issue)

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs review » Reviewed & tested by the community

The code works and I ran the tests locally to confirm with and without the aggregator patch.

I do wonder if we should open a followup to pre-filter in the data provider, rather than skipping most of the tests? Locally it takes over two minutes to run but over 75% of the tests are skipped so this is a lot of wasted setup (not as bad as if they were functional tests, but still):

Testing Drupal\KernelTests\Core\Extension\ModuleConfigureRouteTest
SSS.SSSSSSSSSS.SSS...SSSS..S......SS..SSS..SSS....S...SS...SS.S  63 / 162 ( 38%)
...SSSS..S...S...SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS 126 / 162 ( 77%)
SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS                            162 / 162 (100%)

Time: 02:20.503, Memory: 8.00 MB

OK, but incomplete, skipped, or risky tests!
Tests: 162, Assertions: 39, Skipped: 123.

  • catch committed bb24e8b on 10.0.x 
    Issue #3270323 by Spokje, murilohp, catch, xjm, longwave:...
  • catch committed ad005a7 on 9.4.x
    Issue #3270323 by Spokje, murilohp, catch, xjm, longwave:...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Version: 10.0.x-dev » 9.4.x-dev
Status: Reviewed & tested by the community » Fixed

Way too clever as it turns out, this is just not working, as @murilohp discovered.

This explains how we'd never thought of it before ;)

Yep was thinking the same with the data provider. I think we discussed that already in another issue, but I can't remember where or if there's already a follow-up, so I opened #3271010: Add stable/deprecated versions of coreModuleListDataProvider.

Committed/pushed to 10.0.x and cherry-picked to 9.4.x, thanks!

Spokje closed merge request !1986

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje at Chromatic commented
Assigned: Spokje » Unassigned

Status: Fixed » Closed (fixed)

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