Skip to content

Fix: TestCleanup is always called in case of timeout #3334

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.

Sign up for GitHub

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

Merged
Evangelink merged 1 commit into from
Jul 25, 2024
Merged

Fix: TestCleanup is always called in case of timeout #3334

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
77 changes: 44 additions & 33 deletions src/Adapter/MSTest.TestAdapter/Execution/TestMethodInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,10 @@ public class TestMethodInfo : ITestMethod
/// </summary>
public const int TimeoutWhenNotSet = 0;

private object? _classInstance;
private bool _isTestContextSet;
private bool _isTestCleanupInvoked;

internal TestMethodInfo(
MethodInfo testMethod,
TestClassInfo parent,
Expand Down Expand Up @@ -219,33 +223,33 @@ private TestResult ExecuteInternal(object?[]? arguments)
var result = new TestResult();

// TODO remove dry violation with TestMethodRunner
object? classInstance = CreateTestClassInstance(result);
bool testContextSetup = false;
_classInstance = CreateTestClassInstance(result);
bool isExceptionThrown = false;
bool hasTestInitializePassed = false;
Exception? testRunnerException = null;
_isTestCleanupInvoked = false;

try
{
try
{
if (classInstance != null && SetTestContext(classInstance, result))
if (_classInstance != null && SetTestContext(_classInstance, result))
{
// For any failure after this point, we must run TestCleanup
testContextSetup = true;
_isTestContextSet = true;

if (RunTestInitializeMethod(classInstance, result))
if (RunTestInitializeMethod(_classInstance, result))
{
hasTestInitializePassed = true;
if (IsTimeoutSet)
{
ExecutionContextService.RunActionOnContext(
() => TestMethod.InvokeAsSynchronousTask(classInstance, arguments),
new InstanceExecutionContextScope(classInstance, Parent.ClassType));
() => TestMethod.InvokeAsSynchronousTask(_classInstance, arguments),
new InstanceExecutionContextScope(_classInstance, Parent.ClassType));
}
else
{
TestMethod.InvokeAsSynchronousTask(classInstance, arguments);
TestMethod.InvokeAsSynchronousTask(_classInstance, arguments);
}

result.Outcome = UTF.UnitTestOutcome.Passed;
Expand Down Expand Up @@ -301,10 +305,7 @@ private TestResult ExecuteInternal(object?[]? arguments)
// Pulling it out so extension writers can abort custom cleanups if need be. Having this in a finally block
// does not allow a thread abort exception to be raised within the block but throws one after finally is executed
// crashing the process. This was blocking writing an extension for Dynamic Timeout in VSO.
if (classInstance != null && testContextSetup)
{
RunTestCleanupMethod(classInstance, result);
}
RunTestCleanupMethod(result);

return testRunnerException != null ? throw testRunnerException : result;
}
Expand Down Expand Up @@ -445,14 +446,18 @@ private static TestFailedException HandleMethodException(Exception ex, Exception
/// <summary>
/// Runs TestCleanup methods of parent TestClass and base classes.
/// </summary>
/// <param name="classInstance">Instance of TestClass.</param>
/// <param name="result">Instance of TestResult.</param>
[SuppressMessage("Microsoft.Design", "CA1031:DoNotCatchGeneralExceptionTypes", Justification = "Requirement is to handle all kinds of user exceptions and message appropriately.")]
private void RunTestCleanupMethod(object classInstance, TestResult result)
private void RunTestCleanupMethod(TestResult result)
{
DebugEx.Assert(classInstance != null, "classInstance != null");
DebugEx.Assert(result != null, "result != null");

if (_classInstance is null || !_isTestContextSet || _isTestCleanupInvoked)
{
return;
}

_isTestCleanupInvoked = true;
MethodInfo? testCleanupMethod = Parent.TestCleanupMethod;
Exception? testCleanupException;
try
Expand All @@ -462,22 +467,22 @@ private void RunTestCleanupMethod(object classInstance, TestResult result)
// Test cleanups are called in the order of discovery
// Current TestClass -> Parent -> Grandparent
testCleanupException = testCleanupMethod is not null
? InvokeCleanupMethod(testCleanupMethod, classInstance, Parent.BaseTestCleanupMethodsQueue.Count)
? InvokeCleanupMethod(testCleanupMethod, _classInstance, Parent.BaseTestCleanupMethodsQueue.Count)
: null;
var baseTestCleanupQueue = new Queue<MethodInfo>(Parent.BaseTestCleanupMethodsQueue);
while (baseTestCleanupQueue.Count > 0 && testCleanupException is null)
{
testCleanupMethod = baseTestCleanupQueue.Dequeue();
testCleanupException = InvokeCleanupMethod(testCleanupMethod, classInstance, baseTestCleanupQueue.Count);
testCleanupException = InvokeCleanupMethod(testCleanupMethod, _classInstance, baseTestCleanupQueue.Count);
}
}
finally
{
#if NET6_0_OR_GREATER
// If you implement IAsyncDisposable without calling the DisposeAsync this would result a resource leak.
(classInstance as IAsyncDisposable)?.DisposeAsync().AsTask().Wait();
(_classInstance as IAsyncDisposable)?.DisposeAsync().AsTask().Wait();
#endif
(classInstance as IDisposable)?.Dispose();
(_classInstance as IDisposable)?.Dispose();
}
}
catch (Exception ex)
Expand Down Expand Up @@ -810,24 +815,30 @@ void ExecuteAsyncAction()
}

DebugEx.Assert(result is not null, "result is not null");

// It's possible that some failures happened and that the cleanup wasn't executed, so we need to run it here.
// The method already checks if the cleanup was already executed.
RunTestCleanupMethod(result);
return result;
}

// Timed out or canceled
string errorMessage = string.Format(CultureInfo.CurrentCulture, Resource.Execution_Test_Timeout, TestMethodName);
if (TestMethodOptions.TestContext.Context.CancellationTokenSource.IsCancellationRequested)
{
errorMessage = string.Format(CultureInfo.CurrentCulture, Resource.Execution_Test_Cancelled, TestMethodName);
}
else
{
// Timed out or canceled
string errorMessage = string.Format(CultureInfo.CurrentCulture, Resource.Execution_Test_Timeout, TestMethodName);
if (TestMethodOptions.TestContext.Context.CancellationTokenSource.IsCancellationRequested)
{
errorMessage = string.Format(CultureInfo.CurrentCulture, Resource.Execution_Test_Cancelled, TestMethodName);
}
else
{
// Cancel the token source as test has timed out
TestMethodOptions.TestContext.Context.CancellationTokenSource.Cancel();
}

TestResult timeoutResult = new() { Outcome = UTF.UnitTestOutcome.Timeout, TestFailureException = new TestFailedException(ObjectModelUnitTestOutcome.Timeout, errorMessage) };
return timeoutResult;
// Cancel the token source as test has timed out
TestMethodOptions.TestContext.Context.CancellationTokenSource.Cancel();
}

TestResult timeoutResult = new() { Outcome = UTF.UnitTestOutcome.Timeout, TestFailureException = new TestFailedException(ObjectModelUnitTestOutcome.Timeout, errorMessage) };

// We don't know when the cancellation happened so it's possible that the cleanup wasn't executed, so we need to run it here.
// The method already checks if the cleanup was already executed.
RunTestCleanupMethod(timeoutResult);
return timeoutResult;
}
}