Add ability to execute test actions before SetUp or OneTimeSetUp

[sksk571]

There is quite an old discussion about the issue here: https://groups.google.com/forum/#!topicsearchin/nunit-discuss/testaction/nunit-discuss/IW7qXnYhIJ8

When test action attribute is applied on a class or base class level it should take precedence over methods marked with SetUp and OneTimeSetUp attributes.

Running a test in attached code block I get the following on the test level:

BaseSetUp
SetUp
BeforeTest (BaseFixture)
BeforeTest (Fixture)
BeforeTest (Test)
Test
AfterTest (Test)
AfterTest (Fixture)
AfterTest (BaseFixture)
TearDown
BaseTearDown

And the following on the fixture level:

BaseOneTimeSetUp
OneTimeSetUp
BeforeTest (BaseFixture - OneTime)
BeforeTest (Fixture - OneTime)
AfterTest (Fixture - OneTime)
AfterTest (BaseFixture - OneTime)
OneTimeTearDown
BaseOneTimeTearDown

This behaviour causes difficulties when you try to decompose a monolithic setup code in the base class and split it between reusable attributes and setup in the derived class. In this case there is no ability to execute test action code before SetUp. This should probably be changed to the following (or we at least need ability to override existing NUnit behaviour):

BeforeTest (BaseFixture)
BaseSetUp
BeforeTest (Fixture)
SetUp
BeforeTest (Test)
Test
AfterTest (Test)
TearDown
AfterTest (Fixture)
BaseTearDown
AfterTest (BaseFixture)

And:

BeforeTest (BaseFixture - OneTime)
BaseOneTimeSetUp
BeforeTest (Fixture - OneTime)
OneTimeSetUp
OneTimeTearDown
AfterTest (Fixture - OneTime)
BaseOneTimeTearDown
AfterTest (BaseFixture - OneTime)

Here is the test code:

    public class Action : Attribute, ITestAction
    {
        private readonly string _className;

        public Action(string className)
        {
            _className = className;
        }

        public void BeforeTest(ITest test)
        {
            Trace.Mark(PrepareMarker(test));
        }

        public void AfterTest(ITest test)
        {
            Trace.Mark(PrepareMarker(test));
        }

        private string PrepareMarker(ITest test)
        {
            string s = _className;
            if (test.IsSuite)
            {
                s += " - OneTime";
            }
            return s;
        }

        public ActionTargets Targets { get { return ActionTargets.Suite | ActionTargets.Test; } }
    }

    [Action("BaseFixture")]
    public class BaseFixture
    {
        [OneTimeSetUp]
        public void BaseOneTimeSetUp()
        {
            Trace.Mark();
        }

        [SetUp]
        public void BaseSetUp()
        {
            Trace.Mark();
        }

        [TearDown]
        public void BaseTearDown()
        {
            Trace.Mark();
        }

        [OneTimeTearDown]
        public void BaseOneTimeTearDown()
        {
            Trace.Mark();
        }
    }

    [Action("Fixture")]
    public class Fixture : BaseFixture
    {
        [OneTimeSetUp]
        public void OneTimeSetUp()
        {
            Trace.Mark();
        }

        [SetUp]
        public void SetUp()
        {
            Trace.Mark();
        }

        [TearDown]
        public void TearDown()
        {
            Trace.Mark();
        }

        [OneTimeTearDown]
        public void OneTimeTearDown()
        {
            Trace.Mark();
        }

        [Test]
        [Action("Test")]
        public void Test()
        {
            Trace.Mark();
        }
    }

    public static class Trace
    {
        static Trace()
        {
            Debug.Listeners.Add(new ConsoleTraceListener());
        }

        public static void Mark(string marker = "")
        {
            string s = new StackTrace().GetFrame(1).GetMethod().Name;
            if (!string.IsNullOrEmpty(marker))
            {
                s += " (" + marker + ")";
            }
            Debug.WriteLine(s);
        }
    }

[Sebazzz]

I would like this as well. It is currently not possible for a test class SetUp to have a dependency on the attribute.

[CharliePoole]

@Sebazz This has been sitting for a long time, so I'll try to kick off the design discussion I had hoped we would have. The sequence proposed by @47th in the description of this issue is what I had thought we should do originally - we just never implemented it. However, at this point, I think I would propose a simpler approach, as follows:

