Run TearDowns on VS Cancel Test Run

[ChrisMaddock]

ITestExecutor.Cancel() currently forces an immediate stop. I suggest this should instead request a stop, which would run the appropriate TearDowns before ending the test run. I looked briefly at the MSTest adapter - that appears to use a 'clean cancel' rather than 'forced cancel'. This would be an easy fix, should we decide to do it, it's just changing a parameter passed to the engine.

This issue came from Gitter: https://gitter.im/nunit/nunit?at=596fbc53329651f46ea350f2

cc @OsirisTerje @toftware

[ChrisMaddock]

Marked confirm to check there's no reason not to do this before going ahead, cc @nunit/vs-extensions-team!

[CharliePoole]

When does VS call that method?

[ChrisMaddock]

The VS window has a 'cancel' button when you've started a test run. I assume the two are connected - I haven't tested this theory!

[OsirisTerje]

It is connected. And, ref my comment on Gitter, I am not sure changing the default here is good. One reason is that VS already tries to do a lot for you, ending in very slow response on actions. Sometimes VS is just churning for a long time, just because a ton of extensions are trying to nice to you, and do a lot of stuff to "help" you. When I want to Cancel something, I would like it to - as default - just stop, and stop fast.
It could however, be something that was changeable from either an attribute or from runsettings.

[ChrisMaddock]

If I cancel a build, I would do that for a reason. Either it is hanging, or it is taking too long time (so I want to add a filter). Whether I want to run the teardowns or not, would all depends. VS has only one action, and that is Cancel . Normally I would just like it to stop testing as soon as possible, don't bother about teardowns. I guess that is what it is doing now. In some rare cases where I have integration tests or something like that, I might like it to run the teardowns to clean up. Since we dont add anything to the user interface in VS, there is no easy way to separate between these things, and then I would simply like it to "stop as soon as possible" as the default. And then leave it to the developer to e.g. use the Setup methods to clean up previous runs, or something. imho....

(From @OsirisTerje on Slack)

---

Afraid I'd have to disagree here. For my uses - the primary purposes of TearDown's is to clean up, I would expect them to be run by default - with a force kill being something I'd need to put extra work into.

If I cancel a build, I would do that for a reason. Either it is hanging, or it is taking too long time (so I want to add a filter).

My primary reason would be because I've changed my mind. e.g. I've realised the test will still fail, I've misclicked which test I want to run etc...

Additionally, if the test is taking too long, I'd want to run clean ups - otherwise I have to manually do the cleanups, and the overall process takes even longer.

And then leave it to the developer to e.g. use the Setup methods to clean up previous runs, or something.

I think this is the part that most jumps out at me - surely this is exactly what tear downs are for? To do as you'd suggest - I'd need to duplicate all my clean-up code in both tear down and set-up? So it's in the TearDown for when I do a clean-existing run, and then in the SetUp as well for when I cancel the run? This doesn't seem right?

I'll be honest - I don't have any personal interest in this issue, I just opened it as the existing behaviour seemed wrong to me. If we agree to disagree, that's fine and we can close it. 🙂

Have you used the ReSharper runner? That has a two stage cancel - first request, and then if you click again, 'force'. It would be nice if VS exposed that sort of interface. 😞

[OsirisTerje]

Yes, I use the resharper runner too, but unfortunately VS doesn't have the same interface/possibility. This is however something we could raise with them. But as it, VS have only one Cancel possibility.

You didn't comment on using an attribute or runsettings setting to change it, that could be an option.

About the teardown, I would never assume when a setup is run that everything is in perfect order. Calling the same method as the teardown is doing, if things are not as expected for a clean run, is not the same as duplication. The runners and test code can crash for other reasons, so I would always go for safe there.

IMHO: With continuous testing (after build), Live Unit Testing and similar coming up, the Cancel will mostly be when something goes wrong.

So I see 3+2 options: 1) Keep as is 2) Change to "calling teardowns" 3) Add attribute/runsetting option to control behavior:
3a: Have forced as default 3b: Have clean as default

What do others think?

[ChrisMaddock]

