Alternate Timeout API for dynamic timeout intervals

[jnm2]

There is only one way to specify a timeout as far as I can tell: TimeoutAttribute. The fact that it's an attribute means that the timeout time must be a compile-time constant.

However, I am searching for a way to override that timeout if a given environment variable is present. If TF_BUILD is present, I want to use 7000. Otherwise, I want to use 3000.

If you want to stick with attributes, a conditional timeout attribute could be added such as TimeoutIfEnv("TF_BUILD", 7000). However a more flexible API might be an assertion, taking a page from Throws:

var timeoutMs = Environment.GetEnvironmentVariable("TF_BUILD") != null ? 7000 : 3000;
Assert.That(() =>
{
    // ...
}, Does.Not.Timeout(timeoutMs));

The non-aborting version could be Has.MaxTime.
There could be a TimeSpan overload as well.

What are your opinions?

[JustinRChou]

I am trying to figure out what other cases would you need a dynamic timeout.
Though it would be interesting for performance testing.

[jnm2]

I am trying to figure out what other cases would you need a dynamic timeout.
Though it would be interesting for performance testing.

Parameterized tests might calculate the timeout based on parameter values.
In fact, that allows the timeout to be one of the parameters in a list of TestCase attributes.

[JustinRChou]

So would it be equivalent to doing something like

        [TestCase("a", "b", 1000)]
        public void TimeElasped(object a, object b, int timeout)
        {
            System.Diagnostics.Stopwatch watch = new System.Diagnostics.Stopwatch();
            watch.Start();
            RunMethod(a, b);
            watch.Stop();
            Assert.That(watch.ElapsedMilliseconds, Is.InRange(timeout - 50, timeout + 50));
        }

[jnm2]

@JustinRChou Yes. That's the Has.MaxTime variant. The Does.Not.Timeout variant would have to also abort the thread to keep par with TimeoutAttribute.

[jnm2]

Here's a prototype of the Has.MaxTime variant: https://gist.github.com/jnm2/f5c4e2224114d23e566467dbf1467128
Match definition: https://gist.github.com/jnm2/03e597d81027d64b033aef9620847f4c

[bmolnar2]

I'd be happy to check out this one. Ready to dive into open-source contribution. :)

[jnm2]

@bmolnar2 Awesome! It's yours. I'm assigning myself to the issue to keep track of it. Let us know if you need anything.

[CharliePoole]

If I'm reading the history right, this got made into a feature without significant discussion and without selecting among the alternative implementations. The first is important for all features (we have to agree we need them) and the second for anything we mark as help wanted.

[jnm2]

I don't recall significant discussion on every enhancement, but what sort of discussion would you like to have? Now that we're here, are you asking for @bmolnar2 to leave this for now? If so, it would be nice if the discussion didn't drag on. 😃

[rprouse]

This issue has been open for 10 months and has been help wanted for 4 months. I would assume that if there were no objections to the enhancement in that time that no new objections would be coming this late in the game. I do however agree that we should select between the two variants before @bmolnar2 does any work on this. Personally, I prefer the Has.MaxTime variant. The other variant would allow Does.Timeout which to me would be an ant-pattern.

If anyone has objections, let's try to sort them out quickly.

[CharliePoole]

Basis of my comment was that @jnm2 proposed, @rprouse made it a discussion item and @jnm2 accepted it as a feature without discussing the merits.

I construe making something a discussion item as meaning we aren't sure we want to do it.

We don't do this with every feature but when somebody - especially the project lead - says it needs discussion, then it means the desirability of the feature is up for discussion and the person who wants it to happen should try to get concurrence. That didn't happen here - it just got promoted to a feature without anyone noticing.

If @rprouse sees no problem, I'm not raising one. I'm just describing what I would have expected if I were the person who put a hold on this by marking it for discussion. But if the pipeline means something else I'd like to see it spelled out so I can follow the rules.

[jnm2]

My guess is @rprouse saw that there was discussion and labeled it as such. Maybe I was mistaken, but I don't see an ask for more discussion in the history of this issue.