BeforeTest (BaseFixture)
BeforeTest (Fixture)
BaseSetUp
SetUp
BeforeTest (Test)
Test
AfterTest (Test)
TearDown
BaseTearDown
AfterTest (Fixture)
AfterTest (BaseFixture)

And:

BeforeTest (BaseFixture - OneTime)
BeforeTest (Fixture - OneTime)
BaseOneTimeSetUp
OneTimeSetUp
OneTimeTearDown
BaseOneTimeTearDown
AfterTest (Fixture - OneTime)
AfterTest (BaseFixture - OneTime)

Because it only requires making a distinction between attributes found on methods and those found on classes, this is a bit simpler to implement. It does meet the criterion of allowing SetUp or OneTimeSetUp to depend on the attribute. I think I would lean towards doing this first and then waiting to see if there are other use cases that require the more complex ordering.

[CharliePoole]

Still looking for comments on my alternate proposal.

I have some of this working already and we could do an interim release of it to see how it works, but I'd still like the feedback!

[ChrisMaddock]

As an API - I think I prefer the consistency of @47th's original proposal - however, say that with no view of the technical complications of either implementation! This is fairly niche functionality, so there's definitely value in going with the option which involves the least work, if that satisfies the requirements.

My only concern with that, would be that any changes in behaviour here will likely be breaking, no? So taking this easy route now with a view to potentially changing it later seems like it could cause two sets of breaking changes, rather than just one?

[CharliePoole]

@ChrisMaddock On general design principles, I agree with you. As I said, that was my original intention, but it didn't get implemented that way in 3.0, when we felt free to do breaking changes. It currently still works as it did in NUnit V2. So, if we change it, I was hoping to minimize the amount of change.

Just as background... this is an attribute that works differently from other NUnit 3 attributes - more like a V2 attribute. V2 Attributes were passive. The NUnit core recognized them and knew what to do. They contained almost no code. NUnit 3 Attributes are active. For the most part they actually do the thing they are designed to do in their own code, using Interfaces to tell the part of the framework that runs tests what they are capable of.

It would be possible to redesign action attributes to work in the NUnit 3 active way, but only at the cost of completely breaking all existing user-defined attributes. Can't do that! We could design a replacement attribute that worked like the action attribute but had a different implementation, but that's a bit like inventing work when we don't really have to. To make it worse, SetUp and TearDown (and the one-time counterparts) are also more like V2 attributes! Consequently, all the code for whatever we do ends up in a big mass/mess in the command and/or workitem.

All that said, I could be motivated to do more if someone who is using the attribute has a use case that requires me to do more. 😄

[Sebazzz]

@CharliePoole That looks good to me. I think (also from a usability perspective) it might not be good for attributes on a base class to necessarily run intermixed with setup from a base class.

In addition, it might be good to be able to apply ordering on the attributes. Afaik, ordering is currently undefined.

[lorisp1]

Hi, I've just upgraded to nunit framework 3.7 because I needed to execute some test actions before OneTimeSetup, but I noticed that the code sample above is producing the following result:

BeforeTest (BaseFixture)
BeforeTest (Fixture)
BaseSetUp
SetUp
BeforeTest (Test)
Test
AfterTest (Test)
TearDown
BaseTearDown
AfterTest (Fixture)
AfterTest (BaseFixture)

which is OK, but at the Fixture level, I'm seeing the following output:

BaseOneTimeSetUp
OneTimeSetUp
BeforeTest (BaseFixture - OneTime)
BeforeTest (Fixture - OneTime)
OneTimeTearDown
BaseOneTimeTearDown
AfterTest (Fixture - OneTime)
AfterTest (BaseFixture - OneTime)

while the expected result, if I'm not wrong, is:

BeforeTest (BaseFixture - OneTime)
BeforeTest (Fixture - OneTime)
BaseOneTimeSetUp
OneTimeSetUp
OneTimeTearDown
BaseOneTimeTearDown
AfterTest (Fixture - OneTime)
AfterTest (BaseFixture - OneTime)

[CharliePoole]

