[thomhurst]

Summary

ClassDataSourceSharedNoneNoLeakageTests.Test1/2/3_GetsUniqueInstance asserted InstanceId == 1/2/3 from a static counter. When the instances are built relative to the tests that run them is not fixed, so the tests failed intermittently.

Each test now records the ID of the instance it received in a static dictionary. It fails if another test already received that ID (a shared instance) or if MutationValue is non-zero (leaked mutation). All three tests now mutate the instance. I removed the [Before(TestSession)] counter reset. It is not needed now, and a reset that ran after an instance was built could have produced duplicate IDs.

Validation

• dotnet run -c Release -f net10.0 -- --treenode-filter "/*/*/ClassDataSourceSharedNone*/*", 4 runs in source-generated mode and 4 with --reflection: 5/5 passed in every run.
• Negative check: I temporarily switched the three tests to SharedType.PerClass, and the new assertions failed with Expected to be equal to "Test1_GetsUniqueInstance".

Closes #7250

Summary by CodeRabbit

• Tests
  • Updated regression coverage to verify that instances begin with a clean state and are not shared across tests, regardless of construction order.
  • Removed checks that depended on specific instance IDs or a fixed sequence of test execution.
  • These changes affect test validation only; no end-user behavior changes are included.

[coderabbitai]

No actionable comments were generated in the recent review.

Recent review info

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 24c6681c-b46e-478c-a95d-a2633f94cb6e

Commits

Reviewing files that changed from the base of the PR and between db321c9 and 9bc856f.

Files selected for processing: 1

• tests/TUnit.TestProject/ClassDataSourceSharedNoneRegressionTests.cs

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 0 remain after this review.

Walkthrough

The regression tests no longer assume a fixed instance construction order. They record which test receives each instance ID and check that each instance has an initial mutation value of zero.

Changes: Instance isolation regression tests

Layer / File(s)	Summary
Replace fixed ID assertions with ownership checks tests/TUnit.TestProject/ClassDataSourceSharedNoneRegressionTests.cs	The tests remove the ID-counter reset and session hook. They track instance ID ownership with a concurrent dictionary, check that each instance starts with a mutation value of zero, and retain distinct mutation values for each test. The comments now state that construction order is not fixed.

Estimated code review effort: 2 (Simple) | ~8 minutes

Change: Other · Severity of issue fixed: Low

Merge Risk: Minimal · up to 9bc85

No merge-blocking risk is established by the supplied evidence.

Pre-merge checks: 5

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely describes the main change: making SharedType.None leakage tests independent of build order.
Linked Issues check		Issue #7250 requires the three SharedType.None tests to verify unique, unmutated instances without relying on construction order. The reviewed code records each InstanceId in ConcurrentDictionary, rej…
Out of Scope Changes check		The changes are limited to the regression tests and their test helper state in ClassDataSourceSharedNoneRegressionTests.cs. The ConcurrentDictionary, mutation checks, ID tracking, and removal of the o…
Docstring Coverage		No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…

Full details: Linked Issues check

Explanation

Issue #7250 requires the three SharedType.None tests to verify unique, unmutated instances without relying on construction order. The reviewed code records each InstanceId in ConcurrentDictionary, rejects an ID owned by another test, checks MutationValue is zero, and mutates each instance after the checks. It removes the fixed 1/2/3 assertions and the counter reset. These changes satisfy the linked coding requirements.

Full details: Out of Scope Changes check

Explanation

The changes are limited to the regression tests and their test helper state in ClassDataSourceSharedNoneRegressionTests.cs. The ConcurrentDictionary, mutation checks, ID tracking, and removal of the obsolete reset support the requirements in issue #7250. No unrelated implementation change is present in the reviewed diff.

Full details: Docstring Coverage

Explanation

No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 1 files.

Finishing Touches

Generate docstrings

• Commit to this branch
• Create a new PR

Generate unit tests (beta)

• Commit to this branch
• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

Share

• X
• Mastodon
• Reddit
• LinkedIn

_{A rabbit checks each ID with care
No fixed order waits among them there
Each instance starts at zero
Then takes its test’s own value
The counters rest; the checks stay fair
And flaky runs hop out of sight}

[thomhurst]

@codex review

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-11-02T07:11:50.692020Z	9bc856f	Draft marked ready

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[github-actions]

Review

This is a sound fix for the flaky test. The old tests assumed a fixed ID order (1/2/3) from a static counter, which depends on when instances are built. Checking that no ID is received by two different tests, plus a zero MutationValue, tests the actual property: no sharing and no leakage. Removing the [Before(TestSession)] counter reset also removes a race where a reset could run after an instance was built.

Observations (none blocking):

• The negative check in the PR description (switching to PerClass makes the tests fail) shows the new assertions still catch a regression.
• GetOrAdd keyed by test name is safe under retries or repeats, because each attempt builds a new instance with a new ID.
• One weakness: a test that runs twice (for example a retry) and receives the same shared instance both times would pass. The tests don't use retries, so this doesn't matter here.

LGTM.

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Another round soon, please!

Reviewed commit: 9bc856f385

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[github-actions]

Review

This fixes the flaky test (#7250) properly. The old tests assumed a fixed construction order (InstanceId == 1/2/3), which the engine doesn't guarantee. The new tests check the actual property: no two tests get the same instance, and no instance carries another test's mutation.

• Keying ReceivedInstanceIds by instance ID and comparing the owner's test name is sound. A retry of the same test passes, and a shared instance fails.
• Removing the [Before(TestSession)] counter reset is right. A reset that ran after an instance was built could have produced duplicate IDs.
• Having all three tests mutate the instance means a leak is caught whichever test runs second or third. Your negative check with SharedType.PerClass shows the assertions fail when they should.

Minor, non-blocking:

• The MutationValue == 0 check can only detect a leak if the shared instance has already been mutated when the later test reads it. With parallel execution the ID check covers that case, so the two checks complement each other and coverage is adequate.
• The ConcurrentDictionary is never cleared. That's harmless because each test name is only used by its own test.

LGTM.

[greptile-apps]

Confidence Score: 5/5

[Low impact] The PR appears safe to merge.

Summary

The PR makes the leakage tests independent of construction order.

• SharedType.None leakage tests check their instances without relying on build order.

_{Reviews (1) · Last reviewed commit: "test: assert distinct SharedType.None in..." · Reviewed by Greptile}