Fix test performance of Drupal\node\Tests\NodeTranslationUITest

Rerolled it

Adding issue summary and re-parenting.

I used the suggestion from this issue's summary to create this patch. (I basically removed the default theme, so this code would use the test's theme).

I also reverted the change on run-tests.sh as those changes would not be committed to core.

The fail is possibly unrelated (as it passed before). The bot will retest it.

Reading the description this appears to be updating the install profile.

#23 appears to be removing the theme.

Updated test cases since standard isn't being used.

Can't seem to get around this one issue if anyone could help

There is 1 Add new comment link, /node/1#comment-form, for the en translation of a node on the frontpage. (Found 0.)
Failed asserting that 0 is identical to 1. /var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:121
 /var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/IsIdentical.php:90
 /var/www/html/web/core/modules/node/tests/src/Functional/NodeTranslationUITest.php:383
 /var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:726

Adding a new test module article_test to avoid using olivero_test for an article content type with all the fields.

I removed some of the cache tags as those modules aren't being tested.so removed them.

Still can't figure out

There is 1 Add new comment link, /node/1#comment-form, for the en translation of a node on the frontpage. (Found 0.)
Failed asserting that 0 is identical to 1. /var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:121
 /var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/IsIdentical.php:90
 /var/www/html/web/core/modules/node/tests/src/Functional/NodeTranslationUITest.php:383
 /var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:726

Figured it out! Standard was adding some permissions to the authenticated role that was missing.

bumping as this will unblock others.

> Adding a new test module article_test to avoid using olivero_test for an article content type with all the fields.

That seems like a LOT of code to add for just this test.

AFAICT the article_test module duplicates the article content type that the standard profile installs. Which means that this adds a maintenance burden, as the article_test module will need to be kept in sync with future changes to the profile.

If we're needing to test articles, then why not just test the standard profile?

And if we're needing to test something about having a content type with fields, can we do that more simply?

The goal is to not use the full standard install.

There are a number of tests that rely on the article content type with those fields. Figured it would be easier to have them in one place vs having to manually create the content type, fields, form display, etc each time.

It's a shame we don't yet have profile inheritance :(

What about making a testing profile that's just the article type? There are already profiles called testing_foo in core/profiles.

The advantages would be:

- the files in testing_article would be direct copies of the relevant ones in standard profile. It's therefore easier to keep them in sync
- someone changing the standard profile in future is a bit more likely to notice there is a testing_article profile that they need to update as well, since it's in the same part of the folder structure

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.

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

Note that #3384936: Use the API to set up languages in tests that are not specifically testing the language form 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.

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!