Don't enforce [Timeout] when debugger is attached

[bormm]

Greetings NUnit team!

I was just debugging within a nunit test that has a Timeout attribute. Unfortunately it was aborted by timeout while debugging. I have found this existing issue #1479 that was already fixed by #1785 in 2016.
For me it seems the fix got lost with commit 1a164db .

So is there something wrong with my little research result or was the feature lost without intention?

[ChrisMaddock]

I think you're right, and this was just missed during code review.

We'd welcome a Pull Request to fix it, if you'd be interested?

[bormm]

Ok. I will try to do it. I never changes something in NUnit, but the code looks quite readable :-)

[ChrisMaddock]

Great - thanks! Check out https://github.com/nunit/nunit/blob/master/BUILDING.md for a few specialist details, if you have any issues building/running the tests, and let us know if we can help at all. 🙂

[ChrisMaddock]

Also, given we've already had one regression, it would be really cool if we could unit test this. Maybe we could put a wrapper around the Debugger class or something, to make that possible. 🙂

[thepinkmile]

Hey guys,
Any update on this at all?
I was just about to post this very same issue.

Currently my codebase has it's own implementation of TimeoutAttribute which does this.
Also just to mention that this should probably also be done for MaxTimeAttribute too?
Depending on my workload, I may be able to help with this.

[ChrisMaddock]

Thanks @thepinkmile! The change to MaxTimeAttribute sounds good to me, would appreciate a PR for that.

@bormm How are you getting on with this issue? 🙂 Do shout if we can help at all!

[thepinkmile]

Not sure if this is the correct way to do it, but based on referenced items in initial post.
Would not have a clue how this could be tested though.
Any thoughts welcome.

[bormm]

@thepinkmile @ChrisMaddock Unfortunately I got no resources to work on this issue in my working time, so I am currently not able to do something for this issue. Especially for the DI + testing part, which would take the most time. I am also not sure how useful the testing could be, because you can't test if the encapsulated debugger function is working as expected.

But long story short: If @thepinkmile would just adapt his change and pick this issue, it would be a good thing.

[ChrisMaddock]

I was thinking it might be possible to pass in an IDebugger interface into Timeout command, so we could then mock out the debugger being attached or not, and put in a test that the command doesn't time out when under the debugger.

In this case, perhaps we're just better to get a fix in however. @thepinkmile - if you'd like to PR your fix to TimeoutCommand, that would be very welcome! The MaxTime one, the team might want to discuss a little more, but I think it sounds sensible to me!

@bormm - thanks regardless for your help in identifying and logging the issue. 😄

[thepinkmile]

I currently have it all as a single commit. Will break it down tonight and add as 2 separate commits with 2 different PRs. Will probably have a look at doing some kind of tests for it over weekend if I get time.

[ChrisMaddock]

Sounds great, thanks!

[thepinkmile]

Not had a chance to fix this up yet. Will likely be over the weekend I do it all.
However, I was just wondering what the expectation would be with using [Timeout] or [MaxTime] with [Retry]?
Not sure if this is because of our custom internal impl of Timeout, but we seem to get the timeout error every time.
E.G. If I set Retry(3) and Timeout(20000) and my test takes 10000ms to fail, the retry doesn't seem to reset the timeout for each attempted run.
Not sure if this would be the same as i think our internal version sets the timeout at start of execution (e.g. before SetUp) and ends after completion (e.g. after TearDown). From what I can gather from the TimeoutCommand, this happens only for the test it's self. So this would mean each retry causes a new TimeoutCommand for each attempted run?

[ChrisMaddock]

Which version of the NUnit framework are you using? This was a bug which was recently fixed in 3.11, see #2786. Not sure whether the fix will be relevant to your custom attribute or not, but if you have a look at the related PR, it should hopefully point you in the right direction!

Not had a chance to fix this up yet. Will likely be over the weekend I do it all.

No rush - and thanks! 😄

[thepinkmile]

Apparently we are still using 3.9 (will update this soon I hope).

I am currently working on a new fix for this that includes the IDebugger interface to allow for testing. Also to allow for disabling of this check if required (apparently dotcover sets Debugger.IsAttached to true, so probably not a good thing). Wondering if it is possible to have a command-line flag to enable and disable this check. So running the test it just does the right thing, but if using dotcover then it still bombs out if needed.
I am wondering if in this case it is useful as dotcover can make the process a lot slower anyway, so maybe a case of disable the check and optional parameter to specify a ScaleBy value (default to double set thresholds)?

[ChrisMaddock]

Thanks for taking on the IDebugger idea - sounds promising!

dotcover's an interesting situation. I'm not sure how this works...but you might want to test it out...what happens if you use the --timeout=60 command line parameter? Does that also not work when the debugger is attached?

FYI - I've just unassigned this issue from [@]bormm - unfortunately GitHub won't let me assign it directly to you as a first-time contributor! I've assigned it to myself instead, just so it's clear to anyone else coming along that someones working on it - look forward to your pull request! 😄

[zdenek-jelinek]

Hello, since the topic has not been active for several months, I took the liberty in analysing the problem and would like to ask you for further guidance.

1. I have tested several versions of dotcover (2017.3.5, 2018.2.0, 2019.1.3, 2019.2.0-eap2) with VS2019, directly with dotcover.exe and through dotnet dotcover test and none of these cases had Debugger.IsAttached true during the run, in contrast to what was stated previously. I suggest to ignore the issue.
2. The timeout parameterization is currently streamlined into one timeout parameter so that test timeout has precedence over fixture timeout which has precedence over the argument. Is it expected for the --timeout to cancel the tests even with debugger attached or should it behave the same as the Timeout attribute?
3. I'm not sure if the fix should touch MaxTimeAttribute. For me, the issue is that the test run gets aborted in the middle of my debugging session, I don't care so much about the result being timeout or anything else. This is the difference between Timeout (cancel test and fail it) and MaxTime (wait for test to complete and fail it if it takes too long). Also, the debugger causing NUnit to ignore timeout is only mentioned on Timeout documentation, not MaxTime. On the other hand, the change is trivial - I just need your philosophical guidance here.

Please advise on the way forward with this, I would be happy to implement the fix.

[benviss]

Hello,
Just curious if there is going to be any work done on this ticket soon.