feat: clear parallel constraints and limiter during test registration - #6897
Conversation
Adds TestRegisteredContext.ClearParallelConstraints() and ClearParallelLimiter() so an ITestRegisteredEventReceiver can remove the constraints added by [NotInParallel]/[ParallelGroup] and the limiter set by [ParallelLimiter<T>] or SetParallelLimiter. A limiter set after the clear still applies. Closes #6892
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (8)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughTestRegisteredContext now provides methods to clear parallel constraints and the parallel limiter during registration. Regression tests cover cleared parallelization state and verify that a later receiver can set a limiter. ChangesParallelization clearing
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~12 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant TestRegistrationPipeline
participant ClearingReceiver
participant TestRegisteredContext
participant TestContext
participant LaterReceiver
TestRegistrationPipeline->>ClearingReceiver: invoke registration receiver
ClearingReceiver->>TestRegisteredContext: ClearParallelConstraints()
TestRegisteredContext->>TestContext: clear stored constraints
ClearingReceiver->>TestRegisteredContext: ClearParallelLimiter()
TestRegistrationPipeline->>LaterReceiver: invoke later registration receiver
LaterReceiver->>TestRegisteredContext: set parallel limiter
Merge Risk: ⚪ Minimal · up to The registration-time clearing behavior has no identified issue requiring resolution before merge. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change can make selected tests run concurrently when they previously could not. The reviewed path gives that choice to code registering its own tests, not to an external caller, and no security finding was established. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 21.43% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 14 functions across 4 files. (4 skipped: 4 unsupported.)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
🛠️ Fix failing CI checks 💡
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. A rabbit clears the limits in flight Comment |
|
Code ReviewReviewed the two new registration-time APIs on Correctness
Metadata parity
Tests & docs
No correctness, thread-safety, or convention issues found. This is a small, well-scoped, well-tested addition — nothing to change. 🤖 Generated with Claude Code |
Closes #6892
Summary
Adds two methods to
TestRegisteredContextso anITestRegisteredEventReceivercan remove the parallelization of a test:ClearParallelConstraints()removes every constraint on the test, for example those added by[NotInParallel]and[ParallelGroup].ClearParallelLimiter()removes the limiter, whether[ParallelLimiter<T>]orSetParallelLimiterset it. It also resets the explicit-attribute precedence, so a limiter set after the clear still applies.Constraint attributes add their constraints during discovery, which runs before any registration receiver.
[ParallelLimiter<T>]sets its limiter during registration withOrder => 0, so a receiver that clears it needs a higherOrder. The XML docs say this.The scheduler groups tests after registration from the live
TestContextstate, so no engine change is needed. Both metadata modes share this path.Tests
tests/TUnit.TestProject/Bugs/6892: two tests share a[NotInParallel]key and a limit of 1, and they finish only if they run at the same time (TCS rendezvous, same pattern as Running TestsCases produced by Arguments not in parallel with each other but parallel with any other test #5700). They fail without the clear. A second test checks that a limiter set after the clear is applied.tests/TUnit.Engine.Tests/ClearParallelizationTests: passes in source-generated and reflection modes. AOT mode runs only in CI.tests/TUnit.PublicAPIsnapshots updated for the two new public methods.Summary by CodeRabbit