TimeoutAttribute not available when targeting netcoreapp framework

[rdghickman]

It seems that TimeoutAttribute is not available in the portable version of NUnit 3 when targeting .NET core with a netcoreapp test. This is extremely useful for any sort of test involving threading or integration test, so it would be really useful if this was made available.

Using: NUnit 3.4.0, dotnet-test-nunit 3.4.0-beta1.

[rprouse]

We hope to reimplement timeout and other features that were dropped in the PCL when we add a netstandard version of the framework. They were dropped because timeout requires threads which aren't available in PCL.

[jhickson]

Is there any timeline for enabling TimeAttribute and DelayedConstraint for the netstandard version? We're currently porting our libraries to target both .NET Framework and .NET Core but our test suites make quite heavy use of DelayedConstraint in particular such that porting them to support both platforms would be a pain.

[rprouse]

The first step of porting to .NET Standard is done. Hopefully we can read the thread bases attributes Q1

[ChrisMaddock]

Edit: ignore the below - I missed the ThreadUtility class, which can't be converted straight off.

I was contemplating having a look at reimplementing these things in a Taskbased, manner, but then found System.Threading.Thread is actually available for .NET Standard 1.3.

https://www.nuget.org/packages/System.Threading.Thread/

Actual cross-platform support is apparantley a little sketchy, but not in a way that I think would affect us - our UWP runner will be using the PCL build (or subsequent netstandard 1.0) for the forseeable.

Would you be happy with us pulling in this package, or was this something you specifically didn't do? I can PR, at some point.

[rprouse]

I was planning on pulling in the threading package as we start to tenable functionality. What about Thread utility isn't convertible?

[ChrisMaddock]

Thread.Abort() and such isn't available in .NET Standard. (I don't think?) This is used to kill the thread used for timeouts.

DelayedConstraint is works with System.Threading.Thread - I could push that as a separate PR, and then revisit something Task based for Timeouts?

The reason I didn't want to convert everything to be task based, is that they're not available pre-.NET4, so it would require two separate implementations for pre and post NET4. Hence why I think pulling in the package is good for the thread usages in DelayedConstraint, instead of converting that to use Task.Delay()'s instead of Thread.Sleep(). Thoughts?

[abbotware]

https://github.com/abbotware/nunit-timeout <- sample code for creating a .Net Core Timeout attribute

[abbotware]

btw, if there was a ExecuteAsync on the TestExecutionContext, this could easily be converted into Cooperative Cancellation variant.. but the [Test] method would need a new variant that accepts a CancellationToken

[jnm2]

I'd have a preference for having our test method command inject a CancellationToken versus spinning up new threads or tasks for tests with timeouts and letting them leak. For example, what if a spin wait goes bad and pegs a CPU for the remainder of the test run?

Also, if we spin up a new thread and leave it running, how does this interact with [NonParallelizable]?

[abbotware]

if a test is 'hung' - all bets are off - I think getting partial results is better than no results - worse case add a special overload to the timeout(100, AbortTestRun=true) <- that means if this timeouts - Program.Exit()

[jnm2]

Co-op is the cleanest technique. I think we'd all be on the same page here: the I/O work is canceled, resources are disposed, etc.
The next cleanest is directly implementing the timeout yourself- for example, Assert.That(process.WaitForExit(10_000)); instead of process.WaitForExit(), or the task.WithTimeout extension method I seem to have written six times this year.

Beyond that it gets tricky, because the next cleanest thing to do is flush the current test results back to the runner and kill the process. This would require careful collaboration between the runner and the framework.
So moving on from that, the next cleanest thing is to leave the thread running for the rest of the lifetime of the host process. I actually think that's preferable to aborting the thread, since global state can be corrupted to an arbitrary degree due to so much code having no need to be hardened against thread aborts, and you risk affecting the other tests.

We don't want you to lose your test results. Can we have some specific examples of code that hangs tests, where you can't work around it with using (var cancel = new CancellationTokenSource(10_000)) and passing the token, or Assert.That(foo.Wait(10_000))?

[abbotware]

I am not arguing that Co-op cancel is cleanest... Its just not realistic ... Not all code in the world is on the new Async model (nor does it follow it correctly even if it is.. ie 'Bad Actor' scenario)... Not all code in a unit test is under control by the writer of a test....

I am suggesting the following:

1. Provide Co-Op Cancel
2. Failback to leaking a task/thread if the Co-Op Cancel is not handled in a specified amount of time

[Test]
[Timeout(1000)]
Task [async] TestMethod(Cancellation Task)
//use version 1, fallback to version 2

Caveat: you can't provide Cancelletation Token to these types of tests

[Test]
[Timeout(1000)]
Void TestMethod()
//use version 2

[Test]
[Timeout(1000)]
Task [async] TestMethod() 
//use version 2

