v3 - Revert "Indicate that SetupFixtureAttribute should not be used on base-classes"

[stevenaw]

Fixes #4556
Relates to #4554 #3937 #4158

Based on the discussion in #4554 and #3937 it seems as though nunit has traditionally not supported SetupFixture to work on a base class, though it doesn't seem to appear in the docs. The last time I can find an explicit discussion it appears that this was intended to not work. It appears to have accidentally started working at some point, likely in 3.13.3.

We explicitly turned off the Inherited flag on the attribute in 3.14 and 4.0, which inadvertently seem to have broken some users of 3.x who had built an inheritance structure around SetupFixture.

I've expressed hesitation about reverting the change in 4.0 unless we decide as a team it should work but in the interest of compatibility I've opened this PR for us to consider reverting it on 3.x. There's pros and cons to merging this. Pros would be minimizing accidental breaking changes in 3.x while the cons would be explicitly reenabling a behaviour we seem to want to discourage.

@OsirisTerje @jnm2 I'd like to hear your thoughts on this. I'm also happy to have this PR close without merging if we want to actively discourage this pattern in 3.x. The workaround for users who hit this in 3.14 is simply to add the attribute at each level of their inheritance hierarchy.

[manfred-brands]

In my opinion having a class execute things without having to specify SetupFixture on that class could lead to this being run unexpectedly.

[OsirisTerje]

I had to ask @CharliePoole about this, and I got this answer back from him, which I think clears up the confusion.

So before I get further into this, I'm wondering if my original point was perhaps misunderstood...

The idea wasn't that SetUpFixture classes can't be inherited from but that they can't be inherited
by TestFixture classes. I wonder if that's clear in all the current discussion.

Simple explanation: some attributes provide a class with an identity so the class isa TestFixture
or isa SetUpFixture. These are two different things and behave differently. Originally, in the framework,
I think we felt everyone understood that and would not use inheritance to make the same class be both a
SetUpFixture and a TestFixture (or TestFixture base class).

So... a TestFixture is a class, which contains tests.

A SetupFixture is a class without any tests, which provides onetimesetup for some group of TestFixtures.

By mixing the two concepts, various things can go wrong. Most obviously, there's the fact that OneTimeSetUp
and OneTimeTearDown in a SetUpFixture has different semantics from the same attribute in a TestFixture.

I can go more into detail but please let me know if I'm on the right track in terms of what people are thinking
the "don't inherit" warning was intended to mean.

Charlie

From a maintainability view, it would be good to always add the attribute to inherited classes, but - it is not a requirement. It could be an advice.

More important, attempts to add a TestFixture attribute to a class derived from a SetupFixture should be as prohibited as we can. I would assume this could be done by the Analyzer.

And based on what Charlie writes above, we should revert this, because there is nothing in the framework that disallows you from inheriting from a SetupFixture.

[manfred-brands]

Thanks for clarifying. Yes then it should we reverted. Although we can add a rule to the analyzer to warn at edit time, the framework should also throw an error at runtime if any class with tests derives from a SetUpFixture annotated class

[OsirisTerje]

, the framework should also throw an error at runtime if any class with tests derives from a SetUpFixture annotated class

Yes, this makes sense, we should do that. But, we should still have it in the analyzer so the developer gets an "early warning", or "early error".

[manfred-brands]

, the framework should also throw an error at runtime if any class with tests derives from a SetUpFixture annotated class

Yes, this makes sense, we should do that. But, we should still have it in the analyzer so the developer gets an "early warning", or "early error".

Yes, I meant we need to do both.

[OsirisTerje]

I'll also move Charlie's comment into the documentation for SetupFixture. It is a very good clarification.

[stevenaw]

Thanks for helping confirm that @OsirisTerje
Sounds like we should also revert in 4.0. I can get that PR setup too