Problem/Motivation

Since #3390193: Add a drupalGet() method to KernelTestBase we can convert functional tests that make simple http requests to kernel tests. This issue is an attempt to find low hanging fruit in the system module.

For this I am experimenting with using Github Copilot chat in PhpStorm with Claude Opus 4.6 model. I guided it to find tests in system module that have drupalGet requests, and worked with it to detect patterns that are too complicated for a first pass. Once it had found candidates it performed the conversion and adjusted setup tasks until the tests passed. Then once it was passing it was instructed to remove as many additional steps as possible to produce a minimal diff while still passing. Finally it committed each one with a commit message that indicates the time reduction.

Here is a summary of the results:

| Test                              | Before  | After  | Saved   |
|-----------------------------------|---------|--------|---------|
| AdminMetaTagTest                  |  3.356s | 0.908s |  2.448s |
| ContentNegotiationTest            |  3.291s | 1.109s |  2.182s |
| DefaultMetatagsTest               |  3.166s | 1.042s |  2.124s |
| DefaultMobileMetaTagsTest         |  6.997s | 1.577s |  5.420s |
| ElementsContainerTest             |  4.397s | 0.795s |  3.602s |
| EngineNyanCatTest                 |  4.028s | 0.999s |  3.029s |
| ImageLoadingAttributeTest         |  3.858s | 0.780s |  3.078s |
| RenderArrayNonHtmlSubscriberTest  |  3.619s | 0.694s |  2.925s |
| ServiceProviderWebTest            |  4.287s | 1.104s |  3.183s |
| TimeZoneAbbreviationRouteTest     | 46.916s | 8.335s | 38.581s |
| TwigEnvironmentTest               |  3.586s | 0.709s |  2.877s |

Total before: 87.501s
Total after: 18.052s
Total time saved: 69.449s
Reduction: 79.4%

The juiciest one here is TimeZoneAbbreviationRouteTest because it was using a data provider. This could be a good pattern to target when finding other tests to convert.

Steps to reproduce

Proposed resolution

Remaining tasks

Postponed on #3582228: add $options and $headers parameters to HttpKernelUiHelperTrait::drupalGet()

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3583587

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

mstrelan created an issue. See original summary.

mstrelan opened merge request !15376

mstrelan’s picture

mstrelan commented
Issue summary: View changes
Status: Active » Needs review
mstrelan’s picture

mstrelan commented

Had to revert ContentNegotiationTest and TimeZoneAbbreviationRouteTest while we wait for #3582228: add $options and $headers parameters to HttpKernelUiHelperTrait::drupalGet(). Could postpone on that or move these to a follow up.

andypost’s picture

andypost
he/him
Primary language Russian
commented

Looks great! curious why some tests require system module installed it may cut more

mstrelan’s picture

mstrelan commented

@andypost the tests live in system module, so presumably they are testing some functionality from that module, otherwise they should probably be in core/tests. Each test was run without installing system module, and it was only added in if it was required to make the test pass. FWIW system module is a required module so it is always installed in Functional tests. Changing to Kernel tests means we might need to explicitly install it. Some of the tests require config from system module simply to set the front page route since that's what drupalGet is requesting.

dww’s picture

dww
we/he/they
commented
Status: Needs review » Reviewed & tested by the community
Related issues: +#3582228: add $options and $headers parameters to HttpKernelUiHelperTrait::drupalGet()

Re: #4: I vote move those to a follow-up for once #3582228: add $options and $headers parameters to HttpKernelUiHelperTrait::drupalGet() lands. The rest of this looks great. Might as well get these in ASAP.

Although the toString() in core/modules/system/tests/src/Kernel/Render/RenderArrayNonHtmlSubscriberTest.php wouldn't be necessary once #3582228 is committed, either, since that's expanding the Kernel drupalGet() to handle either a string or a Url $path arg. /shrug

mstrelan’s picture

mstrelan commented

Thanks for the review. I'd be happy to postpone on #3582228: add $options and $headers parameters to HttpKernelUiHelperTrait::drupalGet() so we can un-revert those two commits and simplify the tests using toString() and manually implementing xpath. Looks like it's close.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs review

Moving back to needs review for #8, do we actually want to postpone this, or open a follow-up for further things once the other issue is in? But I think we should decide before commit.

mstrelan’s picture

mstrelan commented
Title: Convert some functional tests in system module to kernel tests » [PP-1] Convert some functional tests in system module to kernel tests
Issue summary: View changes
Status: Needs review » Postponed

I think postpone, the biggest time saving was reverted and it would be good to restore it, and not have to rework some of the others later.

mstrelan’s picture

mstrelan commented
Title: [PP-1] Convert some functional tests in system module to kernel tests » Convert some functional tests in system module to kernel tests
Status: Postponed » Needs review

Blocker is in. Restored the reverted commits and removed the additional toString() calls that are no longer required.

dww’s picture

dww
we/he/they
commented
Status: Needs review » Reviewed & tested by the community

The MR diff looks great once more. Nothing to complain about. Back to RTBC. Thanks!

catch’s picture

catch
he/him
Primary language English
commented

Sorry one more question: #3583594: Add getUrl() and (renamed) xpath() helpers to HttpKernelUiHelperTrait is open, I don't think it's a showstopper to commit this without that issue having landed yet, but then can we convert the test that needs it here in that issue?

Otherwise there's no actual test logic changes in here at all, just slightly different setup, which is great.

mstrelan’s picture

mstrelan commented

I think the short answer is yes. The xpath stuff is a bit messy due to the naming conflict and I'd rather not wait for it. Would prefer to exclude that test from this issue, is that what you are suggesting too?

catch’s picture

catch
he/him
Primary language English
commented

I think we could probably go ahead here but make a note to update it again in that issue?

mstrelan’s picture

mstrelan commented

Done, added to the remaining tasks of the other issue.

  • catch committed 104b606a on 11.x 
    task: #3583587 Convert some functional tests in system module to kernel...

catch closed merge request !15376

  • catch committed 92aa759e on main 
    task: #3583587 Convert some functional tests in system module to kernel...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to main and 11.x, thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.