[Avalonia.Headless.NUnit] Make SetUp and TearDown methods work when async
#18306
Conversation
|
Those test failures are not a good sign, but it's also 2 am. I will look into them shortly! |
|
If you set a breakpoint in the debugger at this line under the NUnit tests, the breakpoint does not get hit when on master. It looks like my changes definitely break some other things, in part because I didn't fully consider the consequences of removing the TearDown tests from the custom test context. I'm rethinking how I did this. |
|
Looks like the tests that were failing before are passing now that I rethought how I did this! |
|
You can test this PR using the following package version. |
|
You can test this PR using the following package version. |
|
You can test this PR using the following package version. |
| { | ||
| Action<TestExecutionContext> beforeAction = c => | ||
| { | ||
| before.AddRange(setUpMethods.Select<IMethodInfo, Action>(m => async void () => |
There was a problem hiding this comment.
async void won't work correctly: only part of the async methods (until the first await) will run as expected, with the continuation running out of order (possibly even after the test), or not at all.
The before and after list should probably have items of type Func<Task>, with awaiting taking place in ExecuteTestMethod.
| before.AddRange(setUpMethods.Select<IMethodInfo, Action>(m => async void () => | ||
| { | ||
| var result = m.Invoke(c.TestObject, []); | ||
| if (result is Task task) |
There was a problem hiding this comment.
The logic to await Task/ValueTask is now repeated 3 times in this class. Extracting it to an helper method would help keeping the code clean.
| Action<TestExecutionContext> beforeAction = c => before.Add(() => beforeTest(c)); | ||
| s_beforeTest.SetValue(beforeAndAfterTestCommand, beforeAction); | ||
| var setUpTearDownInfo = beforeTest.Target?.GetType() | ||
| .GetField("setUpTearDown", BindingFlags.Instance | BindingFlags.Public); |
There was a problem hiding this comment.
I don't know how I feel about this. Reflection is always a last resort, but at least before this PR it was only protected fields, part of the exposed API.
After this PR, reflection is used on a compiler-generated closure type, with a field matching a variable name... This seems brittle, but I don't think we have much of a choice.
For example, this won't work in NUnit 4 (which we don't officially support yet in Avalonia.Headless), since the code has changed: https://github.com/nunit/nunit/blob/main/src/NUnitFramework/framework/Internal/Commands/SetUpTearDownCommand.cs
cc @maxkatz6
There was a problem hiding this comment.
For the record, I agree completely, this was just the best solution I could come up with to ensure these methods run. To be clear, there are workarounds users can implement; because asynchronous test execution works, one can forego the [SetUp]/[TearDown] attributes and manually invoke the methods from each test that needs them (e.g. what we do here). This is not ideal, but if documented could certainly be adequate if this solution is determined to be too brittle.
The ideal solution is of course an NUnit change to make the test execution environment more extensible. Hopefully the issue you commented on can get prioritized to allow for that.
There was a problem hiding this comment.
Does NUnit 4 has better extensibility support? If it does, we can completely drop 3.x support together with reflection.
What does the pull request do?
Makes the
SetUpandTearDownmethods in NUnit work within Avalonia test fixtures.What is the current behavior?
When attempting to use
[SetUp]or[TearDown]attributes within anAvalonia.Headless.NUnitproject, the tests will run forever/until timeout due to improper handling of the async methods (blocking forever since they're not properly awaited). Furthermore, as I was testing I discovered thatTearDownmethods were never executed in the old setup as the after-test commands were called afterExecuteTestMethodhad completed (meaning the_afterTestlist was always empty when called).What is the updated/expected behavior with this PR?
SetUpandTearDowndo not block threads andTearDownactually runs.How was the solution implemented (if it's not obvious)?
We now reflect into the before/after commands and obtain the list of setup methods (assuming the command target is of type
SetUpTearDownItem). We then get the list of setup methods and then add them to the_beforeTestlist, appropriately awaiting the ones that returnTaskorValueTask(same behavior as currently exists for tests). If the command target is not of typeSetUpTearDownItem, we execute the old behavior. For after tasks, we do the same thing exceptrather than adding to thewe add the Actions with test context directly to a list of_afterTestlist, we simply have the command execute the after task directly. Theafterparameters and_afterListfields have been removedAction<TestExecutionContext>and then execute the tests inside theExecuteTestMethodas before.Checklist
Breaking changes
None.
Obsoletions / Deprecations
None.
Fixed issues
Fixes #18304