t806: test: harden Dashboard_Widgets_Test global cleanup with try/finally#821
t806: test: harden Dashboard_Widgets_Test global cleanup with try/finally#821superdav42 merged 1 commit intomainfrom
Conversation
Addresses CodeRabbit review feedback from PR #801: - Capture $original_done and screen state before mutating globals - Wrap test body in try/finally so globals always restored on failure - Restore $wp_scripts->done and screen context in finally block Applies to both test_enqueue_scripts_enqueues_activity_stream_on_index and test_enqueue_scripts_skips_activity_stream_on_per_site_dashboard to prevent cross-test leakage when assertions throw. Fixes #806
|
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 53 minutes and 44 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 (2)
📝 WalkthroughWalkthroughThis PR improves test state isolation in Dashboard_Widgets_Test.php by refactoring two test methods to capture and restore WordPress global state more comprehensively. The tests now snapshot Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
🔨 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: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/WP_Ultimo/Dashboard_Widgets_Test.php`:
- Around line 130-131: The finally blocks that restore the admin screen state
must also clear the global when there was no original screen; update the restore
logic around $original_screen and $original_screen_id so that if
$original_screen_id is truthy you restore the original screen (e.g., via
set_current_screen or assigning $GLOBALS['current_screen'] back to
$original_screen), and otherwise explicitly unset($GLOBALS['current_screen']) to
prevent the forced screen from leaking into other tests; apply this change in
both test methods that capture $original_screen/$original_screen_id.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d6e5fb41-7422-4b44-85dd-0e3b0a9d0487
📒 Files selected for processing (1)
tests/WP_Ultimo/Dashboard_Widgets_Test.php
| $original_screen = function_exists('get_current_screen') ? get_current_screen() : null; | ||
| $original_screen_id = $original_screen ? $original_screen->id : null; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In WordPress test environments, after calling set_current_screen('dashboard') or set_current_screen('dashboard-network'), what is the recommended way to restore a previously-null screen state: unset($GLOBALS['current_screen']) or set_current_screen('')?
💡 Result:
In WordPress test environments, the recommended way to restore a previously-null screen state after calling set_current_screen('dashboard') or set_current_screen('dashboard-network') is unset($GLOBALS['current_screen']) (along with related globals $taxnow and $typenow).
Citations:
- 1: https://lists.automattic.com/pipermail/wp-trac/2021-June/450129.html
- 2: 53431: moves reset of current screen globals to base test case WordPress/wordpress-develop#1380
- 3: https://github.com/WordPress/wordpress-develop/blob/73e822f2/tests/phpunit/includes/abstract-testcase.php
🏁 Script executed:
cat -n tests/WP_Ultimo/Dashboard_Widgets_Test.php | sed -n '125,220p'Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 3808
Unset $GLOBALS['current_screen'] when restoring to null state.
Both finally blocks skip restoration when $original_screen_id is null/falsy, allowing forced screen states to leak into subsequent tests. The finally blocks must explicitly unset the screen global when the original screen did not exist.
Proposed fix
} finally {
if (isset($wp_scripts)) {
$wp_scripts->queue = $original_queue;
$wp_scripts->done = $original_done;
}
- if ($original_screen_id) {
- set_current_screen($original_screen_id);
- }
+ if (null !== $original_screen_id) {
+ set_current_screen($original_screen_id);
+ } else {
+ unset($GLOBALS['current_screen']);
+ }
$pagenow = $original;
}Applies to both test methods (lines 165–167 and 212–214).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $original_screen = function_exists('get_current_screen') ? get_current_screen() : null; | |
| $original_screen_id = $original_screen ? $original_screen->id : null; | |
| } finally { | |
| if (isset($wp_scripts)) { | |
| $wp_scripts->queue = $original_queue; | |
| $wp_scripts->done = $original_done; | |
| } | |
| if (null !== $original_screen_id) { | |
| set_current_screen($original_screen_id); | |
| } else { | |
| unset($GLOBALS['current_screen']); | |
| } | |
| $pagenow = $original; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/WP_Ultimo/Dashboard_Widgets_Test.php` around lines 130 - 131, The
finally blocks that restore the admin screen state must also clear the global
when there was no original screen; update the restore logic around
$original_screen and $original_screen_id so that if $original_screen_id is
truthy you restore the original screen (e.g., via set_current_screen or
assigning $GLOBALS['current_screen'] back to $original_screen), and otherwise
explicitly unset($GLOBALS['current_screen']) to prevent the forced screen from
leaking into other tests; apply this change in both test methods that capture
$original_screen/$original_screen_id.
🔨 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: |
|
Performance Test Results Performance test results for 6a1bf6f 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:
|
superdav42
force-pushed
the
feature/auto-20260413-120244
branch
from
af4de8a to
9327bbe
Compare
MERGE_SUMMARYIssue: #806 — Review followup: PR #801 — test: add template_id checkout validation tests and fix Dashboard_Widgets_Test What was doneAddressed the unaddressed CodeRabbit review feedback from PR #801: Problem: Two dashboard widget tests ( and ) mutated Fix applied to
TestingAll required CI checks passed (PHP 8.2, 8.3, 8.4, 8.5; Cypress 8.2 chrome). |
🔨 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: |
1 participant
Summary
Addresses the unaddressed CodeRabbit review feedback from PR #801 (issue #806).
Problem: Two dashboard widget tests mutate
$wp_scripts->done,$pagenow, and screen context but only restore$wp_scripts->queueand$pagenow— and only in the happy path. If an assertion throws, subsequent tests can inherit mutated globals/screen state causing hard-to-diagnose flicker failures.Fix: For both
test_enqueue_scripts_enqueues_activity_stream_on_indexandtest_enqueue_scripts_skips_activity_stream_on_per_site_dashboard:$original_doneand$original_screen_idbefore mutating statetry/finallyso cleanup runs even when assertions throw$wp_scripts->queue,$wp_scripts->done, screen context, and$pagenowinfinallyFiles changed
tests/WP_Ultimo/Dashboard_Widgets_Test.php— lines 123–170 and 179–217Verification
Tests still pass with the same assertions; cleanup is now guaranteed on failure paths.
Resolves #806
Summary by CodeRabbit