Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions TUnit.Core/TestContext.cs
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,12 @@ internal override void SetAsyncLocalContext()

internal bool RunOnTestDiscovery { get; set; }

/// <summary>
/// Indicates whether this test is reusing the discovery-time instance instead of creating a new instance.
/// When true, property resolution and initialization should be skipped since the instance is already prepared.
/// </summary>
internal bool IsDiscoveryInstanceReused { get; set; }

public object Lock { get; } = new();


Expand Down
3 changes: 2 additions & 1 deletion TUnit.Engine/Building/Interfaces/ITestBuilder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,9 @@ internal interface ITestBuilder
/// <param name="metadata">The test metadata</param>
/// <param name="testData">The test data</param>
/// <param name="testBuilderContext"></param>
/// <param name="isReusingDiscoveryInstance">Whether this test is reusing the discovery instance</param>
/// <returns>An executable test ready for execution</returns>
Task<AbstractExecutableTest> BuildTestAsync(TestMetadata metadata, TestBuilder.TestData testData, TestBuilderContext testBuilderContext);
Task<AbstractExecutableTest> BuildTestAsync(TestMetadata metadata, TestBuilder.TestData testData, TestBuilderContext testBuilderContext, bool isReusingDiscoveryInstance = false);

/// <summary>
/// Builds all executable tests from a single TestMetadata using its DataCombinationGenerator delegate.
Expand Down
19 changes: 17 additions & 2 deletions TUnit.Engine/Building/TestBuilder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,7 @@ public async Task<IEnumerable<AbstractExecutableTest>> BuildTestsFromMetadataAsy
var needsInstanceForMethodDataSources = metadata.DataSources.Any(ds => ds is IAccessesInstanceData);

object? instanceForMethodDataSources = null;
var discoveryInstanceUsed = false;

if (needsInstanceForMethodDataSources)
{
Expand Down Expand Up @@ -382,10 +383,21 @@ await _objectLifecycleService.RegisterObjectAsync(
var basicSkipReason = GetBasicSkipReason(metadata, attributes);

Func<Task<object>> instanceFactory;
bool isReusingDiscoveryInstance = false;

if (basicSkipReason is { Length: > 0 })
{
instanceFactory = () => Task.FromResult<object>(SkippedTestInstance.Instance);
}
else if (methodDataLoopIndex == 1 && i == 0 && instanceForMethodDataSources != null && !discoveryInstanceUsed)
{
// Reuse the discovery instance for the first test to avoid duplicate initialization
Copy link

Copilot AI Dec 6, 2025

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.

Suggested change
// 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.

Copilot uses AI. Check for mistakes.
var capturedInstance = instanceForMethodDataSources;
discoveryInstanceUsed = true;
isReusingDiscoveryInstance = true;

instanceFactory = () => Task.FromResult(capturedInstance);
}
else
{
var capturedMetadata = metadata;
Expand Down Expand Up @@ -420,7 +432,7 @@ await _objectLifecycleService.RegisterObjectAsync(
InitializedAttributes = attributes
};

var test = await BuildTestAsync(metadata, testData, testSpecificContext);
var test = await BuildTestAsync(metadata, testData, testSpecificContext, isReusingDiscoveryInstance);

// If we have a basic skip reason, set it immediately
if (!string.IsNullOrEmpty(basicSkipReason))
Expand Down Expand Up @@ -799,7 +811,7 @@ private async Task<IDataSourceAttribute[]> GetDataSourcesAsync(IDataSourceAttrib

[UnconditionalSuppressMessage("Trimming", "IL2026", Justification = "Hook discovery service handles mode-specific logic; reflection calls suppressed in AOT mode")]
[UnconditionalSuppressMessage("AOT", "IL3050", Justification = "Hook discovery service handles mode-specific logic; dynamic code suppressed in AOT mode")]
public async Task<AbstractExecutableTest> BuildTestAsync(TestMetadata metadata, TestData testData, TestBuilderContext testBuilderContext)
public async Task<AbstractExecutableTest> BuildTestAsync(TestMetadata metadata, TestData testData, TestBuilderContext testBuilderContext, bool isReusingDiscoveryInstance = false)
{
// Discover instance hooks for closed generic types (no-op in source gen mode)
if (metadata.TestClassType is { IsGenericType: true, IsGenericTypeDefinition: false })
Expand All @@ -811,6 +823,9 @@ public async Task<AbstractExecutableTest> BuildTestAsync(TestMetadata metadata,

var context = await CreateTestContextAsync(testId, metadata, testData, testBuilderContext);

// Mark if this test is reusing the discovery instance (already initialized)
context.IsDiscoveryInstanceReused = isReusingDiscoveryInstance;

context.Metadata.TestDetails.ClassInstance = PlaceholderInstance.Instance;

// Arguments will be tracked by TestArgumentTrackingService during TestRegistered event
Expand Down
6 changes: 6 additions & 0 deletions TUnit.Engine/Services/PropertyInjector.cs
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,12 @@ public async Task ResolveAndCachePropertiesAsync(
TestContextEvents events,
TestContext testContext)
{
// Skip property resolution if this test is reusing the discovery instance (already initialized)
if (testContext.IsDiscoveryInstanceReused)
{
return;
}

var plan = PropertyInjectionCache.GetOrCreatePlan(testClassType);

if (!plan.HasProperties)
Expand Down
62 changes: 62 additions & 0 deletions TUnit.TestProject/Bugs/3993/IAsyncInitializerTests.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
using System.Collections.Concurrent;
using TUnit.Core.Interfaces;
using TUnit.TestProject.Attributes;

namespace TUnit.TestProject.Bugs._3993;

/// <summary>
/// Regression test for issue #3993: IAsyncInitializer called 4 times instead of 3
/// when using ClassDataSource with MethodDataSource that accesses instance properties.
/// </summary>
[EngineTest(ExpectedResult.Pass)]
public class IAsyncInitializerTests
{
[ClassDataSource<PerTestCaseSource>(Shared = SharedType.None)]
public required PerTestCaseSource PerTestCaseSource { get; init; }

public IEnumerable<string> PerTestCaseStrings() => PerTestCaseSource.MyString;

[Test]
[MethodDataSource(nameof(PerTestCaseStrings))]
public async Task ForNTestCases_ShouldInitNTimes(string s)
{
await Assert.That(s).IsNotNull();

// 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)
Comment on lines +25 to +28
Copy link

Copilot AI Dec 6, 2025

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.

Suggested change
// 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).

Copilot uses AI. Check for mistakes.
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()
{
Comment on lines +29 to +39
Copy link

Copilot AI Dec 6, 2025

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:

  1. Test 1 initializes (count becomes 1), then immediately asserts count == 3 → FAIL
  2. Test 2 initializes (count becomes 2), then asserts count == 3 → FAIL
  3. 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
Suggested change
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.");

Copilot uses AI. Check for mistakes.
// Reset for next run
PerTestCaseSource.Reset();
}
}

public class PerTestCaseSource : IAsyncInitializer
{
public string[] MyString = [];
private static int _setUps = 0;
public static int SetUps => _setUps;

public Task InitializeAsync()
{
Interlocked.Increment(ref _setUps);
Copy link

Copilot AI Dec 6, 2025

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.

Copilot uses AI. Check for mistakes.
MyString = ["123", "321", "9210"];
return Task.CompletedTask;
}

public static void Reset()
{
_setUps = 0;
}
}
Loading