You didn't comment on using an attribute or runsettings setting to change it, that could be an option.

I didn't, sorry. I don't like the idea of an attribute - this is a setting for the test runner, not for the test assembly. A runsetting could work, although I'd personally think this would be overkill, it's niche, and if it's a setting, there'd need to be a group of people asking to use it.

I will say I am surprised this has never come up before, this isn't a recent change or anything! I guess perhaps there's a limited area of integration-esque tests, which a) use tear downs and b) are lightweight enough for people to be running directly in VS.

About the teardown, I would never assume when a setup is run that everything is in perfect order. Calling the same method as the teardown is doing, if things are not as expected for a clean run, is not the same as duplication.

This is a fair point. I'd consider a setup check to be fixing bad state though, and I wouldn't want my VS test runs to be the creators of that bad state.

My vote would be for option 2 - but I think that's fairly obvious! 😄

[CharliePoole]

We haven't discussed why it is as it is. Very simply, since the UI only gives us one shot, my primary goal was to ensure that we actually stop.

If a TearDown is already hanging, or hug we try to run TearDown and it subsequently hangs, then we don't stop at all.

The only option I see to get both behaviors from the existing UI is to modify NUnit itself to have a third type of shutdown - first trying a cooperative shutdown and then going to force after some amount of time.

[ChrisMaddock]

The only option I see to get both behaviors from the existing UI is to modify NUnit itself to have a third type of shutdown - first trying a cooperative shutdown and then going to force after some amount of time.

This did cross my mind. Would this not be something the adapter should handle, by requesting a cancellation, and then forcing, if it hasn't returned after X seconds? It seems a limitation of the runner interface we're compensating for here, rather than something the framework needs to know about.

I'm sure the NUnit GUI would present have a suitable interface for both types of cancellation. 😄

[CharliePoole]

With the gui we can allow the user to "time out" and do a forced cancellation.

With the adapter we would have to imitate it automatically. It sounds possible.

[optical]

I apologies if this is the wrong discussion to post this, but it seemed relevant.

We are using Nunit in an integration test project. When we run our integration tests in VS, we have a class which has a [SetUpFixture] attribute, along with OneTimeSetUp and OneTimeTearDown Methods.

In our Setup, we'll spin up the application and its dependencies that we are testing, and the teardown will clean all this up. We can't do the clean-up in the setup - although I'll skip over why that is at its lengthy.

With this existing setup, if we hit the stop button in either VS or Resharpers test runner, the OneTimeTearDown method is not run, which means the resources created in Setup linger around.

Is there any existing functionality in NUnit that we could be hooking into to execute this cleanup on Stop/Cancel?

[ghost]

@optical I am on the same boat as you. Did you find a solution?

[optical]

I'm afraid not, we are still interested in finding a solution, but are putting up with the pain right now.

On Tue., 29 May 2018, 4:21 pm bpunie, notifications@github.com wrote:

@optical https://github.com/optical I am on the same boat as you. Did you find a solution?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/nunit/nunit3-vs-adapter/issues/367#issuecomment-392648765, or mute the thread https://github.com/notifications/unsubscribe-auth/AAu0GfsttF-EotSQzd1UC189e1Eei7Y6ks5t3MzdgaJpZM4OfX1k .

[polendri]

I'm also looking for a solution to this. My use case is Webdriver end-to-end tests which involve starting up a mock SMTP server (Netdumbster); when the SMTP server isn't manually stopped, the process hangs around, tying up the port. These tests can take hours to fully execute, so not being able to cancel one without fouling up environment state is a huge pain point. I don't mind if the solution is a hack/workaround; I'm wondering if maybe hooking into AppDomain.CurrentDomain.ProcessExit might work...

[Kikimora]

Same problem here - want to start and stop blockchain nodes in my integration tests. But if I stop or forcibly stop the test then blockchain node keeps running in the background and consume ton of resources.

[zgramana]

I appreciate the argument in favor of cancelling immediately. However, inflexibility in this matter results in significant discomfort for those of us who reasonably rely on [TearDown] to properly release non-CLR-managed resources (whether directly or indirectly).

