Problem/Motivation

A LOT of browser tests are running the language settings form. Making form submissions in a test is expensive, and should not be done as part of test setup - API calls should be used instead.

In particular, content_translation's form alterations to this form make it VERY expensive to submit. This is adding a lot of needless processing to tests.

A search for the path 'admin/config/regional/content-language' in functional tests shows LOTS of test files:

contact/tests/src/Functional/ContactLanguageTest.php
menu_ui/tests/src/Functional/MenuUiNodeTest.php
content_moderation/tests/src/Functional/ModerationContentTranslationTest.php
content_moderation/tests/src/Functional/ModerationFormTest.php
content_moderation/tests/src/Functional/ModerationLocaleTest.php
file/tests/src/Functional/FileOnTranslatedEntityTest.php
file/tests/src/Functional/PrivateFileOnTranslatedEntityTest.php
path/tests/src/Functional/PathLanguageTest.php
path/tests/src/Functional/PathContentModerationTest.php
language/tests/src/Functional/LanguageConfigSchemaTest.php
language/tests/src/Functional/EntityTypeWithoutLanguageFormTest.php
language/tests/src/Functional/LanguageSelectorTranslatableTest.php
workspaces/tests/src/Functional/PathWorkspacesTest.php
field/tests/src/Functional/EntityReference/EntityReferenceFieldTranslatedReferenceViewTest.php
image/tests/src/Functional/ImageOnTranslatedEntityTest.php
node/tests/src/Functional/NodeTranslationUITest.php
views/tests/src/Functional/Plugin/DisplayFeedTranslationTest.php
views/tests/src/Functional/SearchMultilingualTest.php
content_translation/tests/src/FunctionalJavascript/ContentTranslationContextualLinksTest.php
content_translation/tests/src/Functional/ContentTranslationSyncImageTest.php
content_translation/tests/src/Functional/ContentTranslationEnableTest.php
content_translation/tests/src/Functional/ContentTranslationStandardFieldsTest.php
content_translation/tests/src/Functional/ContentTranslationDisableSettingTest.php
content_translation/tests/src/Functional/ContentTranslationOperationsTest.php
content_translation/tests/src/Functional/ContentTranslationUntranslatableFieldsTest.php
content_translation/tests/src/Functional/ContentTranslationContextualLinksTest.php
content_translation/tests/src/Functional/ContentTranslationUISkipTest.php
content_translation/tests/src/Functional/ContentTranslationLanguageChangeTest.php
content_translation/tests/src/Functional/ContentTranslationSettingsTest.php

Some of them such as ContentTranslationEnableTest or ContentTranslationDisableSettingTest are clearly about using the settings form.

But most of them have no need to use the form and should use the API.

Steps to reproduce

Proposed resolution

Identify which tests are specifically testing the form's functionality and document that.

Tests which are covering the form's functionality will be tests that either:

- check output on the form
- check output after submitting the form
- check the site state after submitting the form

The following tests are specifically testing the form:

- content_translation/tests/src/Functional/ContentTranslationEnableTest.php
- content_translation/tests/src/Functional/ContentTranslationDisableSettingTest.php
- content_translation/tests/src/Functional/ContentTranslationUISkipTest.php
- content_translation/tests/src/Functional/ContentTranslationSettingsTest.php

The following tests should be fixed:

- ContactLanguageTest - has no business testing this form, it doesn't belong to this module!
- ModerationFormTest - same
- ContentTranslationContextualLinksTest - says it's using the form 'This tests that caches are properly invalidated' but that surely belongs in a test with a more accurate name.
- ContentTranslationOperationsTest - needs fixing
- ContentTranslationStandardFieldsTest - not sure. The code makes it look like it's checking the standard profile is correctly set up for translation. But the docblock 'Tests that translatable fields are being rendered' suggests it's testing the form. But if that's the case, it shouldn't use the expensive to install standard profile, but test modules instead.
- ContentTranslationSyncImageTest - says " Check that the content translation settings page reflects the changes performed in the field edit page." - but weird place to do that in. Future refactor?
- ContentTranslationUntranslatableFieldsTest - fix
- EntityReferenceFieldTranslatedReferenceViewTest - fix
- ImageOnTranslatedEntityTest - fix
- LanguageConfigSchemaTest - fix
- MenuUiNodeTest - fix. Test comment says 'Use a UI form submission to make the node type and menu link content entity translatable.' but that's just silly.
- NodeTranslationUITest - fix
- PathContentModerationTest - fix
- PathLanguageTest - fix
- SearchMultilingualTest - fix
- DisplayFeedTranslationTest - fix
- PathWorkspacesTest - fix
- RouteCachingLanguageTest - fix
- PrivateFileOnTranslatedEntityTest - fix
- FileOnTranslatedEntityTest - fix

Remaining tasks

