Several SetUpFixtures at the same level may be active at the same time

[vgriph]

When having multiple SetUpFixtures in a at the same level of test assembly all OneTimeSetUp methods are invoked before the first test is invoked.

The following is an excerpt from the trace of an execution of a test assembly with a setup fixture in NUnitTest.NamespaceA and one in NUnitTest.NamespaceB that shows how both setups are executed before the first test in NamespaceA is executed:

TestWorker: Worker#STA_NP executing NUnitTest.dll
WorkItemDispatcher: Enqueuing NUnitTest
TestWorker: Worker#STA_NP executing NUnitTest
WorkItemDispatcher: Enqueuing NamespaceA
WorkItemDispatcher: Enqueuing NamespaceB
TestWorker: Worker#STA_NP executing NamespaceA
WorkItemDispatcher: Enqueuing TestFixture
TestWorker: Worker#STA_NP executing NamespaceB
WorkItemDispatcher: Enqueuing TestFixture
TestWorker: Worker#STA_NP executing TestFixture
WorkItemDispatcher: Directly executing ShouldOnlyHaveNamespaceASetuUpActive
TestWorker: Worker#STA_NP executing TestFixture
WorkItemDispatcher: Directly executing ShouldOnlyHaveNamespaceASetuUpActive

NUnitTest.zip contains a set of tests that reproduces the issue.
TestRunAndTrace.zip contains output and internal trace from running the tests above using NUnit3-console 3.2.1.

[appel1]

Perhaps you could explain why this a problem for you. I don't think the order of when OneTimeSetUp methods are invoked is specified right now. Only that they'll be called once before any fixtures in the same namespace are executed.

https://github.com/nunit/docs/wiki/SetUpFixture-Attribute

[vgriph]

We have system tests that use the SetUpFixtures to manage services needed during tests in a namespace. Several namespaces may contain the same service, i.e. will allocate the same listening port. In NUnit 2.6.2 this worked fine, and no two setup fixtures on the same level were active at the same time. I don't fancy redesigning the service hosting and database setup for our system tests as part of the upgrade to NUnit 3.2.1, especially since it was not listed as a breaking change.

[CharliePoole]

It's not exactly a breaking change, which is why it isn't listed. A breaking change is a change in NUnit's previously defined behavior. In this case, running tests by walking the namespace tree depth-first was simply an implementation detail, albeit one that proved useful to you.

I do think this would be a useful feature. We do not currently consider a SetUpFixture to be "active" or "inactive" as you have indicated, but it seems to be a useful concept. It would be easy to implement for your specific situation, but gets more complicated as we consider the interaction of this feature with tests run in parallel. Since a namespace may have multiple SetUpFixtures, it would be possible for them to have conflicting parallel scopes. We will have to decide how to resolve those. I think some sort of spec will be called for.

[tso-blstream]

We have case when in global we have to prepare SeeTest client, then in iPhone, iPad, Android namespaces we have to open devices for client. Then we have some tests in namespaces iPhone.Auth, etc. which needs app to be authenticated (those outside Auth namespace needs app to be unauthenticated). NUnit first executes iPhone.Auth.Setup, then iPhone tests, then iPhone.Auth tests and at the end iPhone.Auth.TearDown. I was expecting it to work like DFS.

[CharliePoole]

While we marked this as a feature and added it to our backlog, it may not be clear to everyone what that means: that we accept this as something we will fix, change or add. Maybe we should change "Backlog" to "Accepted" :-)

[tso-blstream]

I understand what Backlog means. I just wanted to give you other example of use case. We found workaround for this, but will wait for this to be implemented to use it.

[CharliePoole]

Does your use case involve any parallel execution of child tests or is just an issue of sequencing what we run?

[CharliePoole]

Some thoughts on the design of this...

This is somewhat related to #1239 but is a bit simpler.

If no parallel execution is involved and everything runs on the non-parallel queue, then all we have to do is run children before doing anything else. Since there's only one thread, no two SetUpFixtures can be active at the same time... with one small exception.

The exception is that some child tests may require the STA, which involves a separate queue. Execution may need to switch back and forth between the two queues - possibly several times - in order to complete the work. But there is no way to prevent tests other queued tests like other SetUpFixtures from running when we switch queues - at least not currently.

This leads me to think of a workaround that @tso-blstream and others may want to try: running the tests using the --workers:0 option. With that option no queues are used, no switching occurs and I believe the order of running is compatible with NUnit 2.

When some child tests may be parallel, all bets are off. Since the child of a parallel test may declare itself non-parallel - and vice versa - we're switching back and forth among our various queues all the time in order to get the work done. When one queue is exhausted, we switch to another one, and we are done when there is no work to do anywhere.

I'm thinking that it may be necessary, in the case of a non-parallel fixture with parallel children, to establish a private set of queues just for those children. Since it would take looking at all the children to detect this situation, it may mean that we set up separate queues and workers (or use a pool) for each test suite and fixture!

[tso-blstream]

I have tested it with --workers=0 --inprocess and it worked. Thanks 👍

[CharliePoole]

NOTE: This is out of date, see later comments.

Proposed design ... looking for comments from @nunit/core-team, @nunit/framework-team and anyone else interested in this feature.

