fix(tests): restore null screen state with unset in Dashboard_Widgets_Test finally blocks#849
Conversation
…_Test finally blocks When no screen existed before a test, $original_screen_id is null. The old guard 'if ($original_screen_id)' is falsy for null, so the forced dashboard-network / dashboard screen leaked into subsequent tests. Fix: use 'if (null !== $original_screen_id)' and add an else branch that calls unset($GLOBALS['current_screen']) to restore the pre-test null state. Applies to both finally blocks (lines 165-167, 212-214). This is the WordPress-recommended approach for reverting to a null screen state in test environments (see wordpress-develop abstract-testcase.php). Resolves #835
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 56 minutes and 34 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
SummaryFixes leaking screen state in
Changes
VerificationThe fix matches the pattern used in WordPress core test infrastructure. The change prevents screen state leakage when tests run in isolation (no prior screen context). Merged via PR #849 to main. aidevops.sh v3.8.24 spent 13m on this as a headless bash routine. |
🔨 Build Complete - Ready for Testing!📦 Download Build Artifact (Recommended)Download the zip build, upload to WordPress and test:
🌐 Test in WordPress Playground (Very Experimental)Click the link below to instantly test this PR in your browser - no installation needed! Login credentials: |
Merge SummaryIssue: #835 — Review followup: PR #821 — t806: test: harden Dashboard_Widgets_Test global cleanup with try/finally What was done: The CodeRabbit bot flagged that both Fix applied (both
This matches the WordPress-recommended pattern from Outcome: B — Premise correct, fix implemented and merged. Verification: PHP Lint + Code Quality checks passed. Cypress E2E failures are pre-existing environment setup failures unrelated to this test-file change. |
|
Performance Test Results Performance test results for abc8ca0 are in 🛎️! Note: the numbers in parentheses show the difference to the previous (baseline) test run. Differences below 2% or 0.5 in absolute values are not shown. URL:
|
1 participant
Summary
Fixes leaking screen state in
Dashboard_Widgets_Testwhen no screen existed before test execution.Both
finallyblocks usedif ($original_screen_id)as the guard before callingset_current_screen(). When$original_screen_idisnull(no screen was active before the test), the guard evaluates as falsy and the forceddashboard-network/dashboardscreen leaks into subsequent tests.Fix: replace
if ($original_screen_id)withif (null !== $original_screen_id)and add anelsebranch that callsunset($GLOBALS['current_screen'])— the WordPress-recommended way to restore a previously-null screen state (seewordpress-develop/tests/phpunit/includes/abstract-testcase.php).Applies to both test methods:
test_enqueue_scripts_enqueues_activity_stream_on_index(line 165-167)test_enqueue_scripts_skips_activity_stream_on_per_site_dashboard(line 212-214)Changes
tests/WP_Ultimo/Dashboard_Widgets_Test.php— twofinallyblocks updatedVerification
The fix matches the pattern used in WordPress core test infrastructure. The change prevents screen state leakage when tests run in isolation (no prior screen context).
Resolves #835