Adding to the chorus, we use [SetUp] to create a new Chrome WebDriver instance and [TearDown] to dispose of it (along with the many chrome processes that it, in turn, spawns). We do this in order to prevent an unbounded count of Chrome instances. When a test run finishes normally, it works rather well-- to the point where the user typically remains unaware of all the processes forked off during the process.

However, when the user cancels via the VS test runner--as is often the case during test case development/debugging/modification cycles--the chromedriver parent process and the many chrome child processes remain from each run. These silently accumulate (especially for the unwitting occasional contributors to our test suite) until their workstation performance degrades very noticeably.

Unfortunately we have no sensible remedy. Telling people not to use the VS test runner is a non-starter, as is telling people not to cancel test runs. Since the thread is simply getting aborted, there is no opportunity to handle cleanup. So, we are left with a solution which merely slows the rate at which chrome processes accumulate.

Given the ease by which this behavior may be changed in the NUnit adapter, it would be delightful if at a minimum would could set a run-setting to opt into a non-forced cancel (e.g. <EnableGracefulCancellation>true</GracefulCancellation>).

[CharliePoole]

@zgramana @OsirisTerje @ChrisMaddock Ironically, StopRun(true) is actually broken in the NUnit framework. See nunit/nunit#3276.

Of course, it works here because VS immediately terminates the process... basaically, anything would work.

What about this approach?

void ITestExecutor.Cancel()
{
    if (_activeRunner != null)
    {
        _activeRunner.StopRun(false);

       SpinWait.SpinUntil(!_activeRunner.IsTestRunning, SOME_TIMEOUT);
    }
}

Wouldn't that give you the best of both worlds?

[zgramana]

Perfect!

[ChrisMaddock]

Looks good to me! Should the implementation additionally call StopRun(true) at the end of the timeout, so that we attempt to use framework ‘stop’ functionality before VS kills the process?

[OsirisTerje]

Nice. :-) PR ?

[Danfrid]

@CharliePoole @ChrisMaddock @OsirisTerje

    _activeRunner.StopRun(false);

Can you clarify what is activeRunner and how to initialize it? //I want to use the Stop method to stop the tests if they fall, but there are problems with the implementation

[CharliePoole]

@Danfrid In the context in which I commented, _activeRunner is initialized and ready to use. If it's null, tnen there is nothing running and so nothing to stop.

[Danfrid]

@CharliePoole okaaay.. but not clear..) when test failed, in [SetUp] annotation run code:

   if (TestContext.CurrentContext.Result.Outcome.Status == TestStatus.Failed)
   {
                   var _builder = new DefaultTestAssemblyBuilder();
                   var _runner = new NUnitTestAssemblyRunner(_builder);
...
                  _runner.StopRun(false);
...
    }

but it's doesn't work when test failed, and test explorer continue execution Any suggestion?

[CharliePoole]

Your test runs inside NUnit, which is run by the NUnit adapter, which is run by Test Explorer.

You are trying to control the outside from the inside. That's not possible. The runner you create has nothing to do with the runner that is running your test. It's an entirely new instance. Your call is ignored because the instance you created isn't running anything, so there is nothing to stop.

If we wanted to allow a failed test to stop the entire run, we would have to build that into the framework, perhaps as a special message sent through the TestContext. But we don't have anything like that right now and it would require some discussion to decide if we wanted to have it. That would be best done in a separate issue from this one, which is about what happens when TestExplorer tries to cancel the run.

[roozbehid-ic]

Any progress on this on newer versions of NUnit or VS?

[OsirisTerje]

Still waiting for a PR. I see some have done something above, but no PRs received.

[Overclock303]

hi, is there something up about tests cancellation ?

[KovtunV]

Hi, is there something up about tests cancellation?

[srikcgaa2]

Any progress on this? We are running automated integration test cases on a build server where we spawn process as part of the test case which needs to be disposed/killed if the build gets cancelled on a pipeline running this test case and it would be nice if either teardown or some other mechanism in the test case is getting called where we can take care of disposing properly.