Test timeout with teardown executed?

[kdubau]

Hi folks, I've done a bit of searching and can't find a solution. Searching timeout in the issues here has some relevant results but issues are either closed without resolution (from years back) or still open.

I have a simple question: for those of us that require a way to have a test timeout but still execute the teardown what are our options? If a test case does some setup, and times out during the test, in many cases the teardown still needs to execute. That is, there is still some clean up that needs to happen so things are healthy for the next test.

I've searched and pondered but haven't found a solution. Can you recommend something? Are there any extensions points maybe? It seems timeout functionality is of limited use if we cannot clean up after the timed out test. Arguably, cleaning up after a timed out test can be more important than even a successful test.

[kdubau]

In addition to cleanup, the user might want to do some analysis after the time out. ie. Why did it timeout? The user should be able to capture logs, machine state, etc.

[ChrisMaddock]

I hit exactly the same problem a few months ago - @kdubau, did you come across this thread? https://groups.google.com/forum/#!topic/nunit-discuss/3-v-6bosbyM

I never found the time to address it - I agree it would be a good feature.

[kdubau]

I actually didn't find that particular thread, but that summarizes my situation quite perfectly.

My gut feeling is that Timeout should only apply to the test method, and neither setup nor teardown.

Same here!

allow you to create a method that runs after a timeout, marking it with some special attribute.

Would work for me!

I would love to see this become a feature, but I need something ~now - any ideas for something temporary to hold me over? I can't use OneTimeTearDown, mostly for the same reasons you posted in that discussion thread.

[CharliePoole]

I replied to @ChrisMaddock 's earlier discussion thread with two alternative ways it could be done. I had hoped somebody would pick one and create an issue for it. I sincerely hope this issue will not become another long discussion as to why it's needed, why the current design is bad,etc.

If this is a request for a workaround (i.e. question) the answer is that there really is not one. We should close it. If we want to make this the issue that implements an enhancement, the person wanting to carry it out should pick one of the two approaches I suggested and propose doing it. Neither is something I would support as suitable for a new contributor because they both apply to the innermost workings of our test execution. I'm stepping back from the project myself but I'll be glad to consult with a team member who steps up to do this.

IOW @ChrisMaddock I don't think a lot of design is needed - just picking what needs to be implemented. My suggestion (to you or any implementor) is to first outline the changes needed in each case. They are entirely different, obviously, and I expect that the degree of change involved will have a big influence on the choice.

A key point to remember in all this, of course, is that your timeout can hang as well. If we make the TimeoutAttribute apply to the test alone, what will we do about that?

[ChrisMaddock]

@CharliePoole - can you clarify what you meant here, from your post on NUnit-Discuss?

The WorkItem simply runs the TestCommand that was built for the test, which includes all three of those as well as any ActionAttributes.
In order to distinguish just the test method and to run teardown after timeout, we would need to re-implement timeout as a TestCommand

Timeout is currently a BeforeAndAfterTestCommand. I'm not totally sure the change you're suggesting?

A key point to remember in all this, of course, is that your timeout can hang as well. If we make the TimeoutAttribute apply to the test alone, what will we do about that?

Did you mean 'teardown' instead of 'timeout' there? In my opinion, that's the user's problem. 🙂 TearDown code should manage such things itself.

---

A separate discussion - how should we implement this, in terms of backwards compatibility? Just changing the behaviour would make me happy, but good for existing tests. We could consider an additional [TimeoutTearDown] attribute - but that's a little ugly. Thoughts?

[ChrisMaddock]

I should add - this isn't an issue I imagine I'll have much time to look at personally, at least for a good while yet. 😞 But I'd be keen to spec something out, while we're here - it's a limitation I've encountered before.

[CharliePoole]

@ChrisMaddock You're right! My change moving timeout processing out of WorkItem to the Command chain was merged. I actually thought it was turned down - which is why I was being a bit ironic about it in my comment.

That being the case, changing the behavior is now "just" a matter of TimeoutAttribute implementing a different interface. I say "just" because the hard job, as you point out, is figuring out the right thing to do!

Yes, I meant to type teardown but my Android phone preferred timeout. I think the phone's auto-correct is actually making a lot of design decisions these days! 😄

In principle, all user code should make sure it doesn't time out! Still, we give them an attribute just in case it does. Nevertheless, I don't like the idea of a separate attribute for timeout. My preferences, in order...

1. Make the change and document it. Wait to see if it causes any issues and only then move down this list. Note that we have an existing bug issue that says timeout within TearDown doesn't work, so we would not be breaking anything new there. We would not, however, be catching timeout in SetUp, which we now do.

2. More elegantly... let the TimeoutAttribute appear on SetUp and TearDown, applying to those methods only if necessary. This is a bigger change than the first because it means we need to invent some new interfaces for use in the attribute.

3. Give TimeoutAttribute a named property IncludeSetUpAndTearDown that defaults to false. Document that you usually don't want to use this. The attribute would have to implement both interfaces but do nothing in one of them, depending on the property value.

