Use the API to set up languages in tests that are not specifically testing the language form

    $this->drupalGet('node/add/moderated_content');
    $this->submitForm([
      'title[0][value]' => 'Some moderated content',
      'body[0][value]' => 'First version of the content.',
      'moderation_state[0][state]' => 'draft',
    ], 'Save');

    $this->drupalGet('node/' . $node->id() . '/edit');
    $this->submitForm(['moderation_state[0][state]' => 'published'], 'Save');
    $this->drupalGet('node/' . $node->id() . '/edit');
    $this->submitForm(['moderation_state[0][state]' => 'archived'], 'Save');

This is a larger file but see a few other instances we could probably replace. Probably a few more then the ones I posted.

I will give a check. As the ticket title mentioned "to setup language", I restricted my check to this only.

From my point of view, the others form submit look legit as they serve the purpose of the tests (testing the moderation form).

But it’s not really asserting anything. It’s just going to the node edit form to update a state. Think that could be done directly on the node and saved without visiting the form.

Can see if the committers want to change it. Could argue it's out of scope slightly but seems like small enough changes to me.

The Needs Review Queue Bot tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request. Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)

This only had one patch so think it's clear what was reviewed.

I agree that the language configuration form is not what's under test, so it makes sense to do this with the API.

That said, there's also an entire freaking container rebuild after these lines, which is even more expensive. Makes one question if this should be done inside the test method at all?

It's also super a lot of API calls in a row for a single test method, and something that it seems like a single test trait or the like should provide.

Finally, I reviewed a different issue that was around lines with the same form-based translation setup in a test of not-the-translation-form elsewhere. In fact, core uses this exact pattern in 26 other places:

[ayrton:maintainer | Fri 16:18:56] $ grep -r "admin/config/regional/content-language" * | grep "drupalGet" | wc -l
      27

So, if we're fixing this one spot, we should probably fix all the rest as well, in a consistent way. We should check whether there is an existing trait to set up a language via the API with a single method call, and if not, add one. We also should have that happen in setUp() rather than individual test methods. (We might need to split up some tests if there are other methods in the same class not requiring a language.)

NW to expand the scope of this. Thanks!

One issue per file is not the correct way to scope this. It needs an API.

As a Drupal core release manager, I have discretion over issue scope, so please don't revert my scoping decisions.

I will comment on #3305376: [Meta] Perform set-up tasks in Browser tests using API calls rather than browser requests.

I have started with the patch at #3 as the initial commit.

@xjm, I have started to follow the suggestion to use something separate to unify these groups of API calls.
Currently, it is using traits on the test namespace, but if enough useful and generalizable enough, it may become an actual new API surface.
Keeping it on the trait, since it may not be generic enough now.

I have been changing the code added on related issues, i.e. #3355604: ContentTranslationLanguageChangeTest should perform setup tasks using API calls not form submissions, #3384935: ContactLanguageTest should use API to set up language, #3385837: PathContentModerationTest should use API to set up language, and #3385834: MenuUiNodeTest should use API to set up language; since those cases are not matching the grep.
I also included changes from #3385815: ImageOnTranslatedEntityTest should use API to set up language, and then modified them to use the created traits.

New commits on new 3384936-use-lang-api-instead-of-form topic branch and related new MR #5485 created with the following commits.

- bf992c5553 #3384936-3: Use API to set up languages in ModerationFormTest test
- ec28e72357 Add a language trait helper
- 9709759d3b Start using the language trait on ModerationFormTest
- b01c84b5ad Add enable bundle translation helper to language trait
- 6317ec7a73 Add a content translation test trait helper, extending language test trait helper
- 589ae61595 Move content translation manager actions to the content translation test trait
- e0edafbf50 Use content translation test trait on MenuUiNodeTest
- 1560fb1410 Use content translation test trait on PathContentModerationTest
- 965cfbfcc1 Use language test trait on ContactLanguageTest
- 101a903ac4 Use content translation test trait on ContentTranslationLanguageChangeTest
- 799c1d49e5 Apply patch from #3385815-4 by vbouchet, ayush.khare: ImageOnTranslatedEntityTest should use API to set up language
- 3dffbc3a33 Use content translation test trait on ImageOnTranslatedEntityTest
- e41d0c318a Add a helper to set a field translation status on tests
- 42cf800493 Use new setFieldTranslatable method on ImageOnTranslatedEntityTest
- 0a757c5fee Use language test trait on ContentTranslationUntranslatableFieldsTest
- 95df2e0802 Fix phpcs issues on changed lines :sweat_smile:
- b3fe756f21 Use content translation test trait on ModerationContentTranslationTest
- 8dd7b2b5af Use content translation test trait on ModerationLocaleTest
- 3d2ae9633c Use content translation test trait on ContentTranslationContextualLinksTest
- 474a791068 Use content translation test trait on EntityReferenceFieldTranslatedReferenceViewTest
- aeec7bc391 Remove unused use statement

