testfx: Timeout attribute does not call test cleanup method

Steps to reproduce

[TestClass] 
public class TestCleanup 
{ 
[TestInitialize()] 
public void MyTestInitialize() 
{ 
Console.WriteLine("Calling initialize"); 
} 


// Use TestCleanup to run code after each test has run 

[TestCleanup()] 
public void MyTestCleanup() 
{ 
MessageBox.Show("Cleanup"); 
Console.WriteLine( ng cleanup"); 
} 

[Timeout(5000)] 
[TestMethod] 
public void TestMethod1() 
{ 
Console.WriteLine("Sleeping for 10 seconds"); 
System.Threading.Thread.Sleep(10000); 
Console.WriteLine("Waking up"); 
} 

}

Expected behavior

It is essential all tests execute their TestCleanup code, as this code may have important steps to recover from an error such as a time out.

Actual behavior

The result of executing this code is the test times out but the message box is not displayed and the Calling cleanup message is not written to the console.

AB#1634637

About this issue

Original URL
State: open
Created 6 years ago
Reactions: 33
Comments: 18 (8 by maintainers)

Most upvoted comments

I can’t fathom how this is remotely an enhancement rather than a serious bug that should be fixed ASAP 😕. Makes using Timeout absolutely useless and dangerous.

kzu on Nov 21, 2019

In more complex scenarios like E2E UI tests where more than one failure may need to be collected and analyzed per test, this could be called a bug rather than enhancement.

ghost on Sep 23, 2019

The workaround is to not use the attribute, and instead have your test methods run as part of a call to something like Execute(timeout, () => { actual test method }), where the Execute helper essentially does the same thing the timeout does, which is at https://github.com/microsoft/testfx/blob/master/src/Adapter/PlatformServices.Desktop/Services/DesktopThreadOperations.cs#L27. Instead of returning false for timed out execution, it could just throw an exception which would cause the test to fail like a regular test failure, causing the proper cleanup to run.

As to why the current implementation is doomed and not trivially fixable, see the implementation where you can observe how the timeout is basically wrapping the entire execution (including test class creation, test initialize and cleanup, as well as class cleanup) in a delegate that is then timed-out. If the timeout happens, none of the lifecycle events may run (depending on where the timeout occurs) since that ExecuteInternalWithTimeout has no knowledge of the lifetime events and just wraps ExecuteInternal.

Fixing it would require refactoring that code so that execution is split in the actual lifetime phases, the timeout is applied to just the test method invocation, and the lifetime phases are always ensured to run to completion.

kzu on Nov 27, 2019