WordPress.org

Make WordPress Core

Opened 3 months ago

Last modified 3 months ago

#52607 new defect (bug)

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: needs-unit-tests 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 3 months ago.
52607.2.diff (1.5 KB) - added by SergeyBiryukov 3 months ago.

Download all attachments as: .zip

Change History (7)

#1 @SergeyBiryukov
3 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
3 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 @johnbillion
3 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
3 months ago

  • Milestone changed from Awaiting Review to 5.8

#5 @SergeyBiryukov
3 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.

Note: See TracTickets for help on using tickets.