In hindsight, I should have probably tagged the framework team and asked for more comments. Yes, I agree I should ask for more than one person's vote of confidence before I come back, see support, and move it forward. It felt awkward to say, "@OsirisTerje, I see you've used the thumbs-up reaction. Would you please leave a comment also saying you support this so that I can follow the rules and move this to the backlog?"

From my perspective, watching a lot of issues, I object that the expectations are too grey to be asking for a high level of precision; I've seen issues move forward with less interaction. Since process and precision is important, let's aim for success and at least spell out explicitly what steps are to be followed at every turn. And I wish we could have these meta-discussions out of issue because frankly they are cross-cutting for us and mainly irrelevant to anyone waiting for the feature.

[CharliePoole]

I agree we should minimize meta-discussion on issues but when the process hasn't been followed and it comes up, there's no other way to do it. Unfortunately, we don't have a good place to have such discussions without going non-transparent via email. And it's better to have the discussion than waiting for the PR and then having one of us say "we don't need this feature." So, for all those reasons, I'm continuing here.

From our Wiki...

is:idea An idea about something we might do. We discuss these until they are either dropped or turned into a feature or enhancement we can work on.

Discussion These are items that require some discussion, either about whether we want to do them or how they should be implemented. Some items here may require confirmation or design as well.

As I always did it and as I understood Rob to be continuing, when the project lead applies the combination of those two items, he is putting the brakes on development. If @rprouse is now using them in a different way, then I want to know that because otherwise I can't collaborate successfully on the project.

There's no reason you - the original proposer - can't be the one to re-activate it as a backlog item, but you're supposed to have conducted the discussion and reached consensus first. If we are not clear on this, then the logical extension is that any two committers can add any feature whatsoever to NUnit.

Regarding "thumbs up", one never knows what they mean unless they are applied to a simple declarative statement. If you posted a comment that said "I think this has been discussed enough and I suggest we go ahead with option B" then I'd take thumbs up as agreement to that comment. That wasn't the case here.

In any case, there has never been any discussion of whether we want a feature like this. All the discussion that took place was about how to implement it, as if it had been labelled design rather than idea. Is that's too picky? I don't think so - we all know the difference between "whether" and "how" to do something.

If you had called for all of us to chime in on this, I think folks would have responded. I think that it's up to either the proposer or the project lead to help ideas migrate to backlog items... possibly both. I know I would have chimed in and - in fact - would have favored moving ahead. This is a process issue for me and I used your issue to raise it because your reversal of Rob's categorization shocked me when I saw it.

[jnm2]

Points well taken. It's easy enough to slow down and ask more questions if I'm not sure. I can and will certainly do better.

[jnm2]

