Timeout should apply to test method including its initialize and cleanup

[Youssef1313]

The suggestion in #522 which was implemented in #3334 doesn't sound reasonable to me and adds unnecessary complications.

The timeout applies to the whole test method including initialize and cleanup. If it timed out, we shouldn't attempt to run cleanup. Today, we will run the class cleanup and wait for it. If it took forever, we will be waiting and will have exceeded the test timeout. Try to run the following today:

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

    [TestCleanup]
    public void MyTestCleanup()
    {
        Thread.Sleep(1000000);
    }

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

}

I would expect the test to timeout after ~5 seconds. But because we run test cleanup, we are waiting on the large sleep in it.

[nohwnd]

I don't think skipping cleanup is the right behavior.

Cleanup is there specifically to guarantee that the cleanup will run (as much as possible, we are not talking about turning the power off). It is not primarily about code re-use (as test init), but about being able to clean up.

We also need to think how this will effectively happen, .NET core does not allow the threads to abort, so rather than time out and abort, we are discussing the correct point when to abandon the work to the background, and report that it did not finish on time.

If this was the last test in the assembly we might tear down the process and not run the cleanup, this makes it unpredictable.

So unless the desire is to abort the process right away (which did happen in the past in MSTest and was constant source of confusion and complaints), we are not gaining additional correctness of not corrupting the state of the process by not running the cleanup.

Personally I think that the timeout should apply to the whole test init, test body and test cleanup, and report if they take longer than the sum or their runtimes. When init times out, we should progress to cleanup. When body times out we should progress to cleanup. When cleanup takes too long, it should report in other way to keep the guarantee of running the cleanup.

If the user (or IDE) does not desire to await that cleanup, and subsequents class and assembly cleanups, they should use an 'abort' (e.g. a session time out or hang dump) which guarantees that the work stops right now, no matter the mess it leaves behind.

[Youssef1313]

@nohwnd So for the code sample above, what is your expectation?

• Cleanup taking its full sleep amount?
• Timeout counter resetting when cleanup starts and times out after 5 seconds? i.e, test method times out after 5 seconds, then cleanup times out after 5 seconds, with total duration of the test to be 10 seconds?

[Youssef1313]

Ping @nohwnd for the question above.

[nohwnd]

My ideal world :)

There is time limit for the whole exe after which it terminates (session timeout), e.g. 60 minutes by default, or reasonable amount less than CI runner tells us is the session duration limit.

We run test. if init + body don't finish before timeout, and we go to cleanup. So in example above test body will timeout after 5 seconds and go to cleanup.

Cleanup can take infinite amount of time to run. (We cannot see the future and want to guarantee that the cleanup will run, so we cannot know how long it will take and how big part of the Timeout we should reserve for cleanup).

(for the lack of CleanupTimeout) When Cleanup spends 1/3 timeout time cleaning up (rounded up to seconds), it should warn the user that the cleanup is taking long, and that the test is still being cancelled.

When abort is requested it should report all tests still running, and their stage and then exit. This abort can come from various reasons, either because of the session timeout, or in IDE where it is asking to terminate right away to keep the UI responsive.