Investigate why the ms-excluded test group fails when run on its own

[johnbillion]

When the ms-excluded test group is run on its own it fails with one test failure.

composer test -- --group ms-excluded

There was 1 failure:

1) Tests_AdminBar::test_admin_bar_contains_correct_links_for_users_with_role
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'http://example.org/wp-admin/'
+'http://example.org/'

The test does not fail when run individually or within the whole suite, only when the ms-excluded group is run.

[SergeyBiryukov]

This appears to be caused by set_current_screen( 'dashboard' ) in test_submenu_helpers_position(), which then causes the is_admin() check in wp_admin_bar_site_menu() to return true and switch the link to "Visit Site" instead of "Dashboard".

I thought the $current_screen global would be reset between tests, but that is not the case, so apparently each test is responsible for cleaning this up after using set_current_screen().

52607.diff​ is based on a similar routine from Tests_Admin_includesScreen, or perhaps each of the four tests in Tests_Admin_includesPlugin that use set_current_screen() should clean this up separately.

[SergeyBiryukov]

It looks like some other tests use set_current_screen( 'front' ) to reset this global, so 52607.2.diff​ follows that approach.

[johnbillion]

52607.2.diff​ looks good to me and indeed is the approach used in other tests.

The current screen global should get reset between tests, needs investigating.

[SergeyBiryukov]

In 50433:

Tests: Reset current screen after setting it to dashboard in add_submenu_page() tests.

This avoids polluting other tests and allows the ms-excluded test group to successfully run on its own.

Props johnbillion, SergeyBiryukov.
See #52607.

[chaion07]

We reviewed this ticket during a recent [bug-scrub session]​https://wordpress.slack.com/archives/C02RQBWTW/p1623096926361400. With Beta 1 coming in a day, checking with @SergeyBiryukov to kindly draw some attention to this. Thank you!

[hellofromTonya]

Tested => resolves the reported issue.

Testing results

Removing the committed lines from [50433], tests fail:

npm run test:php -- --group ms-excluded

> WordPress@5.8.0 test:php ../wordpress-develop
> node ./tools/local-env/scripts/docker.js run --rm phpunit phpunit "--group" "ms-excluded"

Creating wordpress-develop_phpunit_run ... done
Installing...
Running as single site... To run multisite, use -c tests/phpunit/multisite.xml
Not running ajax tests. To execute these, use --group ajax.
Not running ms-files tests. To execute these, use --group ms-files.
Not running external-http tests. To execute these, use --group external-http.
PHPUnit 7.5.20 by Sebastian Bergmann and contributors.

.........F.......................                                 33 / 33 (100%)

Time: 6.51 seconds, Memory: 128.00 MB

There was 1 failure:

1) Tests_AdminBar::test_admin_bar_contains_correct_links_for_users_with_role
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'http://example.org/wp-admin/'
+'http://example.org/'

/var/www/tests/phpunit/tests/adminbar.php:128

FAILURES!
Tests: 33, Assertions: 201, Failures: 1.

After reapplying the committed changes, tests all pass:

npm run test:php -- --group ms-excluded

> WordPress@5.8.0 test:php ../wordpress-develop
> node ./tools/local-env/scripts/docker.js run --rm phpunit phpunit "--group" "ms-excluded"

Creating wordpress-develop_phpunit_run ... done
Installing...
Running as single site... To run multisite, use -c tests/phpunit/multisite.xml
Not running ajax tests. To execute these, use --group ajax.
Not running ms-files tests. To execute these, use --group ms-files.
Not running external-http tests. To execute these, use --group external-http.
PHPUnit 7.5.20 by Sebastian Bergmann and contributors.

.................................                                 33 / 33 (100%)

Time: 6.03 seconds, Memory: 128.00 MB

OK (33 tests, 204 assertions)

Test 2: Running full test suite

Repeated the above tests without the --group, i.e. running the full test suite.

Results: As expected, tests pass before and after (as the problem is isolated to a group).

Before the committed changeset:

Time: 2.67 minutes, Memory: 171.05 MB

OK, but incomplete, skipped, or risky tests!
Tests: 12420, Assertions: 57530, Skipped: 20.

After the committed changeset: Passed

Time: 2.65 minutes, Memory: 171.05 MB

OK, but incomplete, skipped, or risky tests!
Tests: 12420, Assertions: 57530, Skipped: 20.

[hellofromTonya]

Replying to SergeyBiryukov:

I thought the $current_screen global would be reset between tests, but that is not the case, so apparently each test is responsible for cleaning this up after using set_current_screen().

Replying to johnbillion:

The current screen global should get reset between tests, needs investigating.

Hmm,that's odd it isn't resetting this global state. I agree it should. I'll investigate and open a new ticket for this work.

[hellofromTonya]

Closing this ticket as the original reported issue is resolved. Moving global state reset to a new ticket for follow-up investigation.

[hellofromTonya]

Follow-up ticket opened #53431 for moving global state reset to test case.