(And sorry @bmolnar2 for the drama I've caused on this one.)

So @nunit/framework-team, we can't think of a good way to make Does.Not.Timeout work because we'd have Does.Timeout as Rob points out. Does anyone have an objection to us moving forward with Has.MaxTime?

[CharliePoole]

@jnm2 Thanks for your gracious reply. You are not actually owed 100% of the blame here, but that's getting too much into the meta.

Since this issue, which you created back in January, only has my attention now. I'll start at the beginning.

What you are suggesting is some way to do timeouts right in the test code, similar to how we replaced ExpectedExceptionAttribute with Throws - except, of course, we would keep TimeoutAttribute as well.
I agree that we need something like that. If we all agree - as I suspect we do - then that moves us from the idea to the design stage: What should a user type into their program to get this feature.

You proposed two alternative syntaxes: Timeout and MaxTime. I'm leaving out the static class prefix because whatever you use will be available in many different constraint expressions. For example...

Does.Timeout
Does.Not.Timeout
Has.Maxtime
Has.No.Maxtime
Does.Not.Throw.And.MaxTime

It's possible to set things up so some of those don't compile, but I think we need to fully specify what will and won't work. It would be OK with me if we allowed only the use directly after Has for the initial implementation, because the other stuff is not really for a new contributor.

We don't use either MaxTime or Timeout in syntactic expressions now, but we do use them for attributes. The attributes have entirely different meanings. Timeout says cancel if this takes more than a certain amount of time. MaxTime implies a performance test. It reports the actual time taken and compares it to a target, failing if the target is mixed. We can't use them interchangeably so I guess you have to clarify which of the two behaviors you are looking for. In reading through the comments, I see both suggested at different points. Once we clarify what is desired, we can pick one of those words or - if neither of them fits in fluently - some third word. But the behavior seems to be the first thing to decide.

[bmolnar2]

I started to go through the various documentations and also gave a try on building the solution. On the latter one, several error occurs, when I build the solution. I may miss some piece of information on that part (after reading Building.md file several times).

Can someone reach me out on mail? I made mine public, so you can do.

I'll start human static code analyzer in the mean time. :)

[jnm2]

@bmolnar2 We're in the middle of a discussion on when we can fix various aspects of the build situation: #2380

For now the workaround is to run .\build.cmd -t build before opening the solution in Visual Studio.

[bmolnar2]

Well, it took me while (also did a windows install), but could build the solution, however only after unloading NetStandard13 and NetStandard16 projects. So I guess, there is are some problems.

But anyway, I can spend some quality time finally to get familiar with the problem and the related code.

[jnm2]

@bmolnar2 I'm certainly sorry to hear it took a lot of time for you. Just curious, what's your VS version and update?

[bmolnar2]

It's 14.0.25431.01 (2015) with Update 3.
I see nunit's solution specifies 15.0.26430.6.
Shall I updated?

[jnm2]

The theory is you shouldn't have to update, but the .NET Standard stuff is pretty confusing. If you think it would make your life easier, updating may solve the issues you're seeing. If you're happy with what you've got going right now, keep it. =)

[bmolnar2]

So, there are a bunch of tests failing in a clear master branch. Is that normal?
I am mostly suspicious about failings like:

Test Name: CodeShouldNotCompile("Is.And")
Test FullName: NUnit.Framework.Syntax.InvalidCodeTests.CodeShouldNotCompile("Is.And")
Test Source: C:\dev\GitHub\nunit\src\NUnitFramework\tests\Syntax\InvalidCodeTests.cs : line 56
Test Outcome: Failed
Test Duration: 0:00:00.001

Result StackTrace:
at System.IO.__Error.WinIOError(Int32 errorCode, String maybeFullPath)
at System.IO.FileStream.Init(String path, FileMode mode, FileAccess access, Int32 rights, Boolean useRights, FileShare share, Int32 bufferSize, FileOptions options, SECURITY_ATTRIBUTES secAttrs, String msgPath, Boolean bFromProxy, Boolean useLongPath, Boolean checkHost)
at System.IO.FileStream..ctor(String path, FileMode mode, FileAccess access)
at System.CodeDom.Compiler.TempFileCollection.EnsureTempNameCreated()
at System.CodeDom.Compiler.TempFileCollection.AddExtension(String fileExtension, Boolean keepFile)
at System.CodeDom.Compiler.TempFileCollection.AddExtension(String fileExtension)
at Microsoft.CSharp.CSharpCodeGenerator.FromSourceBatch(CompilerParameters options, String[] sources)
at Microsoft.CSharp.CSharpCodeGenerator.System.CodeDom.Compiler.ICodeCompiler.CompileAssemblyFromSourceBatch(CompilerParameters options, String[] sources)
at System.CodeDom.Compiler.CodeDomProvider.CompileAssemblyFromSource(CompilerParameters options, String[] sources)
at NUnit.Framework.Syntax.TestCompiler.CompileCode(String code) in C:\dev\GitHub\nunit\src\NUnitFramework\tests\Syntax\TestCompiler.cs:line 61
at NUnit.Framework.Syntax.InvalidCodeTests.CodeShouldNotCompile(String fragment) in C:\dev\GitHub\nunit\src\NUnitFramework\tests\Syntax\InvalidCodeTests.cs:line 61
Result Message: System.UnauthorizedAccessException : Access to the path 'C:\Windows\System32\22ow4xhl.tmp' is denied.

However tests like this are confusing too:

Test Name: NonPublicTest
Test FullName: NUnit.Tests.Assemblies.MockTestFixture.NonPublicTest
Test Source: C:\dev\GitHub\nunit\src\NUnitFramework\mock-assembly\MockAssembly.cs : line 149
Test Outcome: Failed
Test Duration: 0:00:00.0000001