[CharliePoole]

@abbotware I agree that tests need to opt in to cooperative cancellation. How they signal that is a matter of API design.

We can't however default to leaking test threads under NUnit control. If you mean threads started by the tests or the SUT, then yes, that would make sense.

[abbotware]

So what happens when a test is stuck.... ? you lose all the results of the test run

[CharliePoole]

Do you mean stuck in the sense that we can't even kill it? I think that's something to deal with if there is a use case where it can happen without the test writer explicitly trying to make it happen.

I know several tricks that will make a thread refuse to Abort. But if I use those tricks in my test, it seems like the failure to cancel is my own fault. Doctor says: "Stop doing that!"

[CharliePoole]

Also... in the NUnit design, if we leak the hanging thread, we have lost one of the test worker threads. If it happens a few times, we lose all those threads and no more tests get executed. That's why I said we can't just "leak" our own threads but can leak threads created by our users or by the code they are testing.

For threads that are particularly at risk, you can make NUnit dedicate a thread to the test, separate from the worker threads. That thread could be safely leaked.

[abbotware]

seriously? Doctor says: "Stop doing that!" -> what is wrong with this group (Nunit) - you are not the first person to make such an assumption that I have control over all code in question.. I am not going to repeat everything i wrote here; (https://gitter.im/nunit/nunit)

[abbotware]

you should assume a test will hang (murphy's law) - not that all code will do this.. but maybe a 3rd party library gets updated... and some new behavior happens... I want to be able to run my tests and see what is impacted... isnt that the point of unit tests / integration tests?

[CharliePoole]

@abbotware Sorry you are annoyed. I don't use Gitter, so didn't see your comment. However, I looked at it just now and it didn't seem to explain what you mean, just stated that you don't have control. Sometimes, people don't undertand one another because they simply come from different backgrounds, have different meaning for terms or hold different values. Getting mad doesn't make the communication work any better.

My own comment pertains to your test code and to nothing else. If a library has an issue, I think you should write your test code so as to deal with that issue. If this turns out to be a problem that many NUnit users need to deal with independently, then it's a candidate for a feature. But there is no open source (or other) project that entertains features for unexplained needs, at least not as far as I know. If we don't understand it, we try to understand first.

I don't know your background, of course. But the base assumption of NUnit (and pretty much all of the xUnit patterned frameworks) is that your test code is under your control, while the code you are testing may or may not be. In fact, there are three levels of code involved: test, SUT and third-party libraries. In the ideal situation, you have control over the first two, because you are writing the SUT as well. But sometimes that isn't so. By definition, of course, you don't have control over third-party libraries but you might have control over how you use them.

I'm spelling this out because we see many more people these days who say they do not have control over (even) the tests. This is a new audience for me, and I'm not sure I know how to deal with it. If that is your situation, my apologies. But you have posted enough here that I'm led to assume you are in fact a software developer with a fair amount of experience. If you are willing to explain why or how you don't have control over your tests, I want to hear more because it may help us deal with other people who are not able to articulate the problem as well as you.

[CharliePoole]

@abbotware So...

1. You have a working test
2. A third-party library is updated
3. That test now causes NUnit to crash without information that helps you localize the problem.

That's a very specific issue. Thanks.

An approximate solution is to see what test was running when the crash occured. Depending on how you run tests (console, VS, etc.) there are ways to find that out. The solution is approximate, however, because a test can start a thread that impacts NUnit long after that test terminates.

We have always had this issue - i.e. for the 18 years of NUnit's existence - but it becomes more prominent as more people use multi-threading and as NUnit itself runs in parallel.

Existing solutions:

1. Examine the internal trace log
2. Use test label display on the console to see what was running
3. Use non-parallel execution so the above outputs are easier to interpret

None of them are 100% satisfactory, so what would you like to see beyond them?

Finally, does this actually relate to the non-availability of TimeoutAttribute under .NET Core or is it a separate issue?

[abbotware]

uh.. so no attribution for the code I wrote? kinda looks like it was just copy and pasted there...

[CharliePoole]

@abbotware There's no PR yet. What would you like to see if your code is used?

[rprouse]

@DaiMichael I have sent an invite to the contributors team, welcome.

@abbotware we don't normally add attributes to individual authors in the code, we generally only provide recognition through the contribution graph for the project for people that contribute PRs. We could add a small comment in the code if you prefer, but please state that up front. Usually when people provide code samples for us to fix and issue or provide an enhancement, it is to satisfy their own needs.

[abbotware]

Considering this issue has been around for how long?... if it were not for my sample code, this PR would not have been made.. and even more so, the sample code is/was just copy and pasted into someone else's PR. I don't care about the code being copy and pasted since I more than expect that to happen (although it did have a copyright header!), and am glad this is getting merged into NUnit proper... But a little recognition for my effort and or/contribution would be nice.

for example, i even attributed the code for 'Timeout After' to where I found it:
https://github.com/abbotware/nunit-timeout/blob/master/Abbotware.Interop.NUnit/TaskExtensions.cs

[CharliePoole]

@abbotware Again, no PR has been submitted. I can only guess, but it seems unlikely that anyone other than you will submit a PR using your code, given your comments.

That said, you did release your code on GitHub under the MIT license, which allows its use in other works without attribution.

NUnit itself is released under the same license. People take parts of it, even large parts, and incorporate it into other works all the time. It would be silly of us to complain since that's the license we chose.

I'm venting my own personal views here, BTW. They have no connection with the NUnit project's views.

[abbotware]

@CharliePoole - Sorry thought the 'fixes #1638' was a PR request...

Any how - I just thought there was some form of minor recognition (although not required from the license) for helping resolve this old issue given that the method I proposed looked like it was being submitted.

It seems like this is just becoming a bigger deal than it needs to be: Please, just forget I asked. I'll submit a full PR next time for 'proper' recognition.

[CharliePoole]

@abbotware No, that's just a fix to the user's own fork of nunit. It's noted in this issue because he referenced it in a comment.

Your point about attribution is well taken. It's just that you seemed to be addressing it to the NUnit Team, which hasn't actually used the code and isn't even considering it right now, since no PR has been submitted. Technically, we could go out and take the code from either repo and make use of it, but that's not something we normally do.

Curiosity question: did you consider submitting it yourself? You have been very active in this discussion and that would have seemed like a logical outcome.

[abbotware]

@CharliePoole - Given I wrote it as plugin, it seemed more appropriate for an 'NUnit.Contrib' type project than NUnit proper. Kudos for having great extensibility!

I did consider a PR, but wasn't sure where to begin... Now though, I kinda have an idea (thanks to @DaiMichael)

[CharliePoole]

To do a PR...

1. Start with a branch that is based on the latest NUnit master plus the change you are submitting. One thing at a time is best. If you make two different changes, the team may like one but not the other, which slows you down.

2. In your browser, list your branches on GitHub. There's a button to click to submit a PR.

3. Fill out the PR including "Fixes #nn" or "Closes #nn" in the description, where nn is the issue you are fixing. Add any non-obvious info about the PR.

4. Two different NUnit committers need to approve your PR before it will be merged.

That's all. GitHub PRs are really brilliant!

[abbotware]

To Clarify - I meant where in the NUnit code :-)
Given this is tied to build related matters: Net Core vs Framework etc

[CharliePoole]

Ah! Ok then. 😄

[jnm2]

Unless we're given special permission by @abbotware, my understanding of the MIT license is that we must preserve this licensing notice in our source if we copy their code:

https://github.com/abbotware/nunit-timeout/blob/master/LICENSE (@abbotware is the double-'w' intentional?)

If we don't want to have to worry about this, we should start from scratch.

[CharliePoole]

That's why we wait for a PR. By submitting a PR, the author agrees to donate the code to us, which is different from our simply finding it on GItHub. Absent a contribution agreement (which we probably ought to have) the usual assumption is that the work is that of the submitter or they are authorized to submit it and that they are offering it to us under the same license that we use. If they submit it with something in the file that contradicts that assumption, we can ask them to change it or accept it as is. In general, we ask for it to be changed but we have a few bits that we keep copyright to the original authors, such as the .NET 2.0 implementation of Linq classes.

The problem is that many people put a copyright statement on a file intending it to refer to the file. We have used it to refer to the copyright of the entire work, e.g. the NUnit framework. Supposedly, we were going to get some help from the .NET foundation in these matters but it never seems to have come.

[Diaver]

https://github.com/abbotware/nunit-timeout <- sample code for creating a .Net Core Timeout attribute

@CharliePoole can we add that code?
I checked, it is working great.

[CharliePoole]

@Diaver I'm no longer the person to answer such questions but, as stated earlier, the @nunit/framework-team needs to make a decision. Guys?

[mikkelbu]

There is a PR under way (see #3027), but nothing has happened since end of september (and as far as I can tell @DaiMichael has not been active on github since then), also there was also the issue about copyright.

[abbotware]

I relinquish all license / copyright claims (thought that was clear in my previous comments) - Just add/merge the code to NUnit :-)

[rprouse]

Thanks for the clarification @abbotware.

[sebastienpa]

@DaiMichael @jnm2 seems like you guys are getting close to merging PR #3027 to fix this issue, are you still working on this?

[abbotware]

@sebastienpa - Not sure what is going on with the PR, but I wrote a plugin/extension as a proof of concept a while ago and released a working version as a nuget. if you don't want to wait for the PR you can use "Abbotware.Interop.NUnit" nuget