NUnit.Framework 3.13.2 introduced a breaking change that conceals problems with tests

[estrizhok]

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 for NUnit.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

[manfred-brands]

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]

@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]

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]

@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]

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]

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]

@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]

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]

@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]

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]

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 use GetCustomAttributes(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 IsAsyncOperation call, 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.GetCustomAttributes causes the test to be detected and run, but the attribute is silently ignored.

There are two problems with my catch and ignore approach.

1. In the IsAsyncOperation we will miss that this is an async operation and do the wrong things.
2. For nunit extension attributes we won't do the extension, or if there is more than one none of them.

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.TestMethodMetadata constructor which in turn is called from DefaultTestCaseBuilder.BuildTestMethod and NUnitTestCaseBuilder.BuildTestMethod

Catching 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]

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]

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]

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]

@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]

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]

@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...

1. You cannot know when or how often NUnit will construct a fixture, therefore do little or nothing in the contructor.

2. 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

3. 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.

4. 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]

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]

@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]

@manfred-brands Yes... "the caching of fixtures is analogous to what's happening here, but not exactly the same thing."