Result Message: Method is not public

[CharliePoole]

Are you trying to run tests on NUnit using the NUnit VS test adapter? That won't work very well for a number of reasons.

The tests you are seeing fail are run by NUnit as a part of testing NUnit. We need failing tests, of course, in order to see that they fail correctly.

The real tests are found in nunit.framework.tests.dll and nunitlite.tests.dll. To run all tests for all supported runtime at the command-line, use 'build -t Test'. For testing as you work, I'd set up one of the nunitlite.runner projects as the startup project.

[bmolnar2]

Yep, it was the adapter.
Worked from CLI, thanks.

[jnm2]

@bmolnar2 Asking for the sake of making things smoother for first-time contributors: could we have done something to make it easier for you to find https://github.com/nunit/nunit/blob/master/BUILDING.md#running-tests?

[bmolnar2]

A note under the Running Tests section could help on the latter.
Also mentioning the VS2017 (15) dependency for the VS build would save some time, I guess.
Otherwise, so far so good docs. :)

[CharliePoole]

@jnm2 Oddly, that link seems to tell people to run the slow-tests assembly, which is not a top-level test.

@bmolnar2 The Running Tests section of the docs is oriented toward users of NUnit running their own tests. So it doesn't seem like a good place to tell developers of NUnit how to run tests. Maybe a note and a link, however.

[jnm2]

@CharliePoole You're right, I'll fix that.

[CharliePoole]

@rprouse I raised questions on whether we need this feature. Subsequently, we had meta-discussion of our own procedures and answered technical questions from the person working on it, who has now submitted a PR. I think this illustrates what an happen when we allow things to get away from us.

I realize that you expressed a preference for this feature, in the form of Has.Maxtime but it's pretty unclear how that is supposed to work. Of course, I can read the code in the PR and see how the contributor thinks it should work, but he was left to figure that out on his own and we are consequently in the position of reviewing feature desirablility, API design, internal design and coding all at the same time. If you look at the scope of changes in the PR, the designation of this as "Good for first time users" is a bit of a laugh.

On the substance of the feature: I think we all have agreed that there's a need for some more granular way to test the maximum time that things take within a test. We could probably have spent more time on talking about what that should look like, but other kinds of discussions dominated and most of that discussion was between two people.

I came in late and raised questions, which I hoped would cause you to block the issue. That hasn't happened, so I've "requested changes" on the PR without actually specifying the changes I want. I have asked some questions as well, about what seem to be strange choices in the code.

If we can have a discussion and come to an agreement on the shape of the feature, I'm happy to do a true review and merge code that implements it in that shape. If we don't do that, you can merge it in spite of my negative review and I'll have no further comment.

[rprouse]

@CharliePoole has a point about the semantics of MaxTime vs Timeout in other places in our code and that they should match the semantics of the constraints. Reading through the PR, @bmolnar2 has maintained the correct semantics and does not time out the test, but reports if the action takes more than the correct amount of time. I think that is good and a useful feature, but this issue talked about timing out.

I can see wanting to assert that something runs in a given amount of time (through an assert, or through the MaxTime attribute), but I have found that timing related tests like this are extremely fragile and tend to break randomly because of unrelated factors like updates running, other processes, sunspots, etc. 😄

Because of that, I am not sure that this implementation is a good addition. I can see the timeout as originally specified, but that is not how this is written. I made a mistake in preferring Has.MaxTime without thinking of the conflict with MaxTimeAttribute. The conflict is evident now because that is how the PR was implemented.

I am still okay with original syntax of the issue, but only if Does.Timeout or other combinations are not valid. That is part of the problem with the constraint syntax, there are so many permutations to think about. Could someone write Does.Not.Timeout.And.Contains("garbage")? Based on that, do we need a new root, Doesnt.Timeout for this? I can't think of anything better (and don't really like that one).

So, my thoughts,

