Fix test performance of Drupal\node\Tests\NodeTranslationUITest

The Needs Review Queue Bot → tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

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

smustgrave → closed merge request !5600

Bumping to major - this is one of the slowest Functional tests we have.

Note that 📌 Use the API to set up languages in tests that are not specifically testing the language form Fixed is also open but complementary to this issue.

Trying a new approach here where BrowserTestBase::setup(); is called instead of parent::setUp() in the setUp method.

smustgrave → changed the visibility of the branch 2254189-nodeTranslationUiTest to hidden.

So I tried fixing the merge conflict in 5601 but got 100s of merge conflicts. So I just started a new one with the exact changes from 5601.

Testing locally I see tests still pass so think @quietone's solution works!

Just saw @joachim comment, should investigate.

Testing locally without xdebug the test took 3m 33s to run and with this change it was 1m 33s.

Wow I didn't think it would be much of a difference.

But thanks for checking the comment!

I also wanted to see what this would look like without the phstan-ignore line. For now, I'll upload a patch for the work I did. All the tests changed in the patch pass locally. Is it worth doing that to avoid the ignore line?

Only meant to trigger once but page froze

I fixed the error.

But what do you think of the approach?

I made a new branch for the new approach, attempt3.

That looks like a good approach but a naming nitpick - could we call the new helper doSetUp()? Just because it more closely matches naming patterns with similar methods that are always called from the same method.

For the renaming

Re #58: I agree that doSetup is a better name. I have made that change.

I'd thought I'd have time today to get to it but got sidetracked :(

Rename looks good.

Committed/pushed to 11.x and cherry-picked to 10.2.x, thanks!

quietone → closed merge request !6092

quietone → closed merge request !6086

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