Skip to content
This repository was archived by the owner on Aug 24, 2026. It is now read-only.

Fix exception when retrying test #191

Merged
David Barbet (dibarbet) merged 1 commit into from
Aug 8, 2025
Merged

Fix exception when retrying test #191

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 7 additions & 1 deletion ...icrosoft.VisualStudio.Extensibility.Testing.Xunit.Shared/Harness/IdeTestAssemblyRunner.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,6 +303,7 @@ private Func<Task<RunSummary>> CreateTestCollectionInvoker(VisualStudioInstanceF
var previousException = WpfTestSharedData.Instance.Exception;
try
{
// Run the tests again, but using an error reporting test runner that will report the exception.
WpfTestSharedData.Instance.Exception = e;
return await RunTestCollectionForUnspecifiedVersionAsync(completedTestCaseIds, messageBus, testCollection, testCases, cancellationTokenSource).ConfigureAwait(true);
}
Expand Down Expand Up @@ -455,7 +456,12 @@ public bool OnMessage(IMessageSinkMessage message)
testCaseFinished.TestsSkipped + testCaseFinished.TestsFailed);
}

return !_cancellationToken.IsCancellationRequested;
if (_cancellationToken.IsCancellationRequested)
{
return false;
}

return _messageSink.OnMessage(message);

@dibarbet David Barbet (dibarbet) Aug 8, 2025 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
else if (!_finalAttempt && message is ITestFailed testFailed)
{
Expand Down
10 changes: 2 additions & 8 deletions ...osoft.VisualStudio.Extensibility.Testing.Xunit.Shared/Harness/IdeTestFrameworkExecutor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,9 @@ public IdeTestFrameworkExecutor(AssemblyName assemblyName, ISourceInformationPro
[SuppressMessage("Usage", "VSTHRD100:Avoid async void methods", Justification = "Follows pattern expected by Xunit framework.")]
protected override async void RunTestCases(IEnumerable<IXunitTestCase> testCases, IMessageSink executionMessageSink, ITestFrameworkExecutionOptions executionOptions)
{
try
{
using (var assemblyRunner = new IdeTestAssemblyRunner(TestAssembly, testCases, DiagnosticMessageSink, executionMessageSink, executionOptions))
{
await assemblyRunner.RunAsync();
}
}
catch
using (var assemblyRunner = new IdeTestAssemblyRunner(TestAssembly, testCases, DiagnosticMessageSink, executionMessageSink, executionOptions))
{
await assemblyRunner.RunAsync();

@dibarbet David Barbet (dibarbet) Aug 8, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}
}
Expand Down
11 changes: 6 additions & 5 deletions src/Microsoft.VisualStudio.Extensibility.Testing.Xunit.Shared/Threading/IdeTestCaseRunner.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,16 +43,17 @@ public VisualStudioInstanceKey VisualStudioInstanceKey

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)

@dibarbet David Barbet (dibarbet) Aug 8, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
// There was an exception in the test harness running this test previously. Report the failure in we saw in the test harness.
return new ErrorReportingIdeTestRunner(SharedData.Exception, test, messageBus, testClass, constructorArguments, testMethod, testMethodArguments, skipReason, beforeAfterAttributes, aggregator, cancellationTokenSource);
}
else if (Process.GetCurrentProcess().ProcessName == "devenv")
{
// We are already running inside Visual Studio
// TODO: Verify version under test
return new InProcessIdeTestRunner(test, messageBus, testClass, constructorArguments, testMethod, testMethodArguments, skipReason, beforeAfterAttributes, aggregator, cancellationTokenSource);
}
else if (SharedData.Exception is not null)
{
return new ErrorReportingIdeTestRunner(SharedData.Exception, test, messageBus, testClass, constructorArguments, testMethod, testMethodArguments, skipReason, beforeAfterAttributes, aggregator, cancellationTokenSource);
}
else
{
throw new NotSupportedException($"{nameof(IdeFactAttribute)} can only be used with the {nameof(IdeTestFramework)} test framework");
Expand Down
Loading