Better user info about ParallelizableAttribute and ParallelScope

[CharliePoole]

This comes up as I work on issue #164 and I'll implement it in conjunction with that is.sue. However, I think it needs to have separate review, which is why I'm creating this issue.

ParallelizableAttribute takes an optional argument of Type ParallelScope. The values of ParallelScope are a mix of things. Some apply to the item on which the attribute appears and others to descendants. In addition, ParallelScope.None is used both when specified by the user and as a default. This turns out to lose info for us, since there are three, rather than two, actions the dispatcher may take in running a test:

• Run directly on the same thread as parent
• Enqueue on a parallel queue
• Enqueue on a non-parallel queue

I'm proposing a number of changes to make parallel test cases work and also to clarify things for users:

1. I have already defined ParallelScope.Default which indicates that no attribute was used. This turned out to be necessary to effectively implement parallel test cases.

2. For convenience internally I have re-defined the various scopes in two ranges, separating those flags that affect the item on which they appear from those that affect descendants. I use two masks to pick out the flags that are needed in a particular instance.

[Remaining items have to do with how this appears to users]

3. I'd like to deprecate ParallelScope.None in favor of ParallelScope.NonParallel. The former name is confusing enough that it was confusing me in working Run test methods within a fixture in parallel #164. Its implementation has always been "run test on non-parallel queue" but it sounds as if it means "not specified" or "use default". I'm open to other names. I considered "Never" as an option.

4. For consistency, I'd like to define ParallelScope.Parallel as a synonym for ParallelScope.Self. I wouldn't deprecate the latter, however, because combos like ParallelScope.Self+ParallelScope.Children read quite well.

5. To make it even easier for users. It may be useful to define NonParallelizableAttribute with no arguments. Just as [Parallelizable] defaults to [Parallelizable(ParallelScope.Self), this new attribute would mean the same as [Parallelizable(ParallelScope.NonParallel)

6. Some values of ParallelScope make no sense in certain places and are currently ignored silently. For example, Parallelizable(ParallelScope.Fixtures) has no effect if placed on a test case. Rather than silently ignoring the attribute, I propose that we mark the test as invalid so that the user knows it won't do whatever is expected and removes it. Implementation will require looking at each scope value and deciding whether to allow it on an Assembly, a class or a method. It seems to me that getting an early error is better for the user than having their intent (which we don't know of course) ignored silently.

7. I propose adding ParallelScope.Cases to the set of scopes that impact descendants. Used at the assembly level without combining with other scopes, it would cause test fixtures to run sequentially but all the cases within them to run in parallel. I don't think this will get a lot of use but it's consistent with the presense of ParallelScope.Fixtures. I would redefine Children as Fixtures+Cases if we do this.

Please comment! For implementation details, you'll be able to review the PR of course.

[CharliePoole]

@nunit/core-team @nunit/framework-team Comments requested!

[rprouse]

ParallelScope.NonParallel sounds good to me. Should we deprecate or just rename with the same value though?

The rest sounds fine although I find ParallelScope.Cases to be a bit confusing. Tests or TestCases? Doesn't match with Fixtures, but I found Fixtures clear, but not Cases. Might be just me though, so @nunit/framework-team please speak up 😄

[CharliePoole]

Well, if we just rename, they will get a compiler error. If we mark as obsolete, they will get a compiler warning with a message that tells them to use NonParallel. Both require changing the code but deprecation seems slightly more user-friendly and you can ignore the message if you like. What do others think?

Cases was just an idea we can easily drop. The only functionality it adds is when it is used on the assembly, which I don't think would be very common. On a fixture, it would be equivalent to Children since we don't (currently) treat nested classes as children of the surrounding class.

[ChrisMaddock]

Few comments - sorry, I'm aware some of the below wasn't what you intended to be up for discussion, but while we're working behaviour out...

ParallelizableAttribute takes an optional argument [...] ParallelScope.None is used [...] as a default.

So is it right that, by default, if I make a method [Parallelizable] (no level specified) - then that makes it non-parallelizable? That seems an unexpected default - should we instead consider making the argument required?

I'd like to deprecate ParallelScope.None in favor of ParallelScope.NonParallel

Just my personal opinion from an external library user's point of view, I've never found any confusion with None. I also personally prefer Never to ParallelScope.NonParallel - feels a bit less wordy! If we do want to make the change, then +1 for deprecation, but, if the name change is for internal development purposes, perhaps we can do something else, purely internally?

For consistency, I'd like to define ParallelScope.Parallel as a synonym for ParallelScope.Self

As a name, I'm not entirely sure what level of parallelization ParallelScope.Parallel is setting. I guess it would mirror NonParallel - but I think we have a more fine-grained parallelization model than inparallel=true/false, and I'm not sure if having Parallel/NonParallel as two of the options would obscure that.

It may be useful to define NonParallelizableAttribute

👍 Sounds good. You said in this section that [Parallelizable] defaults to Self - rather than None as was above. I'm not sure if that was a proposed change, or current behaviour - but I think 'Self' would be a better default!

Rather than silently ignoring the attribute, I propose that we mark the test as invalid

Great idea! This stuff is hard to work out what's actually happening - I remember the first time I tried to implement it, having to dig into log files to work out what was actually running in parallel. (I blame ReSharper for this!)

I propose adding ParallelScope.Cases to the set of scopes that impact descendants.

Cases was just an idea we can easily drop.

I'd wonder if it's worth leaving this one until we're sure there's a need - purely for the sake of keeping the API a little cleaner.

[CharliePoole]

@ChrisMaddock To clarify defaults...

Currently, if no attribute appears, we default to non-parallel. If the attribute is used without an argument we default to Self.

As for NonParallel versus None, no need to change if I'm the only one who finds it confusing. Let's hear some other opinions.

In the new code, ParallelScope.Default us used internally when no attribute is specified.

[ChrisMaddock]

@ChrisMaddock To clarify defaults...
Currently, if no attribute appears, we default to non-parallel. If the attribute is used without an argument we default to Self.

That make much more sense to me. I've re-read the above and I've misinterpreted, sorry!

[CharliePoole]

Some further thoughts to put this into perspective.

I'd design this differently today. ParallelScope, as used, does two things: (1) selects parallel or non-parallel execution and (2) specifies the scope of that setting (just the test on which the attribute appears, children, etc.). That's because I adapted ParallelScope from MbUnit's TestScope but tried to make it do more things than are probably reasonable for a single enum.

So, ideally, I'd like to separate the parallel/non-parallel distinction from the scope. One way to do that has occured to me: Eliminate ParallelScope.None (deprecate it) in favor of the new NonParallelizable attribute. If this works out, the internal implementation can still keep using the full range of scopes but hide them in the intellisense. If this sounds good I'll pursue it further but I'd rather not spend time on it until we agree this is the way to go.

[rprouse]

That update sounds reasonable to me.

[CharliePoole]

I'm trying a "docs first" approach. Please take a look at these...

https://github.com/nunit/docs/wiki/Parallelizable-Attribute-(New)
https://github.com/nunit/docs/wiki/NonParallelizable-Attribute

[oznetmaster]

Seems good to me.

[ChrisMaddock]

I think this is much cleaner - good shout separating the two out!

[CharliePoole]

Letting this pot simmer for a bit while I work on the NUnit 3 VS Adapter release.