If you look at my comment, immediately before yours, you'll see that there was no solid agreement about what to implement. I implemented a minimal change that we agreed on and left everything else the same. I think your "expected" is what @47th asked for, but we didn't completely give it to him.

What would motivate us to do more might be a specific use case you have that requires some change. In that case, we could make further incremental changes.

[lorisp1]

The behaviour I was expecting was based on the simpler approach proposed here, which by the way would be absolutely fine for my needs.

My use case is this: I have an abstract class IgnoreIf which extends TestActionAttribute and which has several concrete subclasses that checks some conditions and, if met, they invoke Assert.Ignore() (I wrote them when I was using NUnit 2.6). Some of the OneTimeSetup methods of the classes annotated with these attributes perform actions that should not be performed if the test should be ignored.

[CharliePoole]

In the end, I felt we only had agreement on test cases, so I didn't implement my proposal on fixtures. I can look again, but a use-case would help me and the other dev understand better what's needed.

[lorisp1]

Sure, please consider the following classes:

public abstract class IgnoreIf : TestActionAttribute
{
    protected abstract bool ShouldIgnoreTheTest();

    protected abstract string IgnoreMessage { get; }

    public override void BeforeTest(ITest testDetails)
    {
        if (ShouldIgnoreTheTest())
        {
            Assert.Ignore(IgnoreMessage);
        }
    }

    public override ActionTargets Targets => ActionTargets.Suite | ActionTargets.Test;
}

public class IgnoreIfSomethingIsMissing : IgnoreIf
{
    protected override bool ShouldIgnoreTheTest()
    {
        return true;
    }

    protected override string IgnoreMessage => "Something is missing";
}

[TestFixture]
[IgnoreIfSomethingIsMissing]
public class MyTestClass
{
    [OneTimeSetUp]
    public void OneTimeSetup()
    {
        // This code should NOT be executed if something is missing. Instead, the test should be ignored
        Assert.Fail("We shouldn't arrive here if something is missing");
    }

    [Test]
    public void MyTest()
    {
    }
}

Using nunit.framework 3.7.1, I'm obtaining the following result:

NUnit Console Runner 3.6.1
Copyright (C) 2017 Charlie Poole

Runtime Environment
   OS Version: Microsoft Windows NT 10.0.15063.0
  CLR Version: 4.0.30319.42000

Test Files
    TestAction\bin\Debug\TestAction.dll


Errors, Failures and Warnings

1) SetUp Failed : TestAction.MyTestClass
We shouldn't arrive here if something is missing
at TestAction.MyTestClass.OneTimeSetup() in C:\Various\TestAction\TestAction\Sample.cs:line 41

Run Settings
    DisposeRunners: True
    WorkDirectory: C:\Various\TestAction
    ImageRuntimeVersion: 4.0.30319
    ImageTargetFrameworkName: .NETFramework,Version=v4.5.2
    ImageRequiresX86: False
    ImageRequiresDefaultAppDomainAssemblyResolver: False
    NumberOfTestWorkers: 8

Test Run Summary
  Overall result: Failed
  Test Count: 1, Passed: 0, Failed: 1, Warnings: 0, Inconclusive: 0, Skipped: 0
    Failed Tests - Failures: 1, Errors: 0, Invalid: 0
  Start time: 2017-06-15 11:10:12Z
    End time: 2017-06-15 11:10:13Z
    Duration: 0.868 seconds

while with NUnit 2.6.4, adapting the code as required:

public abstract class IgnoreIf : TestActionAttribute
{
    protected abstract bool ShouldIgnoreTheTest();

    protected abstract string IgnoreMessage { get; }

    public override void BeforeTest(TestDetails testDetails)
    {
        if (ShouldIgnoreTheTest())
        {
            Assert.Ignore(IgnoreMessage);
        }
    }

    public override ActionTargets Targets => ActionTargets.Suite | ActionTargets.Test;
}

public class IgnoreIfSomethingIsMissing : IgnoreIf
{
    protected override bool ShouldIgnoreTheTest()
    {
        return true;
    }

    protected override string IgnoreMessage => "Something is missing";
}

