Repository navigation
NUnit.Framework 3.13.2 introduced a breaking change that conceals problems with tests #4096
Description
Before NUnit.Framework 3.13.2 running an assembly compiled from the following code
[TestFixture]
public class BrokenTestClass
{
[Test, Invalid] public void Test() { }
}
public class InvalidAttribute : Attribute
{
public InvalidAttribute() => throw new InvalidProgramException();
}
would issue the An exception was thrown while loading the test twice:
- once inside the xml returned from
ITestEventListener.Explore, - and the second time inside a
<test-suite type="TestFixture" ... />event forNUnit.Engine.ITestEventListener.
Since 3.13.2 this error is only issued during discovery, and is not issued during execution.
The new behavior broke R#/Rider test runner in that a broken test class no longer gets a clear indication of a problem, marking such test as inconclusive instead.
More importantly, the new behavior broke NUnit's own runner in that it no longer reports such errors, making problems of this nature very difficult to notice. For this reason over a 1000 tests weren't running in our CI over the past week without us knowing.
Below is the output of nunit3-console running an assembly with broken tests:
Copyright (c) 2022 Charlie Poole, Rob Prouse
Wednesday, April 13, 2022 3:49:53 PM
Runtime Environment
OS Version: Microsoft Windows NT 6.2.9200.0
Runtime: .NET Framework CLR v4.0.30319.42000
Test Files
c:\Users\jetbrains\source\repos\Tests.Sandbox\Sandbox.NUnit3\bin\Debug\net35\Sandbox.NUnit3.dll
Run Settings
DisposeRunners: True
WorkDirectory: C:\Users\jetbrains\Documents\59\bin\net35
ImageRuntimeVersion: 2.0.50727
ImageRequiresX86: False
ImageRequiresDefaultAppDomainAssemblyResolver: False
TargetRuntimeFramework: net-2.0
NumberOfTestWorkers: 16
Test Run Summary
Overall result: Passed
Test Count: 0, Passed: 0, Failed: 0, Warnings: 0, Inconclusive: 0, Skipped: 0
Start time: 2022-04-13 13:49:53Z
End time: 2022-04-13 13:49:53Z
Duration: 0.510 seconds
Results (nunit3) saved as TestResult.xml
Activity
manfred-brands commentedon Apr 13, 2022
This is similar to #4091 where the caching of test definitions has a side effect on very unusual user attribute definitions. Making a test fail by throwing an exception in an attribute doesn't sound right to me. I think that your attribute should behave more like the explicit or category attributes instead.
estrizhok commentedon Apr 13, 2022
@manfred-brands the intent of the attribute (from the sample) is not to make a test fail, but to demonstrate the problem. Of course, we do not use attributes this way, but an exception raised from an attribute made us completely miss that we have a problem with tests
estrizhok commentedon Apr 21, 2022
Nobody cares that errors get swallowed? I should add that having an attribute throwing exception on a single test, makes nunit not run all of tests in the test class.
@CharliePoole Could you please look at it?
CharliePoole commentedon Apr 21, 2022
@estrizhok Replying since you mentioned me! I no longer work on this project, but I'm sure someone on the @nunit/framework-team will take a look at this and respond. I suggest waiting a bit longer before concluding that nobody cares. :-)
manfred-brands commentedon Apr 21, 2022
It is not that nobody cares, but any program that would use reflection won't expect exceptions when reading a method's attributes.
I found an old article warning about that.
Having said this, I think NUnit as being driven by reflection has two options: ignore any exceptions from 3rd party attributes and run the test regardless or mark the test as non-runnable. How this gets reported by runners VS, ReSharper is another issue.
stevenaw commentedon Apr 21, 2022
I agree that swallowing exceptions is not ideal and I'm not a big fan of ignoring exceptions from attributes defined outside NUnit as it can be a common extension point for both other libraries or users.
@estrizhok Can you explain a bit more about how your attribute is used in a test to help me understand your use case? Does it implement an interface known to NUnit such as ITestBuilder which could defer the exception-throwing logic to somewhere outside of the constructor, and if so does that change the XML output for you?
estrizhok commentedon Apr 22, 2022
@stevenaw No, it's just a CLR attribute, has nothing to do with NUnit. The exception thrown was a simple ArgumentOutOfRangeException because somebody supplied an invalid argument to the constructor.
stevenaw commentedon Apr 22, 2022
Thanks @estrizhok one more question: is the attribute a built-in one within .NET or a custom-defined one you've made? I'm trying to understand how common something like this may be.
@nunit/framework-team thoughts? Is this supported or undefined behaviour, and would the change described above be considered breaking? Explicitly supporting exceptions from attribute constructors is an interesting situation, especially given the exceptions would happen at runtime but attribute args are compile-time constant. Do we know of any attributes in the BCL which may throw from a constructor?
I'm leaning towards agreeing with @manfred-brands here, but I'm curious what others think.
CharliePoole commentedon Apr 22, 2022
@stevenaw Yes, that's the question. It seems to me that an attribute ought not to throw in it's constructor and, in fact, that's how NUnit attributes are designed. If some argument or property needs to be checked for validity, it's generally not done in the constructor or property setter but in some method, usually one which is able to set the test as invalid and give a reason.
OTOH, NUnit is a test framework and it may be called upon to test badly written code. So it seems to me that it should do something reasonable if a poorly designed attribute, perhaps one provided by a third party, throws in its constructor.
It would be interesting to know when this error first became possible and why.
stevenaw commentedon Apr 22, 2022
Lots of discussion in this topic, I should've clarified my stance. I'm leaning towards that we should change this and provide a more descriptive reason for failure, like how it had been. I also think it will be clearer to treat tests with exception-throwing attribute constructors as not-runnable rather than to ignore the exception and run the test.
manfred-brands commentedon Apr 23, 2022
I played with this a bit. If I create a fixture where one test that has a non-nunit attribute that throws, none of the test are even discovered.
Then I wrote code that calls
GetCustomAttributes. This fails when the constructor of any attribute throws and we won't get the non-failing attributes either. However, when I useGetCustomAttributes(typeof(NUnitAttribute), true), it ignores any attribute we don't know how to handle and this doesn't even instantiate those attributes.Debugging Nunit, it fails in the
IsAsyncOperationcall, which asks for all attributes. Wrapping that in a try/catch seemed to have solved the issue in my example where my failing attribute has nothing to do with NUnit.Changing my attribute to implement an NUnit interface (
ITestAction) gets me back to no tests found.Adding similar protection to
MethodWrapper.GetCustomAttributescauses the test to be detected and run, but the attribute is silently ignored.There are two problems with my
catchand ignore approach.IsAsyncOperationwe will miss that this is anasyncoperation and do the wrong things.The alternative approach would be to catch the exception higher, and mark the method as non-runnable (or failed)?
All code is called from the
MethodInfoCache.TestMethodMetadataconstructor which in turn is called fromDefaultTestCaseBuilder.BuildTestMethodandNUnitTestCaseBuilder.BuildTestMethodCatching any exceptions there I now get a failing test with a message "Exception thrown during build".
That seems a better approach.
I will see if I can get a PR up with appropriate tests this weekend.
CharliePoole commentedon Apr 23, 2022
Although the title talks about an nunit 13.2 breaking change. I haven't seen anyone identify the commit where it was broken. Has anyone tried using git-bisect to determine this?
manfred-brands commentedon Apr 23, 2022
I did mention it on my first comment. Not have the hash handy, but when the caching was introduced all attributes were handled at the discovery phase, instead of again at the execution phase. PR #4034
CharliePoole commentedon Apr 23, 2022
Ah... I didn't notice the (re-)introduction of caching.
We used to do a lot of caching in V2, to ensure that no fixture got constructed more than once. We got rid of that in NUnit 3 because it was causing no end of troubles. It turned out that the caching was not helping performance as much as we imagined anyway, at least for well-written fixtures.
Could that be the case here?
stevenaw commentedon Apr 23, 2022
@CharliePoole out of curiosity, can you elaborate on the "well-written fixtures" you mention above, and the performance implications? As a newer involvee in the project, I'm often missing historical context.
PR #4034 was my first thought here too for this issue, though I had thought it was a 3.13.3 change. As far as performance benefits of the method caching, the PR described a 700x improvement, though I'm now curious if some of that could've been related to how the fixtures themselves were written.
CharliePoole commentedon Apr 23, 2022
So... I'm confused. According to GitHub 13.3 contains the caching improvement rather than 13.2. But also, GitHub says that 13.3 came out before 13.2!
CharliePoole commentedon Apr 23, 2022
@stevenaw Sure... bearing in mind that the caching of fixtures is analogous to what's happening here, but not exactly the same thing.
General guidance I've always given on writing test fixtures - both here and as a coach - includes...
You cannot know when or how often NUnit will construct a fixture, therefore do little or nothing in the contructor.
Stuff that should be done one time per fixture belongs in the OneTimeSetUp (formerly TestFixtureSetUp) because (1).
NOTE: This is required to be static in 3.0+ because we eliminated earlier caching
Constructors with arguments should not normally take instances of your own class objects as arguments. Rather, provide the raw data (e.g. strings, ints) needed to construct the class. Do this at the fixture level or test level depending on scope needed. This is required when using TestCase and desirable even when using TestCaseSource.
Worry about efficiency only after having measured a problem. Of course, that's true for other programming as well.
Working with any framework effectively requires figuring out how that framework works rather than fighting it.
People sometimes ask why points like the above are not in the docs somewhere. That's me. I always thought that such guidance belonged elsewhere... in articles, books, etc. rather than the docs. So when I was doing the framework, that's how we did it. Arguments can be made the other way as well. It may even be the case that programmers no longer read such things. :-)
stevenaw commentedon Apr 23, 2022
Thanks @CharliePoole that was more or less what I had thought you had meant, but glad I double checked 🙂
Appreciate the extra context around one-time setup, static, and caching
manfred-brands commentedon Apr 24, 2022
@CharliePoole The caching done here is not for the fixtures, but for the metadata, especially attributes which won't change at runtime (at least they shouldn't)
CharliePoole commentedon Apr 24, 2022
@manfred-brands Yes... "the caching of fixtures is analogous to what's happening here, but not exactly the same thing."