Additional Test Events for OneTimeSetUp / OneTimeTearDown

[z002Holpp]

Hi all,

For our higher-level System Tests, we need a mechanism that notifies us during the execution of a test run when certain parts of the test suite are executed. Regarding the begin and the end of a test case, NUnit already already provides corresponding test events. So we can implement a custom TestEventListener and attach some handlers to it that react when the corresponding events occure.
However, we need a similar mechanism also for OneTimeSetUp and OneTimeTearDown.

We implemented a first draft of a solution that provides additional test events for OneTimeSetUp and OneTimeTearDown.
Our main uncertainty during the implementation was to find the right place where the events should be raised.

The implementation / PR can be found at
#4652
with a brief introduction / explanation in
https://github.com/nunit/nunit/blob/c76d7d2200413d51ef307464562004617f19afec/AdditionalTestEvents.md

It would be a pleasure to discuss this approach with you and maybe some other experts in this round.
The mentioned PR should serve more as a base for a technical discussion rather than being "production-ready".

Thanks and best regards,
Stefan

[OsirisTerje]

@manfred-brands @stevenaw Comments?

[stevenaw]

This is a neat idea and seems to cover a gap in the current event listeners. I acknowledge it may require breaking changes given the interface we'd have to modify.

@OsirisTerje @manfred-brands Thoughts or concerns?

[OsirisTerje]

@stevenaw We don't need to change ITest. ITest is really about the tests themselves, and I don't think it is a fit for these. Since these are changes to the OneTimeSetup/Teardown we can either use another interface or add a new one (if we are missing one, which I suspect). That would move this more into an enhancement than a breaking change.

[stevenaw]

Oh, great point!

[TSAVogt]

Hi,
I am colleague of @z002Holpp and working in the same context.
There are two major challenges:

1. Finding the right place where to raise the event.
2. Designing a new interface for providing the needed information similar to ITest.

We would like to discuss 1. first to have the right location for such notification.

@z002Holpp already provided in Additional Test Events the three options:
a. OneTimeSetUpCommand.cs
b. SetUpTearDownItem.cs (BTW: this is a command and should be renamed, or?)
c. CompositeWorkItem.cs

To be as near as possible to the location where the methods are executed, SetUpTearDownItem.cs would be the best option. But the differentiation between OneTimeSetUp and OneTimeTearDown would not be easy.

On the other hand, what information should be provided by such an event? The OneTimeSetUp / OneTimeTearDown method itself, test result and a unique ID. Unique ID I guess would only be available by a CompositeWorkItem and not by a command.

My god feeling is with @z002Holpp proposal for OneTimeSetUpCommand and OneTimeTearDownCommand. But that solution is missing the unique ID.

What is your opinion about that?

[OsirisTerje]

The SetUpTearDownItem is a node item. The command is SetUpTearDownCommand. Did you mean that?

[TSAVogt]

Yes. When looking at SetUpTearDownItem I was confused about the ending 'Item':

• it is responsible for invoking the corresponding methods like the other commands
• is below Commands and not as the work items below Execution
  Therefore, I had the god feeling that it should be renamed to Command.

[OsirisTerje]

It is an item, along with TestActionItem. even if their names can be seen as a bit misleading, we can't (or should be extremely careful about) change these at all, since we will then introduce breaking changes for anyone out there using these methods, as they are public.

There are a lot of things in the framework, engine and adapter one might like to change. It is not necessarily a good idea to do so.

[TSAVogt]

Okay. For sure, totally understand that. Also fine for me. It is just a little bit confusing when trying to get a little bit deeper into the framework.
But coming back to the question: which class would the best to be extended for notifications about OneTimeSetUp and OneTimeTearDown calls.

[manfred-brands]

How many events do you want? One for a whole TestFixture or one for every OneTimeSetup/Teardown involved as there can be multiple if base classes are involved.

I assume you want a single one, these command are called from CompositeWorkItem.PerformOneTimeSetUp and CompositeWorkItem.PerformOneTimeTearDown respectively.