[TestFixture]
[IgnoreIfSomethingIsMissing]
public class MyTestClass
{
    [TestFixtureSetUp]
    public void OneTimeSetup()
    {
        // This code should not be executed if something is missing
        Assert.Fail("We shouldn't arrive here if something is missing");
    }

    [Test]
    public void MyTest()
    {
    }
}

the test would be ignored:

NUnit Console Runner 3.6.1
Copyright (C) 2017 Charlie Poole

Runtime Environment
   OS Version: Microsoft Windows NT 10.0.15063.0
  CLR Version: 4.0.30319.42000

Test Files
    TestActionNUnit2\bin\Debug\TestActionNUnit2.dll


Tests Not Run

1) Ignored : TestActionNUnit2.MyTestClass.MyTest
Something is missing

Test Run Summary
  Overall result: Inconclusive
  Test Count: 1, Passed: 0, Failed: 0, Warnings: 0, Inconclusive: 0, Skipped: 1
    Skipped Tests - Ignored: 1, Explicit: 0, Other: 0
  Start time: 2017-06-15 11:10:55Z
    End time: 2017-06-15 11:10:56Z
    Duration: 0.718 seconds

[CharliePoole]

OK... it seems like an odd use for an Action Attribute, but I can live with it. 😄

Odd for me because I see Action Attribute as something we have gotten away from in favor of various interfaces. We're forced to maintain it because it's the only way to place an attribute on a fixture and have it injected for every test case. Eventually, we'll find another approach and deprecate this.

In this case, I would have used IApplyToTest if the info needed to ignore the test is available at discovery time or IApplyToContext if it were available only at runtime.

[lorisp1]

OK, then I'll change IgnoreIf so that it implements one of those interfaces instead of extending TestActionAttribute.

Thank you very much for helping me! 😄

[CharliePoole]

It's mutual! I think you have pointed out that there is stuff about Action Attributes that need to be in the docs... like... prefer to use one of our extension interfaces if possible!

[bronumski]

@CharliePoole, I would like to reopen this case. I think I have a valid use case, I really like the ITestAction, the ability to write the attributes and attach it to an interface which I can use where needed. However given the current workflow I am having to make compromises.

First off I find myself writing lots of BDD style tests where the Fixture itself is about the subject under test and a particular scenario. For example if I was writing a test around a web auth module I might structure my tests like this:

- Acme.Web.Tests
-- AuthFixtures
--- ValidUsernameAndPassword.cs
---- [Test]ReturnsRedirectStatusCode()
---- [Test]ReturnsRedirectLocation()
---- ...
--- UnknownUsername.cs
---- [Test]ReturnsUnauthorizedStatusCode()
---- ...
--- InvalidPassword.cs
---- [Test]ReturnsUnauthorizedStatusCode()
---- ...

When I do this I would do my Act and Arrange in the OneTimeSetUp and then have a tests for each of my assertions. This works fine however the real power with the ITestAction for me is the ability to provide a dependency which that particular test is dependent on but it needs to happen before the Arrange and the Act. Currently I am having to compromise by doing either or both of the following:

1. Pushing the dependency up into the base class. Across a suite this gets messy and imposes requirements on tests that don't need the dependency and/or limits the reuse that that ITestAction would give.
2. Keep the ITestAction at a Suite level target and push the Act and the Arrange into the Setup which can have a performance impact on some of the tests.

I appreciate that among a list of ITestActions we cannot rely on the order but it would be nice to have the TestAction before test fire before the OneTimeSetup when targeting a Suite or before the Setup when targeting the Test. I am also open to any alternatives.

[jsgoupil]

This is a giant breaking change... It would be interesting to write that documentation somewhere...
From 3.6 to 3.7

My setup was:
3.6: [SetUp] was running first, then BeforeTest from TestActionAttribute was running.
3.7 BeforeTest runs first

I swapped [SetUp] to [OneTimeSetUp] and it started to work.

[CharliePoole]

@nunit/framework-team Nobody has answered @bronumski 's request to re-open this issue. He addressed his comment to me but since I'm no longer deciding such things, it would be nice for somebody to reply.

As far as I can tell, @bronumski and @jsgoupil are asking to have the change already made reversed. Is that right guys? The other options are to leave it as is or to make further changes as was originally planned.