4. Create an entirely new property. Actually, I hate this. 😄

[ChrisMaddock]

Auto-correct-influenced-design, I love it! 😆

Good list! Personally, I'd favour adding an IncludeSetUpAndTearDown property to TimeoutAttribute but which defaults to true. That would also give myself and @kdubau the ability to get a TimeoutAttribute which meets our needs, and also avoid a breaking change. I'd then want to change this default in 4.x.

My concern with option 1 is that the failure case is a wrong-side failure. We use TimeoutAttribute in some cases to (loosely) monitor performance. If I previously wrote a test to allow 30 secs for SetUp/Test/TearDown - and a NUnit framework change means that this 30 seconds is now only for the 'Test' section, than my test is no longer 'correct', and I get no notification of this. (This wouldn't be an issue if we were making the change in the other direction, (e.g. Test -> SetUp/Test/TearDown) - then I would start seeing a load of Timeout failures and look in to them.)

@rprouse - as a breaking change, would be good to hear your thoughts on this. Also @jnm2 - I know you do a bit with timeouts.

[CharliePoole]

@ChrisMaddock MaxTimeAttribute is the one that's intended to monitor performance. TimeoutAttribute's original motivation was to allow your tests to finish. Of course, folks always use it the way they use it and we discover new features in our own work that way all the time. 😄

Note that we treat Timeout failure as an error meaning "you wrote the test wrong or something unexpected happened in the SUT." Maxtime being exceeded OTOH is actually reported as a test failure.

I agree that not including SetUp is a real breaking change. Not including TearDown would be a breaking change if timeouts actually worked in TearDown, but they don't.

Personally, I wouldn't hesitate to make the change in (1) in the next release. Yes, it's technically breaking, but we've done worse and we are clearly not really following SemVer any longer. It's not a silent failure, which would be really bad. The test will simply hang, where the user thought it would time out. Whether this happens a lot or not is something we would then learn and could correct.

We could, of course, jump right to solution (2), which I like becasue it reuses an attribute we already have in a separate context.

One thing we have not talked about is command-line timeouts. If the user sets a default using --timeout, how should that apply?

[ChrisMaddock]

It's not a silent failure, which would be really bad.

It's only not a silent failure if you're aware there's an independent MaxTimeAttribute as opposed to TimeoutAttribute - which I didn't. 😄 Looks like I've got some changes to make tomorrow! That's my main concern re: just making the change - in our case, it could introduce silent failures.

One thing we have not talked about is command-line timeouts. If the user sets a default using --timeout, how should that apply?

IMO, this should continue to work as it currently does, with no change to functionality, unless we get a user requesting it. (I'm personally only interested in an attribute, and @kdubau hasn't said otherwise!)

[CharliePoole]

If we leave the default timeout alone, that's much less of a breaking change.

Regarding "silent failure" what I mean is that you don't want your tests to appear to pass when there is something wrong. If a setup times out and we don't catch it, the tests won't appear to pass. They will hang. Of course, you won't have much info to tell you which setup hung, so it's not great. ☹️

[ChrisMaddock]

Regarding "silent failure" what I mean is that you don't want your tests to appear to pass when there is something wrong

We agree on the definition of 'silent failure'. 🙂 My point was it could be a silent failure, for users who've been 'mis-using' TimeoutAttribute for performance based things.

Contrived example:

• I write a test: PerformanceTest(). I want to ensure my [Test] runs in under 100ms. I expect [SetUp] to take 500ms, so add [Timeout(600)].
• We make the change in NUnit 3.9 so Timeout only applies to [Test].
• During refactoring my code, I accidentally make [Test] take 300ms. Normally, this would fail my PerformanceTest(), and I'd realise my error. With the change in NUnit 3.9 however, my [Test] has an extra 500ms of leeway, so PerformanceTest() absorbs the extra time, and I have a silent failure.

This is niche, I'll admit. It's also significantly less relevant now I know we've been misusing TimeoutAttribute - but I wonder if we're the only ones there. @kdubau - is MaxTimeAttribute any use for your case, or is it definitely a TimeoutAttribute you need?

[jnm2]

Autocorrect gets me so often! I've been talking about this for long enough, I finally turned it completely off on my phone. We'll see how this goes. 😁

I like IncludeSetUpAndTearDown but I'd actually prefer separate properties because I can see myself wanting a timeout on setup but I can't see myself wanting it on teardown.
Would this be too complex to implement?

Backwards compat defaulting to true vs false– this will change everyone's timings, but honestly I don't expect it to be a problem.
Timeouts are always heavily dependent on runtime environment and as such, the point is to detect and fail
when the test would run forever without actually waiting forever. We do have a back-compat tenet though which we should think about.

You mention that due to a bug there's no compat issue with teardown, so IncludeSetUp could default to true and IncludeTearDown could default to false and we'd have no breaking changes. And I would like that asymmetry.

[CharliePoole]

@jnm2 I didn't have separate IncludeSetUp and IncludeTearDown properties in my list of alternatives. Although they are hypothetically possible, they really don't fit in the design of nested commands and would require some pretty ugly code. Currently, both the initialization of the timeout and the cancellation take place in the same bit of code, keeping everything nice and symmetrical. I think it's better to keep it that way rather than letting the two includes vary independently of one another.

[kdubau]

Love seeing this constructive discussion :)

