CI timeout: NUnit.Framework.Assertions.CollectionAssertTest.PerformanceTests

[jnm2]

It's only .NET Framework 3.5 for most CI errors, it seems.

1. Failed : NUnit.Framework.Assertions.CollectionAssertTest.PerformanceTests(System.Linq.Enumerable+d__b1)
   Elapsed time of 174.8091ms exceeds maximum of 100ms
2. Failed : NUnit.Framework.Assertions.CollectionAssertTest.PerformanceTests(System.Collections.Generic.List`1[System.Double])
   Elapsed time of 201.1894ms exceeds maximum of 100ms

https://ci.appveyor.com/project/CharliePoole/nunit/build/3.9.0-dev-04713#L1241

Just need to bump up the timeout, I think.

[jnm2]

Going to have to fix before 3.10 because it's as high as 312ms on .NET Core on Travis and it's keeping four tests from passing each time.

@nunit/framework-team Should I raise it to 1000ms, or make the tests explicit, or send a test parameter to skip when running in CI?

[CharliePoole]

What about using warnings instead of asserts? As a performance test, we may not want it failing our build anyway and with a warning, we could keep the values more reasonable.

The downside is if we just stop worrying about it because it's only a warning.

[jnm2]

I really like that idea!

[jnm2]

Here's yet another use case for Has.MaxTime... converting the [MaxTime(100)] failure into a warning will not be pretty.

[CharliePoole]

Ah, sorry! Thought we were talking about an Assert on the duration...

I'd do an adhoc fix here and then discuss possible enhancements...

Adhoc:

1. Mark as Explicit OR
2. Significantly increase the maxtime OR
3. Remove MaxTime attribute and use stopwatch to measure and warn

Enhancements

1. Has.Maxtime as you suggested OR
2. Has.Duration.LessThan is possibly more general but maybe the generality isn't needed OR
3. MaxTime as modifier, similar to After

I hate to see us add a feature without a bunch of discussion first, in order to make sure we don't later come back and make a breaking change.

[CharliePoole]

Possible Enhancement 4:
Add a Warning or Failure property to MaxTimeAttribute

[jnm2]

I worked around it with a helper method just to unblock #2609: 5c45bef

But if you want to drive a different fix before the PR is merged, just let me know and I can drop that commit.

[CharliePoole]

Nope. It looks good to me for fixing this problem. If we don't have an issue for creating a more granular way to measure duration, we should probably create one. I'd like it if we looked at alternatives rather than being biased by the existing implementation. 😸

I do like the general idea of having a helper class with constraints we need for our tests. I'm thinking of Has.Result as a possibility for checking the result of an assertion.

[jnm2]

If we don't have an issue for creating a more granular way to measure duration, we should probably create one. I'd like it if we looked at alternatives rather than being biased by the existing implementation.

As you know we have mine, but the more alternatives we look at the better as far as I'm concerned. Do you want to start a higher-level issue so we can start a new discussion?

[jnm2]

Another alternative to compare is a more widely applicable attribute [ErrorsAsWarnings] which I think has come up multiple times. That won't help with granularity but it's probably the only way you'd be able to warn for max-time other than a constraint.

[CharliePoole]

Actually I've lost track of that issue.

[jnm2]

Here it is: #2021

[Me] Here's yet another use case for Has.MaxTime... converting the [MaxTime(100)] failure into a warning will not be pretty.

😂 I forgot I said that... turns out I like making things pretty 😜

[CharliePoole]

But isn't that one about Timeout?

[jnm2]

It asks to consider both Has.MaxTime and Does.Not.Timeout.

I've since decided that timeout is only for logic and to prevent CI from running for an hour, never performance testing. So I personally have no need for that to be granular though we could do it for consistency with MaxTime if we wanted. Also, the work @bmolnar2 kindly contributed is only for Has.MaxTime as per the discussion there. Everyone seemed to lose steam on that issue though...

[CharliePoole]

Both Rob and I pointed out that TImeout and MaxTime already have meaning in NUnit and the meanings are different. We all (I thought) decided to leave that one about Timeout, and the title still indicates "Dynamic TImeout." Can you point me to the PR for Has.Maxtime?

Restating the (existing) definitions:

• MaxTIme implies that the operation runs to completion and then we look at the duration.
• Timeout implies that the operation is cancelled if it runs longer than a certain duration.

Both exist as attributes which gives them the scope of an entire test.

Both could exist at some more granular level (one or more statements) but I don't see much use for Timeout in that context. Maxtime OTOH is potentially useful for "poor man's performance testing."

If you agree that the focus should be on MaxTIme, I can create a separate issue or we can try to clearly re-dedicate the existing issue to be about MaxTIme and not TImeout by renaming it and adding a comment that restarts the discussion... hopefully.

[jnm2]

Has.MaxTime PR: #2513

Yes, I agree. I'd prefer if you'd close the long and confusing #2021 and start fresh with the focus on only MaxTime. We could keep @bmolnar2's PR open in case we end up using it as the solution.

[CharliePoole]

See #2616