1. Identify the tests
2. Add a @covers ContentLanguageSettingsForm or @covers _content_translation_form_language_content_settings_form_alter to these test classes to document that they are correctly testing the form
3. File follow-on issues to convert other tests to API calls (as child issues of #3305376: [Meta] Perform set-up tasks in Browser tests using API calls rather than browser requests.

Issue fork drupal-3355787

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

joachim created an issue. See original summary.

joachim’s picture

joachim commented
Issue summary: View changes

These are the ones which I think are testing the form:

content_translation/tests/src/Functional/ContentTranslationEnableTest.php
content_translation/tests/src/Functional/ContentTranslationDisableSettingTest.php
content_translation/tests/src/Functional/ContentTranslationUISkipTest.php
content_translation/tests/src/Functional/ContentTranslationSettingsTest.php

I note in passing that there appears to be no coverage in language module of the ContentLanguageSettingsForm *without* content_translation's form alterations. Is that something that should be fixed as side issue?

joachim’s picture

joachim commented
Issue tags: +sustainability
joachim’s picture

joachim commented

Tests that run the form in a test method rather than setUp():

- ContactLanguageTest - has no business testing this form, it doesn't belong to this module!
- ModerationFormTest - same
- ContentTranslationContextualLinksTest - says it's using the form 'This tests that caches are properly invalidated' but that surely belongs in a test with a more accurate name.
- ContentTranslationOperationsTest - needs fixing
- ContentTranslationStandardFieldsTest - not sure. The code makes it look like it's checking the standard profile is correctly set up for translation. But the docblock 'Tests that translatable fields are being rendered' suggests it's testing the form. But if that's the case, it shouldn't use the expensive to install standard profile, but test modules instead.
- ContentTranslationSyncImageTest - says " Check that the content translation settings page reflects the changes performed in the field edit page." - but weird place to do that in. Future refactor?
- ContentTranslationUntranslatableFieldsTest - fix
- EntityReferenceFieldTranslatedReferenceViewTest - fix
- ImageOnTranslatedEntityTest - fix
- LanguageConfigSchemaTest - fix
- MenuUiNodeTest - fix. Test comment says 'Use a UI form submission to make the node type and menu link content entity translatable.' but that's just silly.
- NodeTranslationUITest - fix
- PathContentModerationTest - fix
- PathLanguageTest - fix
- SearchMultilingualTest - fix
- DisplayFeedTranslationTest - fix
- PathWorkspacesTest - fix
- RouteCachingLanguageTest - fix

dww’s picture

dww
we/he/they
commented

Love this, thanks for getting it going! Hope to have time to help sometime "soon".

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.

joachim’s picture

joachim commented
Status: Active » Needs review

Going to set this to NR - the list in #4 needs review and approval before doing the work.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Think could be added too

PrivateFileOnTranslatedEntityTest
FileOnTranslatedEntityTest

joachim’s picture

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

Thanks!
Updated the IS to add the analysis so far, to make reviewing easier.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

So I believe the list you got is good.

Now that is a lot so not sure if breaking it up would be quicker in the long run.

joachim’s picture

joachim commented
Status: Needs work » Needs review

Made a MR which adds @covers tags to the tests which are actually testing the form.

Once this is merged, we can file follow-on issues to convert all the other tests which use this form to use API calls instead.

joachim opened merge request !4659

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs followup

Tests all green so the @covers seem good.

Tagging for followup after merged.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented

+1 on baby steps to annotate these tests, and then follow through with converting other tests to use API calls in other issues.

My one very minor gripe is that @covers has a specific meaning for PHPUnit, such that all the coverage in a coverage report for that test will be filtered so it only contains that class or function. So this is a little bit of an abuse of the annotation, but is also completely reasonable for this process.

Some of these tests could likely be converted to KTB, but that's another meta for another day.

Very curious how the determination was made per test... Is there some magical regex or other methodology to generate a list like this? We could apply it to other settings forms, in order to find places to convert form-fill actions to API calls.

joachim’s picture

joachim commented

> Some of these tests could likely be converted to KTB, but that's another meta for another day.

I've got my eye on that sort of thing too! Functional tests which just check the page content for a simple string could be done as kernel tests. But we need a way to write the responses to HTML files for debugging and development.

> Very curious how the determination was made per test... Is there some magical regex or other methodology to generate a list like this?

I just did a search for drupalGet('PATH') in all files whose name matches '/Functional/'.

I don't think I managed to find a way to refine it to look specifically in the setUp() method. But that is a good heuristic - we could potentially teach it to DrupalRector. It's not comprehensive though - lots of tests to setup-ish things in their test methods.

  • catch committed a55e653e on 11.x 
    Issue #3355787 by joachim, smustgrave, Mile23: Identify which browser...
catch’s picture

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

Committed a55e653 and pushed to 11.x. Thanks!

joachim’s picture

joachim commented
Assigned: Unassigned » joachim

I'm working my way down the list filing issues for the tests to fix.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Parent issue: » #3305376: [Meta] Perform set-up tasks in Browser tests using API calls rather than browser requests

Setting this issue to be a child of the meta.

The follow-ups are other children.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Issue tags: -Needs followup
joachim’s picture

joachim commented
Assigned: joachim » Unassigned

All done! Phew!

Status: Fixed » Closed (fixed)

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