MaxTimeAttribute any use for your case, or is it definitely a TimeoutAttribute you need?

MaxTime wouldn't work for me. As I understand it, MaxTime doesn't cancel the test, it will let it finish but reports as a failure if the time is exceeded. I am looking to recover from an unexpected event, namely bugs (or even third-party tools) resulting in hangs. Therefore, I need the test to be canceled, execute the teardown to clean things up, and proceed to the next test.

I personally like @CharliePoole's option 1, but option 2 would be just as well. Hell, option 3 is fine with me too!

One thing we have not talked about is command-line timeouts. If the user sets a default using --timeout, how should that apply?

I would actually use this default setting too. I'm imagining I'd have a lengthy default timeout, and smaller timeouts on fixtures and methods as need. So I would advocate whatever changes are made to the TimeoutAttribute are also applied to --timeout. That said, I could just apply the attribute at the assembly level if --timeout doesn't change.

[CharliePoole]

I have a strong suspicion that there is a subset (possibly large) of users, who make use of --timeout but not TimeoutAttribute.

[ChrisMaddock]

I agree with @kdubau - there's no need to change the behaviour of --timeout, if that can be done with an assembly level attribute.

---

I want to decide on a design here - I don't think it's something I have time to pick up immediately, but would like to get something decided so it would be ready for someone to look at. Are we all happy with adding an IncludeSetUpAndTearDown property to TimeoutAttribute, which defaults to true? I don't see the need to make any potentially-breaking-change to implement this feature request.

[CharliePoole]

I hate properties that act like feature switches. Can we use a new attribute instead?

[jnm2]

Sounds good.

[jnm2]

I hate properties that act like feature switches

I would have said that a majority of our attributes have such behavior modifiers.
Not sure what the alternative is– TimeoutExcludingSetUpAndTearDownAttribute is distasteful to me. That said, I don't have a strong opinion.

[CharliePoole]

I didn't say modifying behavior, but feature switch. That's a term we used to mean turning a feature in it off. We only have a few of those, and they are all mistakes IMO.

If we had a new attribute, it would implement a completely different interface. Using a property means we implement both interfaces but ignore one of them based on the property.

I think that's a great example where your code us telling you you did something wrong.

Best name for the new attribute, of course, is Timeout, but that's taken. TestTimeout is possible.

However, I think I'm convincing myself that we should do the breaking change and implement option 2 immediately.

[jnm2]

I like option 2 the best and I'm not worried about compatibility because of how flaky and unreliable precise timings are (due to how CPUs work). The theory is that timeouts only exist to convert unpleasantly long hangs into more timely failures.

[CharliePoole]

Option 2 is a bit of work, but would give cleaner code. If we go that way, I'm willing to give it a shot.

[ChrisMaddock]

I still prefer the non-breaking change path - but I feel like people are more opposed to the attribute property, than I am to option 2! And you're both right, it is a minor breaking change.

[jnm2]

Weirdly I'm not a tie breaker because I am okay with either approach and only have a small preference for option 2. Maybe @rprouse should weigh in, especially since we're discussing compatibility again. 😀

[rprouse]

Sorry guys, I am on vacation with my wife's family in Portugal, so I haven't been doing a good job of keeping up with discussions. I also prefer option 2. I think it is most elegant from a code perspective and I also think it gives the user what they would expect without having to read documentation of what the attribute does or does not include. I like it because it is very explicit and would allow you to assign different timeouts to each.

[CharliePoole]

Sounds like a decision!

One thing we haven't resolved, however. 😞 What should be the meaning of a TimeoutAttribute placed on a fixture or on the assembly. Should it keep it's current meaning or apply only to the test? Alternatively, should we create a DefaultTimeoutAttribute since that's what [Timeout] on a fixture actually is.

@jnm2 This is one of those examples I was referring to where I used the same attribute with two subtly different meanings. Turns out it almost always gets us into trouble. 🤕

Charlie

[jnm2]

I'd say applying to all methods, whether tests or not, makes sense. Before if a setup took too long and the fixture had a timeout specified, it would time out, so we'd preserve that. Instead of sharing the time now we're giving each method that amount of time which seems reasonable given that Timeout is just a backstop to convert uncomfortably long hangs into timely failures.

[goblinmaks]

Just want to know if anyone implement [TearDown] execution after [Timeout] is reached.
Maybe anything changed in NUnit framework that can simplify implementation on this enhancement ?
@jnm2 @CharliePoole @ChrisMaddock
Thanks,

[manfred-brands]

@goblinmaks This works on .NET Framework, but not on anything later as there is no support for Thread.Abort. See #4021.