$ git diff --shortstat  11.x
 13 files changed, 165 insertions(+), 127 deletions(-)

Changes are growing, but they still feel similar.

Also, many of the grep matches are actually relevant to be using the UI, most of the set at core/modules/content_translation/tests/src/Functional/\*Test.php.
Here an alternative grep, via git.

$ git grep -e 'admin/config/regional/content-language' --and -e 'drupalGet' --name-only 
core/modules/content_translation/tests/src/Functional/ContentTranslationContextualLinksTest.php
core/modules/content_translation/tests/src/Functional/ContentTranslationDisableSettingTest.php
core/modules/content_translation/tests/src/Functional/ContentTranslationEnableTest.php
core/modules/content_translation/tests/src/Functional/ContentTranslationOperationsTest.php
core/modules/content_translation/tests/src/Functional/ContentTranslationSettingsTest.php
core/modules/content_translation/tests/src/Functional/ContentTranslationSyncImageTest.php
core/modules/content_translation/tests/src/Functional/ContentTranslationUISkipTest.php
core/modules/field/tests/src/Functional/EntityReference/EntityReferenceFieldTranslatedReferenceViewTest.php
core/modules/file/tests/src/Functional/FileOnTranslatedEntityTest.php
core/modules/file/tests/src/Functional/PrivateFileOnTranslatedEntityTest.php
core/modules/language/tests/src/Functional/EntityTypeWithoutLanguageFormTest.php
core/modules/language/tests/src/Functional/LanguageSelectorTranslatableTest.php
core/modules/menu_ui/tests/src/Functional/MenuUiContentTranslationTest.php
core/modules/node/tests/src/Functional/NodeTranslationUITest.php
core/modules/path/tests/src/Functional/PathLanguageTest.php
core/modules/views/tests/src/Functional/Plugin/DisplayFeedTranslationTest.php
core/modules/views/tests/src/Functional/SearchMultilingualTest.php
core/modules/workspaces/tests/src/Functional/PathWorkspacesTest.php
core/tests/Drupal/FunctionalTests/Routing/RouteCachingLanguageTest.php

The missing match at EntityReferenceFieldTranslatedReferenceViewTest is a disable translation case.
I have not yet figured out the right way TM to do it correctly via API, suggestions are welcomed.

Next steps are likely to continue wit the list above starting at FileOnTranslatedEntityTest.

The tests that make a form submission of that page as part of testing the actual form were identified and marked with a @covers annotation.

@joachim, thanks for mentioning that!
It was useful to be more confident on where to change things.
I only see now that you actually linked to #3355787: Identify which browser tests should be running the language settings form and add a @covers to document it on comment #13 above.

Next steps are likely to continue wit the list above starting at FileOnTranslatedEntityTest.

I think I got to the end of the list!

$ git grep -e 'admin/config/regional/content-language' --and -e 'drupalGet' --name-only 
core/modules/content_translation/tests/src/Functional/ContentTranslationDisableSettingTest.php
core/modules/content_translation/tests/src/Functional/ContentTranslationEnableTest.php
core/modules/content_translation/tests/src/Functional/ContentTranslationSettingsTest.php
core/modules/content_translation/tests/src/Functional/ContentTranslationSyncImageTest.php
core/modules/content_translation/tests/src/Functional/ContentTranslationUISkipTest.php
core/modules/language/tests/src/Functional/EntityTypeWithoutLanguageFormTest.php
core/modules/language/tests/src/Functional/LanguageSelectorTranslatableTest.php

The missing set of matches are relevant to keep for the following reasons.

• ContentTranslationDisableSettingTest, ContentTranslationEnableTest, ContentTranslationSettingsTest, ContentTranslationUISkipTest: they are expected to use the UI, as identified on its covers annotation, added at #3355787: Identify which browser tests should be running the language settings form and add a @covers to document it.
• EntityTypeWithoutLanguageFormTest and LanguageSelectorTranslatableTest are actually the same case, but they were not annotated. The first is checking hiding behavior of the form, and the second is checking the behavior of the setting page translated and when the content_translation module is not installed.
• ContentTranslationSyncImageTest is using the form to verify if translation synchronization has been saved correctly, and it is actually testing that feature, which is implemented on content_translation_form_language_content_settings_submit(), extending that is setup from \_content_translation_form_language_content_settings_form_alter, so I added a covers annotation instead, following the pattern from other cases like this.

Changes from the original point are not few, but highly related, so it likely made sense to follow @xjm suggestion from comment #12.

$ git diff --shortstat 6dfc925033..HEAD
 26 files changed, 244 insertions(+), 271 deletions(-)

Locally I have seen some performance improvements on most of the changed tests.
I did not made a fully performance review, but I have seen 3s improvements on multiple tests, which is great.