If you want multiple there are the OneTimeSetUpCommand and OneTimeTearDownCommand classes.

[z002Holpp]

@manfred-brands: Thanks for the proposal!
Yes, we want one event for every OneTimeSetUp/TearDown involved.
Would you do it in the same way as implemented in this PR?

I have adapted the existing NUnit tests as well and had some doubt that the events are not being raised as expected. However, If we now have kind of an confirmation that OneTimeSetUpCommand and OneTimeTearDownCommand are good starting points, I will try to come up with a more isolated test in order to see more clearly whats happening and when. :)

[z002Holpp]

Hi,

I added small sample test case to my PR that check whether test events for OneTimeSetUp and OneTimeTearDown are coming in the right order and right number.

As suggested, events are raised in OneTimeSetUpCommand and OneTimeSetUpCommand.
In the test case you will see that I defined a small sample fixture TestFixtureWithOneTimeSetUpTearDown in NUnit.TestData.TestFixtureTests that contains

• 1 OneTimeSetUp in a base class
• 1 OneTimeSetUp in a derived fixture class
• 1 OneTimeTearDown in a derived fixture class

For my understanding, we would expect 2 "start/finished" events for a OneTimeSetUp (one for the base class, one for the derived class) and 1 "start/finished" event for the OneTimeTearDown.
However, the test actually raises the events for OneTimeTearDown twice.

It can be easily seen in the new test of the PR.

So the question is:
Is OneTimeSetUpCommand and OneTimeSetUpCommand not the right place for raising the events OR is this an issue in NUnit itsself?

I noticed kind of an interesting place in src/NUnitFramework/framework/Internal/Execution/CompositeWorkItem.cs. I commented it also in the PR with the following words:

The following loop iterates over both setup and teardown items,
creating however OneTimeTearDownCommand objects.
Did not investigate further, but this could be the reason why I observed
additional unexpected OneTimeTearDown events when addion a
OneTimeSetUp in a base class.
It would be great if someone could have a look at my test and my finding in NUnits CompositeWorkItem.cs.

Thanks in advance for having a look! Please tell me if any information is missing or should be presented in an more convenient way!

[manfred-brands]

However, the test actually raises the events for OneTimeTearDown twice.

I have copied your tests and put a breakpoint in the MyOneTimeTearDown and it is only hit once.

If you add 'text output' to the methods, it confirms that the current NUnit behaves as expected:

BaseClassWithOneTimeSetUp.BaseOneTimeSetUp
TestFixtureWithOneTimeSetUpTearDown.MyOneTimeSetUp
TestFixtureWithOneTimeSetUpTearDown.MySetUp
TestFixtureWithOneTimeSetUpTearDown.MyTest1
TestFixtureWithOneTimeSetUpTearDown.MyTearDown
TestFixtureWithOneTimeSetUpTearDown.MySetUp
TestFixtureWithOneTimeSetUpTearDown.MyTest2
TestFixtureWithOneTimeSetUpTearDown.MyTearDown
TestFixtureWithOneTimeSetUpTearDown.MyOneTimeTearDown

I haven't yet had a chance to look at your code changes.

[manfred-brands]

I downloaded your code, but it doesn't compile with dotnet build as it has 28 warnings which become errors.
Mainly missing XML comments for new public methods and several StyleCop issues for unnecessary whitespace.

After working around these. indeed the OneTimeTearDownCommand is called twice:

Once for each class in the hierarchy, but the 2nd calls has no members in the _tearDownMethods member.

The reason the _setUpMethods and _tearDownMethods are combined in one class is that NUnit only executes the latter if the first has been run.

So in your case, you should only fire the OneTimeTearDownEvent if: _setUpWasRun && _tearDownMethods.Count > 0
You will need to add a property to the SetUpTearDownItem

Note that the similar adjustment has to be made in the OneTimeSetUpCommand to only raise an event if _setUpMethods.Count > 0

[z002Holpp]

Hi,

@manfred-brands:
Thanks for your inputs! Regarding the not compiling code, I have done the necessary adaptations and will take care about it in the future. :)