Some team members have referred to this as a breaking change. Although I disagree, if you all think it is breaking, then I suppose backing it out makes sense... although that would probably break someone!

[jsgoupil]

I had to refactor the hell out of my unit tests to support these changes. I don't think we need to rollback personally.

A lifecycle documentation page would be super helpful.

[CharliePoole]

@jsgoupil Sorry, I don't know what a "lifecycle documentation page" is.

[bronumski]

@CharliePoole - I am not seeing anything broken as such but then I have not moved to 3.7 yet. My comment was more about the sentiment of the original poster. From where I was I couldn't tell that a change had already been made so I was adding to the existing request to have the TestActions Before and After methods wrap the fixtures Setup and Teardown methods. So if that what has happened then I am happy.

However it sounds like this would cause others such as @jsgoupil issues. If that is the wider use case then I would understand if it had to be backed out. You did hint in another thread that there might be an alternative feature making its way down the pipeline that would replace the ITestAction implementation. Did I miss read that?

[jsgoupil]

@CharliePoole I meant to have a documentation page explaining what functions will be called in which order.

[CharliePoole]

@jsgoupil That sounds like a good idea. Probably belongs under the heading of Usage Notes in the docs.

[CharliePoole]

@bronumski With the change, it's working as you requested. This is how it was designed and documented in the first place with NUnit 3.0. However, it was implemented wrong and stayed that way for several releases. Some people came to rely on the incorrect implementation. On the NUnit team there are differing views about how to handle the situation, to say the least. 😄

[rominator1983]

What I understood from your discussion is that the "new" interfaces (the ones listed here? https://github.com/nunit/docs/wiki/Custom-Attributes) should be used instead of ITestAction.

The question that arises for me is: How would I have to implement my attribute so that I have a hook for something to be done before every SetUp and after TearDown (not OneTimeSetup/OneTimeTearDown)?

I searched the documentation but honestly I don't have a clue since none of the interface I looked into do something similiar to ITestAction. It's sad to hear that you describe that as a kind of a deprecated edge case since I would very much like to implement something like this for an easier refactoring of hundrids/thousands of my tests just right now.

Can you help me out on that?

[CharliePoole]

As my earlier comments were intended to indicate...

1. There is no such hook in the new API
2. I think, like you, that there is a need for one
3. I think this issue should be re-opened or a new one created (I closed it myself in the first place because the original request seemed to be satisfied)

@nunit/framework-team It has been almost two months since my September 22 comment asking that somebody else take this on and decide whether to open a new issue, re-open this one, etc. I don't feel like I should be making the decision. Whatever is decided has to be designed, implemented and maintained by the new people on the team. If you decide to go ahead, I'm here for input.

[jnm2]

I can shepherd this one. To make discussion easier I'd like to open a new issue and direct everyone there.

Before I do that, I'm going to read through this issue and come up with a proposal. Tagging myself so it shows up in my bookmarked list and I don't forget.

[CharliePoole]

@jnm2 I think it starts with one problem, about test actions, which is pretty much resolved, but then morphs to a new one, about extending the API so that interface methods like IApplyToTest can be called on children. But see what you think!

[rominator1983]

Thanks for pointing that out (again). I have only come recently to that discussion.

From an extensibility point of view (a developer using NUnit and running into some composition-limitations of C# and NUnit) I would like to see the following: The possibility to link the following hook points to a test by declaring an attribute at class/method level:

• before one time setup
• after one time setup
• before one time teardown
• after one time teardown
• before setup
• after setup
• before teardown
• after teardown

From what I know, the hooks should be implented as an interface (most probably on the attribute itself to simplify things further?)

I do not know, if it is necessary to have a before/after test hook too since after setup and before teardown should have that covered.

For consisteny it should not matter if the attribute is defined at class or method level. This would only define, for which test the linked hooks are called. (Otherwise it would be annoying.)

Of course I do not have any insights into the NUnit architecture. As most people I just want to use it for my daily development. So I cant argue on what the parameters for the methods of the interface should be.

I hope that helps you.
Please let me know what you think about this.

[pljanot]

Recently I ran into quite hookup desire (maybe due to my bad design) although I would looking forward to see hookup points as another level of customization. Probably this desire could be workaround but why not to see native solution.