1. The PR as it stands is a no-go. This issue was for a more granular [Timeout] not [MaxTime]. If the PR can be re-jigged appropriately, then 👍, but...
2. Has.MaxTime implies the current implementation and Does.Not.Timeout is problematic. Someone needs to propose something appropriate before we move forward. I can't think of anything.
3. Anything proposed needs to take into account which other constraints might be chained. In this case, ideally none. That said, you can chain most things off a Throws like Assert.That(() => throw new Exception("garbage"), Throws.Exception.And.Contains("garbage")) and it just fails with an InvalidOperationException, so we shouldn't go overboard. I think things that you would expect to work should work.

@bmolnar2 I am sorry for this confusion, I should have been tracking the issues more closely.

[CharliePoole]

I see two places where this went awry.

First, in talking about MaxTime and Timeout as variants of the same thing. Of course, they both specify time limits and so are superficially similar. But the first just times the test, reporting a failure if the max time is exceeded. The second requires additional infrastructure. Some thread has to be waiting to Join on the test thread with a timeout specified. So they are really quite different animals.

Second, in making this a constraint... or in only considering Constraints as alternatives. It sounds like either could be done by using an Assert that takes a block of code, like

Assert.TimeOut(  () => // Or MaxTime
    {
        // Code being timed
    },
        timeSpan // or time in ms
    );

Alternatively, we could try to solve @jnm2 's original problem by making an enhancement to TimeoutAttribute. Both of those approaches seem lots simpler.

[jnm2]

I have one alternative proposal since everyone seems to like Does.Not.Timeout(int) so long as it is not composed together with other constraints.

 public static class NUnit.Framework.Does
 {
-    public static Constraints.ConstraintExpression Not { get; }
+    public static Constraints.DoesNotConstraintExpression Not { get; } 
 }
 
+public sealed class NUnit.Framework.Constraints.DoesNotConstraintExpression : ConstraintExpression 
+{
+    public Constraint Timeout(int milliseconds);
+}

This gets the best of both worlds I think and isn't a big change or a breaking change.
What do you all think?

[CharliePoole]

That allows you to write things like...

Does.Not.TImeout(500).StartsWith("Hello!");
Does.Not.Timeout(100).And.Equals(42);

Are you intending to support that? Frankly, I see no need. Again... see above... why does this have to be a constraint at all? An expression like

Assert.TimeOut(() => { ... });

Is consistent with expressions like Assert.Multiple syntactically and allows you to put anything you like, including asserts, inside the block.

[jnm2]

As Rob said above:

That said, you can chain most things off a Throws like Assert.That(() => throw new Exception("garbage"), Throws.Exception.And.Contains("garbage")) and it just fails with an InvalidOperationException, so we shouldn't go overboard. I think things that you would expect to work should work.

If we don't think that all the syntax under the Throws class is a problem, we shouldn't consider this a problem either.

That said, Assert.Timeout(TestDelegate, int) and Assert.Timeout(AsyncTestDelegate, int) would get the job done if we don't want to provide Assert.That constraint syntax. 👍

[CharliePoole]

It's a weakness of the syntax we use that we can't detect some errors until runtime. It's not peculiar to Throws. Something like Assert.That(42, Does.StartWith("4")) does the same thing.

However, we made a ThrowsConstraint because it allows valid chaining as well as invalid. For example: Assert.That(() => throw new Exception("garbage"), Throws.Exception.With.Message.EqualTo("garbage"))

In the case of Timeout, I don't see a great value in making it a constraint and thereby complicating the syntax from the user point of view. Giving Assert.Timeout the same structure as Assert.Multiple (plus an extra arg for the timeout) would have the advantage that you can apply a timeout to any bit of code, whether it's an assert or not.

Somewhere earlier in the discussion, performance testing was mentioned. This would not be for that purpose except as a sort of "performance smoke test". Really for a performance test, we want to be able to grab the timing at various stages and then apply rules to them in order to either fail the test or possibly issue a warning. Probably something for a subsequent issue in any case.

[jnm2]

We have a use case for warning, so that makes me want to go with a constraint for maximum flexibility. Since we can't make Constrant unchainable, perhaps we should go for Has.Duration.LessThan(ms)?

[jnm2]

Closing this issue in favor of a fresh look at #2616.