-
-
Notifications
You must be signed in to change notification settings - Fork 105
feat: add support for reusing discovery instances in test building and property resolution #3998
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…nstances and add regression test for IAsyncInitializer calls
…d property resolution
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request implements an optimization to reuse discovery-time test class instances for the first test case, preventing unnecessary duplicate initialization of IAsyncInitializer objects. The fix addresses issue #3992 where instance initialization was occurring 4 times instead of 3 when using ClassDataSource with MethodDataSource that accesses instance properties.
Key Changes
- Discovery instance reuse: The test class instance created during test discovery (to evaluate method data sources that access instance properties) is now reused for the first test case, avoiding redundant initialization
- Property resolution optimization: Added early-return logic to skip property resolution when a test reuses the discovery instance
- Test context tracking: Introduced
IsDiscoveryInstanceReusedflag to track which tests are reusing discovery instances
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| TUnit.Core/TestContext.cs | Added IsDiscoveryInstanceReused property to track when a test reuses the discovery instance |
| TUnit.Engine/Building/Interfaces/ITestBuilder.cs | Extended BuildTestAsync interface with optional isReusingDiscoveryInstance parameter |
| TUnit.Engine/Building/TestBuilder.cs | Implemented logic to capture and reuse the discovery instance for the first test case in each class data iteration |
| TUnit.Engine/Services/PropertyInjector.cs | Added early-return optimization to skip property resolution when reusing discovery instances |
| TUnit.TestProject/Bugs/3993/IAsyncInitializerTests.cs | Added regression test to verify initialization count with instance reuse optimization |
| // Each of the 3 test cases should have its own isolated PerTestCaseSource instance | ||
| // Since Shared = SharedType.None, each test gets a new instance | ||
| // Therefore, InitializeAsync should be called exactly 3 times (once per test) | ||
| // NOT 4 times (which would include the discovery instance) |
Copilot
AI
Dec 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The comment states "Each of the 3 test cases should have its own isolated PerTestCaseSource instance", but with Shared = SharedType.None and the discovery instance reuse optimization, the first test case actually reuses the discovery instance rather than getting "its own" new instance. Consider updating the comment to clarify that the first test reuses the discovery instance while the other two get new isolated instances, or rephrase to focus on the initialization count rather than instance isolation.
| // Each of the 3 test cases should have its own isolated PerTestCaseSource instance | |
| // Since Shared = SharedType.None, each test gets a new instance | |
| // Therefore, InitializeAsync should be called exactly 3 times (once per test) | |
| // NOT 4 times (which would include the discovery instance) | |
| // There are 3 test cases, and InitializeAsync should be called exactly 3 times (once per test). | |
| // With Shared = SharedType.None, the first test case reuses the discovery instance, | |
| // while the other two get new isolated PerTestCaseSource instances. | |
| // The initialization count should be 3, not 4 (which would include an extra discovery instance). |
| } | ||
| else if (methodDataLoopIndex == 1 && i == 0 && instanceForMethodDataSources != null && !discoveryInstanceUsed) | ||
| { | ||
| // Reuse the discovery instance for the first test to avoid duplicate initialization |
Copilot
AI
Dec 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment "Reuse the discovery instance for the first test to avoid duplicate initialization" could be more precise. Consider clarifying that this specifically reuses the instance created for evaluating method data sources that access instance properties (via IAccessesInstanceData), and that it only applies to the very first test in the first method data iteration with no repeats.
| // Reuse the discovery instance for the first test to avoid duplicate initialization | |
| // Reuse the instance created for evaluating method data sources that access instance properties (via IAccessesInstanceData). | |
| // This reuse only applies to the very first test in the first method data iteration (methodDataLoopIndex == 1 && i == 0) with no repeats, | |
| // ensuring correct instance semantics and avoiding duplicate initialization for tests that depend on instance data during discovery. |
| await Assert | ||
| .That(PerTestCaseSource.SetUps) | ||
| .IsEqualTo(3) | ||
| .Because("each of the 3 test cases should be wrapped in its own isolated test class, " + | ||
| "causing a call to async init, but no more than those three calls should be needed. " + | ||
| "The discovery-time instance should NOT be initialized."); | ||
| } | ||
|
|
||
| [After(Class)] | ||
| public static void ResetCounter() | ||
| { |
Copilot
AI
Dec 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This assertion has a race condition when tests run in parallel (TUnit's default behavior). The static counter _setUps is incremented during each test's initialization, but the assertion checks for an exact value of 3. The timeline could be:
- Test 1 initializes (count becomes 1), then immediately asserts count == 3 → FAIL
- Test 2 initializes (count becomes 2), then asserts count == 3 → FAIL
- Test 3 initializes (count becomes 3), then asserts count == 3 → PASS
To fix this, either:
- Add
[NotInParallel]attribute to force sequential execution - Change assertion to check a range:
.IsGreaterThanOrEqualTo(1).And.IsLessThanOrEqualTo(3) - Move the assertion to an
[After(Class)]hook that runs after all tests complete - Use a non-static counter to track per-instance initialization
| await Assert | |
| .That(PerTestCaseSource.SetUps) | |
| .IsEqualTo(3) | |
| .Because("each of the 3 test cases should be wrapped in its own isolated test class, " + | |
| "causing a call to async init, but no more than those three calls should be needed. " + | |
| "The discovery-time instance should NOT be initialized."); | |
| } | |
| [After(Class)] | |
| public static void ResetCounter() | |
| { | |
| // Assertion about SetUps moved to [After(Class)] hook to avoid race condition. | |
| } | |
| [After(Class)] | |
| public static async Task AssertSetUpsAndResetAsync() | |
| { | |
| await Assert | |
| .That(PerTestCaseSource.SetUps) | |
| .IsEqualTo(3) | |
| .Because("each of the 3 test cases should be wrapped in its own isolated test class, " + | |
| "causing a call to async init, but no more than those three calls should be needed. " + | |
| "The discovery-time instance should NOT be initialized."); |
|
|
||
| public Task InitializeAsync() | ||
| { | ||
| Interlocked.Increment(ref _setUps); |
Copilot
AI
Dec 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Write to static field from instance method, property, or constructor.
Fixes #3992