I also implemented your remarks regarding the test logic, and now my sample test case works as expected. Great!

The second big question - after where to actually fire the events - is what kind of information it should get. For example, the TestStarted and TestFinished events get an ITest or ITestResult from which they take all necessary information.

For OneTimeSetUp and OneTimeTearDown it doesnt make much sense to use ITest as well, since they are not bound to a single test, right? What kind of information would we need anyway?

• A unique ID
• The namespace / method name
• The parent object
• A result? Since a OneTimeSetUp and OneTimeTearDown can also fail...
• ... maybe more?

What would be the bare minimum to start with?
I will continue in parallel enhancing the test suite with some more scenarios.

[z002Holpp]

Hi @manfred-brands, @OsirisTerje,

I have added now

• Logic for failures in OneTimeSetUp / OneTimeTearDown such that the test events are still raised even case of errors
• Additional tests that cover the above scenarios
• Restructured the tests, in fact I am reusing pre-defined test fixtures from nunit.testdata

That looks quite fine now, not fully production-ready but that I would add later once the proof of concept is done.

As a next step, I will start to add information to the events, like method name, execution result, etc, as mentioned in my previous post.

I would be delighted to get some general opinions or guidance here.

[manfred-brands]

@z002Holpp This ticket is based on your requirement, what your system intends to do with the data.

For the Starting events the Test fixture name or the full class + method name. Remember there can be multiple OneTimeSetUp methods for a fixture.

For the OneTimeSetUp finished event add the status of the setup method as that will explain why no tests are run.

For the OneTimeTearDown finished event you could add the fixture test result. However as there could be multiple of these events does it make sense to include this in each?

Unless you add a TestFixtureCompleted event that fires after every test and tear down has run with the result.

There are different use cases. Do you really want events for every teardown method to know what is going on, e.g. for a progress info or do you want a single event for a fixture to show end result.

[z002Holpp]

@manfred-brands, @OsirisTerje

I stumbled over another problem:

The detection of OneTimeSetUp / OneTimeTearDown methods in WortkItem.cs seems to differ depending on the fixture structure.

When the SetUpTearDownItem is created, its done by descending through the inheritance hierarchy. That means that per hierarchy level, one SetUpTearDownItem is created.

I already wondered why SetUpTearDownItem actually contains a List of setup methods, although I could not find a situation where the List was holding more than one method. However, if you have a test fixture with multiple OneTimeSetUp at the same level, one instance of SetUpTearDownItem holds all of them.

That makes the place where I raise the OneTimeSetUpStarted event (OneTimeSetUpCommand.cs) questionable. At that level, I can raise only one event, even if the SetUpTearDownItem holds more than one OneTimeSetUp methods.

BeforeTest = context =>
{
  setUpTearDown.RunSetUp(context);
              
  bool eventShouldBeFired = setUpTearDown.HasSetUpMethods;
  
  if (eventShouldBeFired)
    context.Listener.OneTimeSetUpStarted(Test);   // <-- Event is raised ONCE
                
  try
  {
    setUpTearDown.RunSetUp(context);  // <-- Multiple OneTimeSetUp methods can be executed here
  }
  finally
  {
    if (eventShouldBeFired)
      context.Listener.OneTimeSetUpFinished(Test);
  }
};

I could raise the event where the actual OneTimeSetUp methods are executed (method RunSetUpOrTearDownMethod in SetUpTearDownItem), but there other issues arise, as mentioned earlier in this thread.

The test which covers this problem can be found at OneTimeSetUpTearDownEventTests.cs with test case TestFixtureWithMultipleSetUpTearDown_EventsForEachOneTimeSetUpOneTimeTearDown.

For me this seems to be anyway an edge case. I have never seen test fixtures in production that have multiple OneTimeSetUps / OneTimeTearDowns on the same hierarchy level. However, NUnit covers this scenarion and we have to consider it for the additional test events as well.

Would you suggets to raise the event at a different place than OneTimeSetUpCommand or do you see any other way how to handle multiple OneTimeSetUp methods on the same fixture hierarchy level?

[manfred-brands]

