-
-
Notifications
You must be signed in to change notification settings - Fork 108
feat: prevent side effects from non-IAsyncInitializer property getters during discovery #4054
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,132 @@ | ||||||||
| using TUnit.Core.Interfaces; | ||||||||
| using TUnit.TestProject.Attributes; | ||||||||
|
|
||||||||
| namespace TUnit.TestProject.Bugs._4049; | ||||||||
|
|
||||||||
| /// <summary> | ||||||||
| /// Regression test for issue #4049: Property getters should not be called during discovery | ||||||||
| /// for properties that don't return IAsyncInitializer. | ||||||||
| /// | ||||||||
| /// This simulates the WebApplicationFactory.Server scenario where accessing a property | ||||||||
| /// causes side effects (like starting a test server) before the object is configured. | ||||||||
| /// </summary> | ||||||||
| [EngineTest(ExpectedResult.Pass)] | ||||||||
| public class PropertyGetterSideEffectTests | ||||||||
| { | ||||||||
| private static int _sideEffectCount = 0; | ||||||||
|
|
||||||||
| /// <summary> | ||||||||
| /// Simulates a class like WebApplicationFactory where accessing certain properties | ||||||||
| /// triggers initialization (like building the test host). | ||||||||
| /// </summary> | ||||||||
| public class FixtureWithSideEffects : IAsyncInitializer | ||||||||
| { | ||||||||
| private object? _server; | ||||||||
|
|
||||||||
| /// <summary> | ||||||||
| /// Simulates WebApplicationFactory.Server - accessing this property has side effects. | ||||||||
| /// This property type is 'object', NOT IAsyncInitializer, so TUnit should NOT access it. | ||||||||
| /// </summary> | ||||||||
| public object Server | ||||||||
| { | ||||||||
| get | ||||||||
| { | ||||||||
| Interlocked.Increment(ref _sideEffectCount); | ||||||||
| Console.WriteLine($"Server getter called! Side effect count: {_sideEffectCount}"); | ||||||||
| _server ??= new object(); // Side effect: lazy initialization | ||||||||
| return _server; | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| public bool IsInitialized { get; private set; } | ||||||||
|
|
||||||||
| public Task InitializeAsync() | ||||||||
| { | ||||||||
| IsInitialized = true; | ||||||||
| Console.WriteLine("FixtureWithSideEffects.InitializeAsync called"); | ||||||||
| return Task.CompletedTask; | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| [Test] | ||||||||
| [ClassDataSource<FixtureWithSideEffects>] | ||||||||
| public async Task PropertyGetters_WithSideEffects_ShouldNotBeCalledDuringDiscovery(FixtureWithSideEffects fixture) | ||||||||
| { | ||||||||
| // Verify the fixture was properly initialized | ||||||||
| await Assert.That(fixture.IsInitialized).IsTrue(); | ||||||||
|
|
||||||||
| // The Server property getter should NOT have been called during discovery. | ||||||||
| // It should only be called if the test explicitly accesses it. | ||||||||
| // At this point, we haven't accessed Server, so count should be 0. | ||||||||
| await Assert.That(_sideEffectCount).IsEqualTo(0) | ||||||||
| .Because("TUnit should not access non-IAsyncInitializer property getters during discovery"); | ||||||||
|
|
||||||||
| // Now let's explicitly access the Server to verify it works | ||||||||
| var server = fixture.Server; | ||||||||
| await Assert.That(server).IsNotNull(); | ||||||||
| await Assert.That(_sideEffectCount).IsEqualTo(1); | ||||||||
|
|
||||||||
| Console.WriteLine($"Test passed: Server getter was only called when explicitly accessed (count: {_sideEffectCount})"); | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| /// <summary> | ||||||||
| /// Test to verify that nested IAsyncInitializer properties ARE still discovered | ||||||||
| /// when they are properly typed. | ||||||||
| /// </summary> | ||||||||
| [EngineTest(ExpectedResult.Pass)] | ||||||||
|
||||||||
| [EngineTest(ExpectedResult.Pass)] | |
| [EngineTest(ExpectedResult.Pass)] | |
| [NotInParallel] |
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.
Test classes use static counters but lack the NotInParallel attribute. If these tests run concurrently, the static fields _sideEffectCount, _parentInitCount, and _childInitCount could experience race conditions, leading to flaky test failures. Add [NotInParallel] attribute to both test classes to ensure sequential execution.