WordPress.org

Make WordPress Core

Opened 5 months ago

Closed 5 weeks ago

Last modified 5 weeks ago

#52607 closed defect (bug) (fixed)

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

Reported by: johnbillion Owned by:
Milestone: 5.8 Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: has-patch commit
Focuses: multisite Cc:

Description

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.

Attachments (2)

52607.diff (554 bytes) - added by SergeyBiryukov 5 months ago.
52607.2.diff (1.5 KB) - added by SergeyBiryukov 5 months ago.

Download all attachments as: .zip

Change History (14)

#1 @SergeyBiryukov
5 months ago

  • Keywords has-patch added

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.

#2 @SergeyBiryukov
5 months ago

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

#3 follow-up: @johnbillion
5 months ago

  • Keywords commit added

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.

#4 @SergeyBiryukov
5 months ago

  • Milestone changed from Awaiting Review to 5.8

#5 @SergeyBiryukov
5 months ago

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.

This ticket was mentioned in Slack in #core by chaion07. View the logs.


7 weeks ago

#7 @chaion07
7 weeks ago

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!

This ticket was mentioned in Slack in #core-test by hellofromtonya. View the logs.


5 weeks ago

#9 @hellofromTonya
5 weeks ago

  • Keywords needs-unit-tests removed

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.

#10 in reply to: ↑ 3 @hellofromTonya
5 weeks ago

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.

#11 @hellofromTonya
5 weeks ago

  • Resolution set to fixed
  • Status changed from new to closed

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

#12 @hellofromTonya
5 weeks ago

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

Note: See TracTickets for help on using tickets.