Fix exception when retrying test - #191
Conversation
all xunit harness exceptions
e9f179e to
4ce2e14
Compare
David Barbet (dibarbet)
changed the title
| return false; | ||
| } | ||
|
|
||
| return _messageSink.OnMessage(message); |
There was a problem hiding this comment.
this is the fix. the newish xunit runner is expecting that if we call _messageSink.OnMessage(message) for an ITestCaseStarting message (adds to a cache), we also call it for ITestCaseFinished (removes from cache).
Otherwise if we don't call it for ITestCaseFinished we get an exception when calling it for ITestCaseStarting when retrying a test
| protected override XunitTestRunner CreateTestRunner(ITest test, IMessageBus messageBus, Type testClass, object?[] constructorArguments, MethodInfo testMethod, object?[]? testMethodArguments, string skipReason, IReadOnlyList<BeforeAfterTestAttribute> beforeAfterAttributes, ExceptionAggregator aggregator, CancellationTokenSource cancellationTokenSource) | ||
| { | ||
| if (Process.GetCurrentProcess().ProcessName == "devenv") | ||
| if (SharedData.Exception is not null) |
There was a problem hiding this comment.
This is related to the exception handling, but slightly speculative. There is code in IdeTestAssemblyRunner to set SharedData.Exception when there is an exception in the harness. It then eventually calls into this to run the test cases again to report the harness exception for each test case.
However - it appears as though if devenv is still running (aka it wasn't a devenv crash that caused this exception), then we'd never report the harness failure.
I moved this check up to report the harness failure even if we're still running devenv.
This check isn't enough by itself though - as the exception we were hitting would just be thrown again later on as we tried to report the test case starting message for the ErrorReportingIdeTestRunner
| catch | ||
| using (var assemblyRunner = new IdeTestAssemblyRunner(TestAssembly, testCases, DiagnosticMessageSink, executionMessageSink, executionOptions)) | ||
| { | ||
| await assemblyRunner.RunAsync(); |
There was a problem hiding this comment.
There is a bunch of exception handling code in places below this to report test failures if the harness fails.
However the exception handling code attempts to report actual test case failures via the xunit API. If those throw, then the exception bubbles all the way here, and would previously just silently catch it (though there are some logs that contain the exception).
Now, if we get here the actual test runner reports an unhandled exception and doesn't succeed.
This also matches what the xunit samples do, e.g. https://github.com/xunit/samples.xunit/blob/28d3683f74b104d33544efe5d1ae45ce9b0ad8c5/v2/AssemblyFixtureExample/XunitExtensions/XunitTestFrameworkExecutorWithAssemblyFixture.cs#L14
|
Same known issue in CI (verified tests) |
2 participants
Also don't swallow any xunit harness exception that occurs in the actual xunit infrastructure.