1. Define a concept of "active" for a test. In the case of a test method, it is the same as "running" but for fixtures the test is active from the start of onetime setup to the end of one time teardown. It may be running a method or not. For example, the one time setup may have finished along with a few tests but some other tests may simply be queued. In this case, noting is running but the test fixture is still active.

2. Create a database of active WorkItems, allowing us to check which work items are active at any given time. A Dictionary<string, WorkItem> is probably the best option, with the key being the ID of the item. Initially, this would only be used for CompositeWorkItems representing fixtures. The item would be added to the dictionary when it started execution and be removed only when it completed. We would be able to determine if any item is active by looking up its ID in the dictionary.

3. Define some way to indicate which tests (including fixtures and setup fixtures) are allowed to be active together. In the original proposal for parallel execution, I stipulated a property ExclusionGroup for the ParallelizableAttribute with a string value. This is probably a reasonable approach, although we could consider other options such as a new Attribute. For a given exclusion group (string), only one test should be permitted to be active at any time.

4. Create (lazily) a set of queues indexed by the exclusion group - that is a Dictionary<string, Queue>. When a fixture is about to be dispatched, check first to see if it belongs to an exclusion group. If so, check to see if any fixture is active in that group. If none is active, dispatch the work item, otherwise add it to the queue for that exclusion group.

5. When a fixture with an exclusion group terminates, dequeue the next fixture in the same group and dispatch it.

Ideally, we would all stand around a white board and talk about this. Lacking that... pleas comment here.

[jnm2]

I don't see a problem with it, but right now I can't imagine writing tests with some kind of global state so I'd never use it. Even my integration tests stay out of each other's way.

[CharliePoole]

@jnm2 The use case here is for SetUpFixture. Since we don't give you any way to access instance information for a SetUpFixture from a TestFixture or TestCase, use of some sort of global state seems to be fairly common.

I definitely agree that it is both possible and desirable to structure tests so that this is not necessary!

[CharliePoole]

Here's a simpler case that we may want to resolve first...

Let's say you have a [NonParallelizable] test fixture. It's executing (by itself, of course) from the non-parallelizable queue. Now let's say that some of the test methods in that fixture are marked [Parallelizable]. As they are encountered, they will be added to the Parallelizable queue.

Eventually, the dispatcher will exhaust the non-parallelizable queue and switech execution to the parallelizable queue and those test cases will get run. However, there may be other fixtures and their test cases on that queue and they will be run even though the original non-parallelizable fixture is still active.

This is essentially caused by the same internal issue as the initial problem: we do not distinguish between the active state and the running state. That is, there is no way to consider a fixture as active except when it's code is executing. However, from a user point of view, it is seen as active from the first one-time setup to the final one-time teardown.

I'm going to give this a think and if it seems like it can be resolved more simply than this issue, I'll create a new issue that we may want to work on first.

[rprouse]

Your design looks okay, but it is very complex. Is there no way to simplify by stepping back and changing the way we do the initial queuing of the tests? Probably not, I am saying this without having looked at that code in ages, but I always hate to see additional complexity 😄

[CharliePoole]

Agreed. Have you looked at #2143, which identifies a simpler issue that works toward the problem?

[CharliePoole]

I think PR #2151, which resolves issue #2143, takes care of the problem of setup fixtures running together and eliminates the need for specifying "Groups" of fixtures which may not run together even though they are parallelizable. I'll lay out what I think will be the effect of #2151 in several different situations and we can decide whether I'm right!

1. Non-parallel execution If a SetUpFixture is marked as non-parallelizable, Ensure that parallel cases under a non-parallel fixture only run with one another #2151 creates a new, empty set of queues for everything under it to use. No other setup fixtures at the same level can run until all the tests under the initial setup fixture have completed.

2. Parallel execution If a SetUpFixture is marked as parallel, then other setup fixtures and test fixtures can run in parallel with it. That's the problem for which the current issue was written. Basically, we want the setup fixture to run in parallel with other, non-related fixtures, but not with other setup fixtures that conflict with it, e.g.: multiple setup fixtures that target different browsers. Under Ensure that parallel cases under a non-parallel fixture only run with one another #2151, this can be resolved by nesting all those setup fixtures under a higher-level setup fixture. The higher-level fixture is parallelizable while the individual setup fixtures are non-parallelizable. They will then each run separately from one another but in parallel with everything else. The following code illustrates the concept...

namespace A
{
    [Parallelizable]
    public class TopLevelFixture{ }
}

namespace A.B
{
    [NonParallelizable] // or leave as default
    public class ConflictingFixture1 { }

    [NonParallelizable] // or leave as default
    public class ConflictingFixture2 { }
}

Let's discuss this after the review and merge of #2151. You'll have to understand how that works in order to make sense of this one. 😄

[CharliePoole]

Can we close this for the release based on PR #2151? I'd like it if one of the reviewers of #2151 would follow my logic in the preceding comment to see if you agree with me that this is now fixed.

[rprouse]

I agree that this should be considered complete with the changes in #2151.

[CharliePoole]

In that case...

[CharliePoole]

I'm quite happy with how this turned out. It's a much simpler solution than I originally envisioned. We may need to get more complicated to handle dependencies, but for now this is pretty good.