New commits on 3384936-use-lang-api-instead-of-form topic branch and related MR #5485 are the following.

- 8a461eeb03 Add a way to disable a given entity bundle translation from the language test trait
- 70d76a317c Use language translation test trait on ContentTranslationOperationsTest
- 08b4010835 Use disableBundleTranslation on EntityReferenceFieldTranslatedReferenceViewTest
- 0b1a618951 Use content translation test trait on FileOnTranslatedEntityTest
- b006797e17 Use content translation test trait on PrivateFileOnTranslatedEntityTest
- 4c178eff02 Use language test trait on LanguageSelectorTranslatableTest
- 091254e831 Use language test trait on MenuUiContentTranslationTest
- 8b6f4ecfe1 Use language test trait on NodeTranslationUITest
- 63124ebc88 Skip UI on node article translation setup for NodeTranslationUITest
- 18b99dbda8 Use content translation test trait on PathLanguageTest
- 283a4a7099 Use content translation test trait on DisplayFeedTranslationTest
- b56db49d11 Use language test trait on SearchMultilingualTest
- 3972484183 Use content translation test trait on PathWorkspacesTest
- 5bb538b3dd Use content translation test trait on RouteCachingLanguageTest
- f3cb974759 Use content translation test trait on ContentTranslationContextualLinksTest
- 2f650a931e Add covers pointing to content language settings form on EntityTypeWithoutLanguageFormTest
- 29faaea3f6 Add covers pointing to content language settings form on LanguageSelectorTranslatableTest
- bda937f13b Add covers pointing to relevant form alter on ContentTranslationSyncImageTest

At this point, this is ready for review, marking it accordingly on the issue status.

Not fully reviewed but proposed solution mentions changing API but API changes section is empty.

Also believe with an API change a change record needed to be included.

Not fully reviewed but proposed solution mentions changing API but API changes section is empty.

@smustgrave, indeed the IS really needed an update, now changed.

Also believe with an API change a change record needed to be included.

Right, I have just created a related change record at New traits to modifying language configuration during testing.

Thanks! CR reads fantastic

Refactor of the tests seem to have broken anything so think this should be good to go.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

I manually tried to merge from mainline, i.e. 11.x, into the topic branch, no merge conflicts.

I also tried the other way around, merging the topic branch into mainline, and no merge conflict there either.

I am moving the issue back to its previous state, and the "no-needs-review-bot" tag to avoid the bot.

Adding an extra tag, given the indirect improvement I noticed locally over the time to run for multiple of the changed tests.

Saving reviewer credits.

Great work and research on this.

I made some changes to the CR to better describe when the traits should be used. (I also removed the verbose method list since the example code explains it better anyway.)

Tagging for FM review of the proposed API design.

Meanwhile, I posted a number of minor things to fix on the MR.

Thanks again!

@xjm, thanks for the thorough review 👍
I appreciate a lot that you used the suggestions feature in gitlab for the set of direct syntactic/semantic suggestion.

I see the related change mentioned about strict typing in trait at b70dfff449a278e2248f42dc5bd6623093c41e0e.
It is nice to see that pattern starting to make its way into core.

I have modified the topic branch to have clear type hints, along with declaring strict typing on the new traits.
I also added a few small changes here and there, trying to use a bit more the added traits.

I added a follow-up about the to-do item in the added code at #3408046: API to disable bundle translation in one operation.

The following commits were added.

- 1f6db0a809 Add some extra type hints
- 64f7f64292 Improve syntax on multiple new docblocks
- e7d677b53c Improve two docblock parameter type docs
- 56a07e912b Get strict types declaration on traits from mainline
- 490f463719 Fix a couple LanguageTestTrait return values
- c6c4bf7d03 Use strict types on the new test traits
- 4e745b61fb Use static keyword to call new traits static methods
- 30a347274d Use create language trait method a bit more
- 5ed5cc94db Unify a couple of comments
- a7f5cbaeaf fix typo
- 05bc830064 Link follow-up on one-operation entity language disable

Back to NR now.

Hard to quantify, but this may be knocking as much as thirty seconds off NodeTranslationUiTest, which is also one of the very slowest functional tests:

https://git.drupalcode.org/project/drupal/-/jobs/561732

vs.

https://git.drupalcode.org/issue/drupal-3384936/-/jobs/471819

Bumping to major.

Also looks like we could make NodeTranslationUiTest use the testing profile - is there already an issue for that?

Brought this up in #needs-review-queue-initative channel and @catch said the approach seems good enough for an outside reivew. Going to leave tag for now.

Applying the MR and searched for $this->drupalGet('admin/config/regional/content-language and found 12 instances now. But reviewing those they appear to be checking settings and the form itself, so valid.

This looks great. Makes things a lot more concise as well as the speed-up. Committed/pushed to 11.x and cherry-picked to 10.2.x, thanks!

Publish the CR