@z002Holpp I'm not near a computer to check myself, but how are the existing SetUp/TearDown event doing this. One event per method or one per attribute or per level.

[z002Holpp]

@manfred-brands: I would have to check that. I will come back to that in the next days.

[z002Holpp]

@manfred-brands:
The current implementation of Setup / TearDown is quite similar to the one for OneTimeSetUp / OneTimeTearDown.
The SetUpTearDownCommand runs the methods contained in SetUpTearDownItem which might be multiple.

So to answer your question: The call out of the command class is one per attribute per level, while i am not sure what you mean with

existing SetUp/TearDown event

since NUnit does not have events for SetUp / TearDown.

Edit:
Would it be a possibility to refactor the code in a way that the execution of the individual methods is done by OneTimeTearDownCommand? So something like

From:

public OneTimeTearDownCommand(TestCommand innerCommand, SetUpTearDownItem setUpTearDownItem)
{
  // ...
  AfterTest = context =>
  {
    // ...
    setUpTearDownItem.RunTearDown(context); <-- this will execute ALL teardown items
  }
}

To:

public OneTimeTearDownCommand(TestCommand innerCommand, SetUpTearDownItem setUpTearDownItem)
{
  // ...
  AfterTest = context =>
  {
    // ...
    foreach (var tearDownMethod in setUpTearDownItem.tearDowns)
      setUpTearDownItem.RunTearDown(tearDownMethod, context);
  }
}

Or maybe even to create allow only one method per SetUpTearDownItem and handover a list of those items to the OneTimeTearDownCommand? Something like:

From:

public OneTimeTearDownCommand(TestCommand innerCommand, SetUpTearDownItem setUpTearDownItem)
    : base(innerCommand)
{

To:

public OneTimeTearDownCommand(TestCommand innerCommand, IEnumerable<SetUpTearDownItem> setUpTearDownItems)
    : base(innerCommand)
{

[OsirisTerje]

@z002Holpp Please consider that you should strive to make the changes non-breaking, and try to not touch too much code. If a refactoring of existing code should be done, that should be done separately.
Don't try to cover too much either. If the current NUnit code makes it hard to do something, but easier to go with something simpler, choose the simple way.

1. Don't change the ITestListener interface. Add a new one, that way breaking changes are avoided.
   1.1. If at a later time it make sense to merge those two interfaces, that can be done when a major refactoring including other breaking changes are done. Although, the I in SOLID tell us we should not do that.
2. Don't change inner working, add what you can do. And accept that it might not be perfect now. If that means there is only one event, instead of a hierarchy of events, that is how it is.
3. If you have a suggestion for a refactoring, cleanup of the current design, feel free to do that, but create a new issue for that, which should be unrelated to this issue.

A test is regarded as unit. There are events for Test-started and test-finished, but they are not bound to Setup and Teardown. Anything you do in those are part of the test. E.g. if you write something to the console in a setup, it will be output as part of the test-case event. The hierarchy is resolved into one object, the class instance, just like the compiler will do (or has done, as that is what we see).

The Onetimesetup/teardowns doesn't belong to the test itself, but to the suite (fixture). A suite in NUnit is also regarded as a kind of test (composite test). However, there is no direct support there for OneTimeSetup/Teardown (or ctors), The events could have been linked to, or part of, the suite-start and suite-finished events.

[OsirisTerje]

To show what I mean by staying with the current design, I have added an experimental PR #4690.

I have added extra information to the existing start-suite event (aka TestStarted). If the start is a TestSuite (e.g. TestFIxture) and it does have OneTimeSetup or OneTimeTearDown methods, the information regarding those is included in the TestStarted event.

Given the following testcode:

public class ParentTests
{
    protected int Value { get; private set;}
    [OneTimeSetUp]
    public void OneTimeSetupParent()
    {
        Value = 42;
    }

    [SetUp]
    public void SetupParent()
    {
        Value ++;
    }

}
public class MyTests : ParentTests
{
    private int MyValue { get; set; }
    [OneTimeSetUp]
    public void OneTimeSetupMy()
    {
        MyValue = Value + 1;
    }
    [SetUp]
    public void SetupMy()
    {
        MyValue++;
    }

    [Test]
    public void Test1()
    {
        Assert.That(MyValue,Is.EqualTo(44));
    }
}

This is a simple hierarchy with OneTimeSetup's in both the parent and the derived class.

The resulting events, seen from the Dotnet test , is :

<start-suite id='0-1000' parentId='0-1003' name='MyTests' fullname='AddingEvents.MyTests' type='TestFixture'>
   <OneTimeSetup>
      <method name='ParentTests.OneTimeSetupParent' />
      <method name='MyTests.OneTimeSetupMy' />
   </OneTimeSetup>
</start-suite>

This is implemented in the TestProgressReporter, which is the one used for Visual Studio and dotnet test. Since this is XML (which is the protocol format for these), it passes through both the NUnit.Engine and the NUnit3TestAdapter. The adapter doesn't do anything with it, except dumping it to the dump file.

The main point is that this is a non-breaking change.

Since we have multiple classes acting as TestListeners, one could - if we decide to do so - implement these in a common base class. The TestListeners are not sharing anything, which can easily mean we have duplications. But that is anyway optimizations.

Doing it this way means it won't affect any other internal classes. The events will not come out any faster than this in any case.
One could add more information, but as this is specified right now, I understand it to be enough to signal that these methods have been called.

Also note that we currently don't write out any information for the TestFinished except for adding information to the test run. This could be implemented, but there has been no ask for this so far. This would also be a non-breaking change.

Adding a complete new set of detailed events would increase the payload, and we should then consider whether that is actually needed.

Since you're going to implement your own test listener, you actually don't need anything more here. You are going to execute your tests sequentially, and by hooking into the TestStarted event, you know the methods are to be executed.

If you really need to have them just after the execution, then we could add a new listener to the TestExecutionContext which cover more detailed events, but again, just raises them in the workitem process. Note that in this case the engine would also need a new extension point for this type of listener.
Such a listener could be a version 2 of the existing listener though, including the old one, e.g. ITestEventListener2.

[z002Holpp]

@OsirisTerje
Thank you for the comprehensive information and your effort in formulating it here. Its very much appreciated. I will bring the mentioned points to the PR. Question about how to fire individual events for each individual OneTimeSetUp method (if on the same hierarchy level) seems still open, but I will adress it after implementing the changes.
Regarding your last question:

If you really need to have them just after the execution, then ...

Yes, we would need the events right before and after the execution.
Thanks again!

[OsirisTerje]

Yes, we would need the events right before and after the execution.

You then need:

1. a new Listener interface, e.g. IActionListener, IDetailedListener, IMethodListener or something like that. This is crucial.
2. Call it from the appropriate places. Currently the event listener calls comes from the Composite- and SimpleWorkItem (but implemented in the WorkItem baseclass). So, since an Item encapsulates a method call (or multiple similar ones), that would probably be the correct place. Note that there are places where methods are being executed both by a command and by an item. If there are an item connected to a command, then it is normally the item that executes the method.
3. Add the same new interface to the NUnit.Engine, so that it can be used as an extension point.
4. NUnit itself doesn't need any implementation of this interface.

I have uploaded a very simplified sequence diagram for dispatching, commands, items and events here, but it give you and idea of how things are connected. You may already know this, but I believe a drawing is a good way to see how things are designed, and also possibly show discrepancies in the code versus the design.

About where to raise it: As close to the execution of the method as you can. That would normally be in the items.

[OsirisTerje]

@z002Holpp I have added more diagrams to the NUnit analysis.

The startup show in more detail than the simplified diagram how the different classes are involved during the start of a test run.

The eventflow diagram show just how the events from from NUnit back to the test runner. A new set of events need a similar event flow, but it must be distinctive new classes, except that some of these classes can be made reusable by turning them a) into generic classes b) split into an abstract base class and concrete derived classes for the different event types.

In the table here I have added some suggestions for the classes that are involved on how to handle the different issues, so that you avoid breaking current interfaces.