New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose ParallelizableAttribute (and other attribute) constructor arguments as properties #2196

Closed
realityexists opened this Issue May 27, 2017 · 48 comments

Comments

Projects
None yet
5 participants
@realityexists

realityexists commented May 27, 2017

NUnit 3.6.1
I have some code that uses Reflection to look for ParallelizableAttribute on my tests and checks the Scope value passed to the constructor, but currently there is no easy way to get this value. I have to manually look in the Properties bag for a string (that I found by inspecting the source, which could easily change) and cast the result. Please provide type-safe access to all constructor arguments for all attributes via public properties. That seems like best practice for such tests anyway, as opposed to relying on the global environment.

@CharliePoole CharliePoole self-assigned this May 28, 2017

@CharliePoole

This comment has been minimized.

Show comment
Hide comment
@CharliePoole

CharliePoole May 28, 2017

Member

To get any sort of priority on this request in the general case, you should give us a use-case that is relevant to the purpose of NUnit. IOW, why do you want to reflect on the attributes and maybe some more "whys" after that (see Five Whys).

Note that the names of the properties NUnit relies upon are listed in PropertyNames.cs and not likely to change precisely because NUnit relies on them.

However, in the case of ParallelizableAttribute, this is a bug. The docs say that there is a named property Scope and there isn't - or no longer is. We'll have to fix either the docs or the code... I opt for the code.

Member

CharliePoole commented May 28, 2017

To get any sort of priority on this request in the general case, you should give us a use-case that is relevant to the purpose of NUnit. IOW, why do you want to reflect on the attributes and maybe some more "whys" after that (see Five Whys).

Note that the names of the properties NUnit relies upon are listed in PropertyNames.cs and not likely to change precisely because NUnit relies on them.

However, in the case of ParallelizableAttribute, this is a bug. The docs say that there is a named property Scope and there isn't - or no longer is. We'll have to fix either the docs or the code... I opt for the code.

@realityexists

This comment has been minimized.

Show comment
Hide comment
@realityexists

realityexists May 28, 2017

In this particular case I have some test code that I know breaks when run in parallel, so any tests that use that code should be in non-parallelizable fixtures. It's easy to forget about this, though, so I have an automated check for this: look for ParallelizableAttribute on the text fixture and check it's ParallelScope value.

As a general principle, .NET attribute are designed to be inspected via Reflection. I don't know if there is any official guideline on this from MS, but it's a defacto standard: virtually all attributes in the .NET framework expose their state via properties and the vast majority of those in third-party libraries do, too. It's just a general way to give access at runtime to what has been declared at compile time, which is something .NET generally aims to do. So I think this should be done even if the specific case above gets a better solution (e.g. you add a TestContext.CurrentContext.OtherTestsRunningInParallel property or something like that - which I wouldn't object to ;)).

realityexists commented May 28, 2017

In this particular case I have some test code that I know breaks when run in parallel, so any tests that use that code should be in non-parallelizable fixtures. It's easy to forget about this, though, so I have an automated check for this: look for ParallelizableAttribute on the text fixture and check it's ParallelScope value.

As a general principle, .NET attribute are designed to be inspected via Reflection. I don't know if there is any official guideline on this from MS, but it's a defacto standard: virtually all attributes in the .NET framework expose their state via properties and the vast majority of those in third-party libraries do, too. It's just a general way to give access at runtime to what has been declared at compile time, which is something .NET generally aims to do. So I think this should be done even if the specific case above gets a better solution (e.g. you add a TestContext.CurrentContext.OtherTestsRunningInParallel property or something like that - which I wouldn't object to ;)).

@realityexists

This comment has been minimized.

Show comment
Hide comment
@realityexists

realityexists May 28, 2017

We'll have to fix either the docs or the code... I opt for the code.

I think there's a reason right there to expose this as a property. It's already accessible, but in a way that's subject to silent breakage. If you had renamed the property it would have at least been a compile-time error - impossible to miss and easy to fix.

realityexists commented May 28, 2017

We'll have to fix either the docs or the code... I opt for the code.

I think there's a reason right there to expose this as a property. It's already accessible, but in a way that's subject to silent breakage. If you had renamed the property it would have at least been a compile-time error - impossible to miss and easy to fix.

@CharliePoole

This comment has been minimized.

Show comment
Hide comment
@CharliePoole

CharliePoole May 28, 2017

Member

No, it's not accessible. Except in the docs, which say there is a property. 😄

Member

CharliePoole commented May 28, 2017

No, it's not accessible. Except in the docs, which say there is a property. 😄

@CharliePoole

This comment has been minimized.

Show comment
Hide comment
@CharliePoole

CharliePoole May 28, 2017

Member

One risk in exposing properties publicly is that somebody may change them imagining that will change the behavior of NUnit. But generally, when the tests are being run, the properties have already done their job.

Member

CharliePoole commented May 28, 2017

One risk in exposing properties publicly is that somebody may change them imagining that will change the behavior of NUnit. But generally, when the tests are being run, the properties have already done their job.

@realityexists

This comment has been minimized.

Show comment
Hide comment
@realityexists

realityexists May 28, 2017

In that case they should definitely be read-only properties.

realityexists commented May 28, 2017

In that case they should definitely be read-only properties.

@jnm2 jnm2 added the help wanted label Jun 26, 2017

@jadarnel27

This comment has been minimized.

Show comment
Hide comment
@jadarnel27

jadarnel27 Aug 18, 2017

Contributor

I'd like to work on this issue.

PS: is making a comment like this the correct way of requesting that an issue be assigned to me?

Contributor

jadarnel27 commented Aug 18, 2017

I'd like to work on this issue.

PS: is making a comment like this the correct way of requesting that an issue be assigned to me?

@ChrisMaddock

This comment has been minimized.

Show comment
Hide comment
@ChrisMaddock

ChrisMaddock Aug 18, 2017

Member

It is - it's yours! 😄

@nunit/framework-team - does anyone have any thoughts to share on this before too much code is written? I'm not sure we entirely reached a conclusion back in May. I presume this would be exposed through CurrentContext.Properties?

Member

ChrisMaddock commented Aug 18, 2017

It is - it's yours! 😄

@nunit/framework-team - does anyone have any thoughts to share on this before too much code is written? I'm not sure we entirely reached a conclusion back in May. I presume this would be exposed through CurrentContext.Properties?

@realityexists

This comment has been minimized.

Show comment
Hide comment
@realityexists

realityexists Aug 18, 2017

I'd like to repeat my original request to expose attribute data as read-only properties on the attributes, following the defacto standard for .NET attributes, even if that information is also provided elsewhere.

realityexists commented Aug 18, 2017

I'd like to repeat my original request to expose attribute data as read-only properties on the attributes, following the defacto standard for .NET attributes, even if that information is also provided elsewhere.

@CharliePoole

This comment has been minimized.

Show comment
Hide comment
@CharliePoole

CharliePoole Aug 18, 2017

Member

@ChrisMaddock The request was for a change to the attribute, so that reflection might be used. Originally, we talked about a named property on the attribute, which is by definition read / write. @realityexists suggested it should be a read-only property after I pointed out the potential confusion arising if the user actually changed the attribute.

Note that the docs will need to be changed as well.

Member

CharliePoole commented Aug 18, 2017

@ChrisMaddock The request was for a change to the attribute, so that reflection might be used. Originally, we talked about a named property on the attribute, which is by definition read / write. @realityexists suggested it should be a read-only property after I pointed out the potential confusion arising if the user actually changed the attribute.

Note that the docs will need to be changed as well.

@jadarnel27

This comment has been minimized.

Show comment
Hide comment
@jadarnel27

jadarnel27 Aug 18, 2017

Contributor

It looks to me like the code and documentation differ on whether this property is called "Scope" or "ParallelScope."

Docs:

The Scope may also be specified using the named property Scope=.

Code:

    /// <summary>
    /// The ParallelScope associated with a test
    /// </summary>
    public const string ParallelScope = "ParallelScope";

So I think this is the bug in PropertyNames.cs that @CharliePoole was saying needs to be fixed.

The other request, to add a public readonly ParallelScope Scope property to the ParallelizableAttribute type, I think hasn't been decided on for sure.

A non-exhaustive review of attributes shows that most do not expose their constructor parameters as properties (not even as readonly properties). There are exceptions to that rule though (OrderAttribute, for one).

So adding that property to this type would actually decrease consistency in the codebase. Maybe that's a larger conversation, to decide whether or not such properties are desirable on all attribute types?

Contributor

jadarnel27 commented Aug 18, 2017

It looks to me like the code and documentation differ on whether this property is called "Scope" or "ParallelScope."

Docs:

The Scope may also be specified using the named property Scope=.

Code:

    /// <summary>
    /// The ParallelScope associated with a test
    /// </summary>
    public const string ParallelScope = "ParallelScope";

So I think this is the bug in PropertyNames.cs that @CharliePoole was saying needs to be fixed.

The other request, to add a public readonly ParallelScope Scope property to the ParallelizableAttribute type, I think hasn't been decided on for sure.

A non-exhaustive review of attributes shows that most do not expose their constructor parameters as properties (not even as readonly properties). There are exceptions to that rule though (OrderAttribute, for one).

So adding that property to this type would actually decrease consistency in the codebase. Maybe that's a larger conversation, to decide whether or not such properties are desirable on all attribute types?

@realityexists

This comment has been minimized.

Show comment
Hide comment
@realityexists

realityexists Aug 18, 2017

IMO, yes, they are desirable on all attribute types. As I pointed out above this is how virtually all .NET Framework attributes work as well as most third-party library attributes.

realityexists commented Aug 18, 2017

IMO, yes, they are desirable on all attribute types. As I pointed out above this is how virtually all .NET Framework attributes work as well as most third-party library attributes.

@CharliePoole

This comment has been minimized.

Show comment
Hide comment
@CharliePoole

CharliePoole Aug 18, 2017

Member

This is how confusion arises on our issues! We accept a very narrow issue - as we did this one - intending that the very narrow fix be made. Then the discussion ramifies to cover a broader range of considerations.

Thinking broadly is an important thing. Elucidating that thinking in the context of a narrow issue, which we have invited new contributors to work on, can only be confusing to them.

That said, since the broader issue has come up, my opinion is that we do not want to make a change across all of NUnit to conform to a "standard" that somebody believes our code should follow. Whether we expose constructor args as properties is an internal detail of NUnit and not a user issue at all.

BTW, it's stated in our design documentation that we do not use attributes in the typical way that Microsoft designed them. Anyone working on the internals of NUnit needs to understand that.

Member

CharliePoole commented Aug 18, 2017

This is how confusion arises on our issues! We accept a very narrow issue - as we did this one - intending that the very narrow fix be made. Then the discussion ramifies to cover a broader range of considerations.

Thinking broadly is an important thing. Elucidating that thinking in the context of a narrow issue, which we have invited new contributors to work on, can only be confusing to them.

That said, since the broader issue has come up, my opinion is that we do not want to make a change across all of NUnit to conform to a "standard" that somebody believes our code should follow. Whether we expose constructor args as properties is an internal detail of NUnit and not a user issue at all.

BTW, it's stated in our design documentation that we do not use attributes in the typical way that Microsoft designed them. Anyone working on the internals of NUnit needs to understand that.

@realityexists

This comment has been minimized.

Show comment
Hide comment
@realityexists

realityexists Aug 18, 2017

I would say it is a user issue for any attributes that a user would use. "Constructor arguments" does sound like an implementation detail, but it's not about exposing "constructor arguments" as such, but rather about exposing the "values" of attributes. That is, I want to be able to tell, at runtime: what configuration got applied to this test (using attributes)? If those values could be set some other way, like write-only properties I would still say to add a get accessor. It's just that write-only properties are virtually unheard of and there is no other way to pass data to an attribute (that I know of), so it was simpler to say "expose constructor arguments". Hopefully this clarifies what I meant.

Re the scope of this issue: yes, the issue is primarily about one particular attribute. I was responding to the argument that it would be "inconsistent" to do it for one attribute. So to address the secondary issue of consistency I suggest that doing this for all attributes is better than for none of them.

I don't know in what atypical way NUnit uses attributes (I wasn't able to find the design doc you refer to), but you certainly can't expect users to understand this. So if it affects users you can expect user confusion.

realityexists commented Aug 18, 2017

I would say it is a user issue for any attributes that a user would use. "Constructor arguments" does sound like an implementation detail, but it's not about exposing "constructor arguments" as such, but rather about exposing the "values" of attributes. That is, I want to be able to tell, at runtime: what configuration got applied to this test (using attributes)? If those values could be set some other way, like write-only properties I would still say to add a get accessor. It's just that write-only properties are virtually unheard of and there is no other way to pass data to an attribute (that I know of), so it was simpler to say "expose constructor arguments". Hopefully this clarifies what I meant.

Re the scope of this issue: yes, the issue is primarily about one particular attribute. I was responding to the argument that it would be "inconsistent" to do it for one attribute. So to address the secondary issue of consistency I suggest that doing this for all attributes is better than for none of them.

I don't know in what atypical way NUnit uses attributes (I wasn't able to find the design doc you refer to), but you certainly can't expect users to understand this. So if it affects users you can expect user confusion.

@jadarnel27

This comment has been minimized.

Show comment
Hide comment
@jadarnel27

jadarnel27 Aug 18, 2017

Contributor

I don't know if there is any official guideline on this from MS, but it's a defacto standard

@realityexists for what it's worth, these are the official design guidelines from Microsoft: CA1019: Define accessors for attribute arguments

But the NUnit framework, rather than user code, is the "consumer" for these passed-in values. Which seems like a valid justification for choosing to not expose them as properties.

I'm sure you know that what you're doing with the Reflection-based check is essentially static analysis. This could be accomplished with a custom Rosyln-based analyzer (like it's done here), or probably with an off the shelf static analysis tool. So I don't know if that's a good reason to change the design of a mature framework like NUnit.

Contributor

jadarnel27 commented Aug 18, 2017

I don't know if there is any official guideline on this from MS, but it's a defacto standard

@realityexists for what it's worth, these are the official design guidelines from Microsoft: CA1019: Define accessors for attribute arguments

But the NUnit framework, rather than user code, is the "consumer" for these passed-in values. Which seems like a valid justification for choosing to not expose them as properties.

I'm sure you know that what you're doing with the Reflection-based check is essentially static analysis. This could be accomplished with a custom Rosyln-based analyzer (like it's done here), or probably with an off the shelf static analysis tool. So I don't know if that's a good reason to change the design of a mature framework like NUnit.

@jadarnel27

This comment has been minimized.

Show comment
Hide comment
@jadarnel27

jadarnel27 Aug 18, 2017

Contributor

Of course, I'm very new to working with the NUnit source code. Just sharing my opinion 😄

@CharliePoole Thanks for weighing in on the larger issue! I would be interested in seeing that part of the design documentation, if you have a link handy. I've read through the developer docs / coding standards, but didn't see anything specific to attributes like that.

Contributor

jadarnel27 commented Aug 18, 2017

Of course, I'm very new to working with the NUnit source code. Just sharing my opinion 😄

@CharliePoole Thanks for weighing in on the larger issue! I would be interested in seeing that part of the design documentation, if you have a link handy. I've read through the developer docs / coding standards, but didn't see anything specific to attributes like that.

@realityexists

This comment has been minimized.

Show comment
Hide comment
@realityexists

realityexists Aug 18, 2017

Thanks for finding that, @jadarnel27.

There is no need for a Roslyn-based analyzer, because it's already possible to check the attributes using Reflection. It's just liable to break as the NUnit implementation changes.

Sure, it's not absolutely essential to expose these values as properties. If that MS design guideline and virtually everyone else doesn't convince NUnit then I don't think I can. I see little downside to it, though, other than the effort to add the properties (which is pretty easy with code refactoring tools). Even if you were to remove or rename them later it would still be better than not having them, because any programs using the values would now break noisly (with compile errors), which is better than breaking silently.

realityexists commented Aug 18, 2017

Thanks for finding that, @jadarnel27.

There is no need for a Roslyn-based analyzer, because it's already possible to check the attributes using Reflection. It's just liable to break as the NUnit implementation changes.

Sure, it's not absolutely essential to expose these values as properties. If that MS design guideline and virtually everyone else doesn't convince NUnit then I don't think I can. I see little downside to it, though, other than the effort to add the properties (which is pretty easy with code refactoring tools). Even if you were to remove or rename them later it would still be better than not having them, because any programs using the values would now break noisly (with compile errors), which is better than breaking silently.

@CharliePoole

This comment has been minimized.

Show comment
Hide comment
@CharliePoole

CharliePoole Aug 18, 2017

Member

@realityexists I've been assuming that you are talking about code you have that is separate from your tests. Is that correct? Asking because tests already have a way to get the info you want.

Member

CharliePoole commented Aug 18, 2017

@realityexists I've been assuming that you are talking about code you have that is separate from your tests. Is that correct? Asking because tests already have a way to get the info you want.

@CharliePoole

This comment has been minimized.

Show comment
Hide comment
@CharliePoole

CharliePoole Aug 18, 2017

Member

@jadarnel27 It looks as if we removed a lot of the design docs once NUnit3 was complete. What's left in the internal documentation is here: https://github.com/nunit/docs/wiki/Active-Attributes.

I'm familiar with the Microsoft document you referenced. I think it's important to understand the motivation behind the statement that getters should be defined for constructor arguments. In "traditional" use of attributes, they are no more than markers on code elements and some other code examines them through reflection. The attribute values are typically tested and the external code branches accordingly. In order for that to work, it's obviously necessary for the external code to access the values. That's how NUnit V2 worked and it's how most xunit type frameworks have worked in the past.

When the attribute itself takes over the action that was previously performed by external code, then such access becomes unnecessary. IMO, it then becomes an application of the "tell, don't ask" principle, whereby we tell objects what to do rather than asking them about their internal state. External code that examines internal state and then attempts to duplicate the logic of NUnit is subject to failure when the internal logic changes.

So, perhaps surprisingly, the designers of NUnit had heard of the Microsoft guidelines but chose not to follow them.

Member

CharliePoole commented Aug 18, 2017

@jadarnel27 It looks as if we removed a lot of the design docs once NUnit3 was complete. What's left in the internal documentation is here: https://github.com/nunit/docs/wiki/Active-Attributes.

I'm familiar with the Microsoft document you referenced. I think it's important to understand the motivation behind the statement that getters should be defined for constructor arguments. In "traditional" use of attributes, they are no more than markers on code elements and some other code examines them through reflection. The attribute values are typically tested and the external code branches accordingly. In order for that to work, it's obviously necessary for the external code to access the values. That's how NUnit V2 worked and it's how most xunit type frameworks have worked in the past.

When the attribute itself takes over the action that was previously performed by external code, then such access becomes unnecessary. IMO, it then becomes an application of the "tell, don't ask" principle, whereby we tell objects what to do rather than asking them about their internal state. External code that examines internal state and then attempts to duplicate the logic of NUnit is subject to failure when the internal logic changes.

So, perhaps surprisingly, the designers of NUnit had heard of the Microsoft guidelines but chose not to follow them.

@realityexists

This comment has been minimized.

Show comment
Hide comment
@realityexists

realityexists Aug 18, 2017

Currently the code that checks ParallelizableAttribute seems to run only as part of a test. I think at some point (maybe when I raised this issue) I also had a separate test that examined all unit tests in the project using Reflection and failed if it detected certain combinations of conditions that I knew caused problems. What's the way for tests to get this info?

realityexists commented Aug 18, 2017

Currently the code that checks ParallelizableAttribute seems to run only as part of a test. I think at some point (maybe when I raised this issue) I also had a separate test that examined all unit tests in the project using Reflection and failed if it detected certain combinations of conditions that I knew caused problems. What's the way for tests to get this info?

@jadarnel27

This comment has been minimized.

Show comment
Hide comment
@jadarnel27

jadarnel27 Aug 18, 2017

Contributor

@CharliePoole Thanks for the link, and the history behind attribute usage in NUnit! Both are very helpful.

By the way, I'm not at all surprised that "...the designers of NUnit had heard of the Microsoft guidelines..." - you all are clearly a very professional bunch, and I didn't intend to imply otherwise 😄 The code has been quite pleasant to read and work with in my limited experience so far.

Contributor

jadarnel27 commented Aug 18, 2017

@CharliePoole Thanks for the link, and the history behind attribute usage in NUnit! Both are very helpful.

By the way, I'm not at all surprised that "...the designers of NUnit had heard of the Microsoft guidelines..." - you all are clearly a very professional bunch, and I didn't intend to imply otherwise 😄 The code has been quite pleasant to read and work with in my limited experience so far.

@CharliePoole

This comment has been minimized.

Show comment
Hide comment
@CharliePoole

CharliePoole Aug 18, 2017

Member

@jadarnel27 I hadn't taken you as implying otherwise. 😄

@realityexists The test's "ParallelScope" property gives you that information. It can be accessed from the test itself using TestContext.CurrentContext.Test.Properties.Get("ParallelScope")

Member

CharliePoole commented Aug 18, 2017

@jadarnel27 I hadn't taken you as implying otherwise. 😄

@realityexists The test's "ParallelScope" property gives you that information. It can be accessed from the test itself using TestContext.CurrentContext.Test.Properties.Get("ParallelScope")

@realityexists

This comment has been minimized.

Show comment
Hide comment
@realityexists

realityexists Aug 18, 2017

I just tried this and there is no such property (nor any other properties), even though the test fixture is marked [Parallelizable(ParallelScope.None)]. NUnit version 3.6.1.0.

realityexists commented Aug 18, 2017

I just tried this and there is no such property (nor any other properties), even though the test fixture is marked [Parallelizable(ParallelScope.None)]. NUnit version 3.6.1.0.

@CharliePoole

This comment has been minimized.

Show comment
Hide comment
@CharliePoole

CharliePoole Aug 19, 2017

Member

Info about the test in the TestContext pertains to the currently executing test. If you access it in a setup, teardown or while the test is executing then it pertains to that particular test case only. The fixture is also a test, of course, but it is only directly executing in the one time setup or one time teardown. With that limitation, this approach may not be useful to you.

Member

CharliePoole commented Aug 19, 2017

Info about the test in the TestContext pertains to the currently executing test. If you access it in a setup, teardown or while the test is executing then it pertains to that particular test case only. The fixture is also a test, of course, but it is only directly executing in the one time setup or one time teardown. With that limitation, this approach may not be useful to you.

@realityexists

This comment has been minimized.

Show comment
Hide comment
@realityexists

realityexists Aug 19, 2017

Indeed, this means it's not useful. I want the "effective" setting on the test, basically - will it run in parallel or not?

realityexists commented Aug 19, 2017

Indeed, this means it's not useful. I want the "effective" setting on the test, basically - will it run in parallel or not?

@jnm2

This comment has been minimized.

Show comment
Hide comment
@jnm2

jnm2 Aug 20, 2017

Contributor

I think amenability to stable static analysis is a reasonable request. I've used static analysis myself in unit tests to great effect in large codebases. Testing other unit tests for certain properties on attributes is exactly the kind of thing I would do.

Coupling to each constructor and trying to simulate what the given constructor does for each parameter position is a kludge; examining strongly-typed properties is the way to go. I agree that we're in a unique position with regard to the .NET Framework guidelines because our attributes completely encapsulate behavior, but for such a tiny and well understood convenience as read-only properties I'm not sure why it's even worth pushing back on this.

Contributor

jnm2 commented Aug 20, 2017

I think amenability to stable static analysis is a reasonable request. I've used static analysis myself in unit tests to great effect in large codebases. Testing other unit tests for certain properties on attributes is exactly the kind of thing I would do.

Coupling to each constructor and trying to simulate what the given constructor does for each parameter position is a kludge; examining strongly-typed properties is the way to go. I agree that we're in a unique position with regard to the .NET Framework guidelines because our attributes completely encapsulate behavior, but for such a tiny and well understood convenience as read-only properties I'm not sure why it's even worth pushing back on this.

@CharliePoole

This comment has been minimized.

Show comment
Hide comment
@CharliePoole

CharliePoole Aug 21, 2017

Member

I'm not pushing back against making this particular property available. But I am pushing back to the extent that I want it to be clear that the property does not give the full picture about whether the test will be run in parallel or not. Look at our own tests. The vast majority of tests have neither the Parallelizable nor the NonParallelizable attribute. What conclusion will you draw when examining those tests?

So what I'm concerned about is that the next request may say "Well that attribute property didn't do the job for us, we need to know whether the test will run in parallel." I'm saying in advance that I don't think we should ever do that.

I'm also in favor of setting a policy that we will expose constructor arguments for other attributes as we work on them. No sweeps please!

Member

CharliePoole commented Aug 21, 2017

I'm not pushing back against making this particular property available. But I am pushing back to the extent that I want it to be clear that the property does not give the full picture about whether the test will be run in parallel or not. Look at our own tests. The vast majority of tests have neither the Parallelizable nor the NonParallelizable attribute. What conclusion will you draw when examining those tests?

So what I'm concerned about is that the next request may say "Well that attribute property didn't do the job for us, we need to know whether the test will run in parallel." I'm saying in advance that I don't think we should ever do that.

I'm also in favor of setting a policy that we will expose constructor arguments for other attributes as we work on them. No sweeps please!

@jnm2

This comment has been minimized.

Show comment
Hide comment
@jnm2

jnm2 Aug 21, 2017

Contributor

@CharliePoole Thanks, all that makes perfect sense.

Contributor

jnm2 commented Aug 21, 2017

@CharliePoole Thanks, all that makes perfect sense.

@realityexists

This comment has been minimized.

Show comment
Hide comment
@realityexists

realityexists Aug 21, 2017

"we need to know whether the test will run in parallel." I'm saying in advance that I don't think we should ever do that.

Why wouldn't you want to reveal whether the test will run in parallel or not? I think I have a pretty good use case for knowing that. I know that I have certain code that will fail when run in parallel (which NUnit could not know), so why not let me detect that.

Yes, of course, the programmer should ensure that code is not run in parallel, but this is complicated in practice, because the affected code may be deep into the call hierarchy. So it's very easy to miss when writing the test, yet very easy to include when writing the affected code.

realityexists commented Aug 21, 2017

"we need to know whether the test will run in parallel." I'm saying in advance that I don't think we should ever do that.

Why wouldn't you want to reveal whether the test will run in parallel or not? I think I have a pretty good use case for knowing that. I know that I have certain code that will fail when run in parallel (which NUnit could not know), so why not let me detect that.

Yes, of course, the programmer should ensure that code is not run in parallel, but this is complicated in practice, because the affected code may be deep into the call hierarchy. So it's very easy to miss when writing the test, yet very easy to include when writing the affected code.

@CharliePoole

This comment has been minimized.

Show comment
Hide comment
@CharliePoole

CharliePoole Aug 21, 2017

Member

It's not a question of "revealing" anything. Of course we could give you that info if we had it. But we don't have a and developing it would require us to create code that shadows the logic of test execution, separate from the execution itself. It would introduce the risk of those two lines of code getting out of sync, all for a niche application of nunit that is outside it's main goal of running tests.

I can understand why you might think that NUnit has such info available for you, but as it happens it doesn't. NUnit figures out how to run each test as it goes, not all in one place and not ahead of time.

Parallel execution is still evolving and new features will get added, but I don't see us going back and starting over with a central planning stage at this point. Have you considered just requiring those tests that can't be run in parallel to be marked that way?

Member

CharliePoole commented Aug 21, 2017

It's not a question of "revealing" anything. Of course we could give you that info if we had it. But we don't have a and developing it would require us to create code that shadows the logic of test execution, separate from the execution itself. It would introduce the risk of those two lines of code getting out of sync, all for a niche application of nunit that is outside it's main goal of running tests.

I can understand why you might think that NUnit has such info available for you, but as it happens it doesn't. NUnit figures out how to run each test as it goes, not all in one place and not ahead of time.

Parallel execution is still evolving and new features will get added, but I don't see us going back and starting over with a central planning stage at this point. Have you considered just requiring those tests that can't be run in parallel to be marked that way?

@realityexists

This comment has been minimized.

Show comment
Hide comment
@realityexists

realityexists Aug 21, 2017

Sure, I meant "revealing" that a test is running in parallel while it's running. NUnit obviously has that info by then. This is, of course, a bit different to the original request. A test may be configured as parallelizable, but when it's run by itself is it running "in parallel" (with nothing) or not? I can raise a separate issue for this if you'd like.

For the moment, checking the value of the [Parallelizable] attribute seems to work, but if you ever add, for example, a command line option that says "run everything in parallel despite what any [Parallelizable] attributes say" then the above would become important.

realityexists commented Aug 21, 2017

Sure, I meant "revealing" that a test is running in parallel while it's running. NUnit obviously has that info by then. This is, of course, a bit different to the original request. A test may be configured as parallelizable, but when it's run by itself is it running "in parallel" (with nothing) or not? I can raise a separate issue for this if you'd like.

For the moment, checking the value of the [Parallelizable] attribute seems to work, but if you ever add, for example, a command line option that says "run everything in parallel despite what any [Parallelizable] attributes say" then the above would become important.

@CharliePoole

This comment has been minimized.

Show comment
Hide comment
@CharliePoole

CharliePoole Aug 21, 2017

Member

That (the command-line option) seems a bit unlikely. 😄 Of course, we do have the reverse option, which runs nothing in parallel, but generally things are marked [NonParallelizable] because they don't run correctly in parallel - at least that's the intent. NUnit does, of course, reserve the right to run unmarked tests in any way it sees fit.

Member

CharliePoole commented Aug 21, 2017

That (the command-line option) seems a bit unlikely. 😄 Of course, we do have the reverse option, which runs nothing in parallel, but generally things are marked [NonParallelizable] because they don't run correctly in parallel - at least that's the intent. NUnit does, of course, reserve the right to run unmarked tests in any way it sees fit.

@CharliePoole

This comment has been minimized.

Show comment
Hide comment
@CharliePoole

CharliePoole Aug 21, 2017

Member

It's actually possible to determine if a test is running in parallel from within the test. If you need that, I can show you some code. We might also make it available in a simple TestContext property. Another thing that might be useful would be to report in the XML how the test was actually run.

Member

CharliePoole commented Aug 21, 2017

It's actually possible to determine if a test is running in parallel from within the test. If you need that, I can show you some code. We might also make it available in a simple TestContext property. Another thing that might be useful would be to report in the XML how the test was actually run.

@realityexists

This comment has been minimized.

Show comment
Hide comment
@realityexists

realityexists Aug 21, 2017

Yes, please, that code would be useful.

realityexists commented Aug 21, 2017

Yes, please, that code would be useful.

@CharliePoole

This comment has been minimized.

Show comment
Hide comment
@CharliePoole

CharliePoole Aug 21, 2017

Member

There are two basic approaches. Both of them require you to be using a parallel build of NUnit. If you are using one of the .NET Standard builds, the code won't compile. That should not be a problem since parallelism isn't supported at all.

Method 1 depends on the names that we conventionally give to test workers. The non-parallel workers all have "NP" in their name. Obviously, this could change very easily. However, it doesn't require you to access any internal classes.

if (TestContext.CurrentContext.WorkerId.Contains("NP"))
    Console.WriteLine("Running on Non-Parallel TestWorker");
else
    Console.WriteLine("Running on Parallel TestWorker");

Method 2 has the advantage of not depending on a string value but requires use of the internal class TestExecutionContext.

if (TestExecutionContext.CurrentContext.TestWorker.WorkQUeue.IsParallelQueue)
    Console.WriteLine("Running from Parallel Queue");
else
    Console.WriteLine("Running from Non-Parallel Queue");

Note that both approaches simply tell you what queue or worker is being used. A parallel test within a non-parallel fixture runs on the parallel queue. A non-parallel test within a parallel fixture runs on the parallel queue. Depending on exactly what you are trying to figure out, you might have to make some other checks.

Member

CharliePoole commented Aug 21, 2017

There are two basic approaches. Both of them require you to be using a parallel build of NUnit. If you are using one of the .NET Standard builds, the code won't compile. That should not be a problem since parallelism isn't supported at all.

Method 1 depends on the names that we conventionally give to test workers. The non-parallel workers all have "NP" in their name. Obviously, this could change very easily. However, it doesn't require you to access any internal classes.

if (TestContext.CurrentContext.WorkerId.Contains("NP"))
    Console.WriteLine("Running on Non-Parallel TestWorker");
else
    Console.WriteLine("Running on Parallel TestWorker");

Method 2 has the advantage of not depending on a string value but requires use of the internal class TestExecutionContext.

if (TestExecutionContext.CurrentContext.TestWorker.WorkQUeue.IsParallelQueue)
    Console.WriteLine("Running from Parallel Queue");
else
    Console.WriteLine("Running from Non-Parallel Queue");

Note that both approaches simply tell you what queue or worker is being used. A parallel test within a non-parallel fixture runs on the parallel queue. A non-parallel test within a parallel fixture runs on the parallel queue. Depending on exactly what you are trying to figure out, you might have to make some other checks.

@realityexists

This comment has been minimized.

Show comment
Hide comment
@realityexists

realityexists Aug 21, 2017

I'm trying to figure out whether another test may run at the same time as this one.

realityexists commented Aug 21, 2017

I'm trying to figure out whether another test may run at the same time as this one.

@CharliePoole

This comment has been minimized.

Show comment
Hide comment
@CharliePoole

CharliePoole Aug 21, 2017

Member

The tests given will result in some false positives, but not false negatives. That is, if you are running on a NP queue, no other tests should be running at the same time. But if you are on a parallel queue, then other tests may or may not be running, since it depends in part on the other tests.

I am convinced as a result of this discussion that we need to consider several different kinds of parallelism: between methods in the same fixture, between parameterized instances of the same fixture and across fixtures. I'm afraid that will take a bit of work and probably a few releases to get to. I'm taking it into account in my next framework, however. 😄

Member

CharliePoole commented Aug 21, 2017

The tests given will result in some false positives, but not false negatives. That is, if you are running on a NP queue, no other tests should be running at the same time. But if you are on a parallel queue, then other tests may or may not be running, since it depends in part on the other tests.

I am convinced as a result of this discussion that we need to consider several different kinds of parallelism: between methods in the same fixture, between parameterized instances of the same fixture and across fixtures. I'm afraid that will take a bit of work and probably a few releases to get to. I'm taking it into account in my next framework, however. 😄

@realityexists

This comment has been minimized.

Show comment
Hide comment
@realityexists

realityexists Aug 21, 2017

Hmm, false positives would make my check useless, as it would sometimes fail the test when there is no problem. So that doesn't seem better than my current method of just checking the [Parallelizable] attribute.

realityexists commented Aug 21, 2017

Hmm, false positives would make my check useless, as it would sometimes fail the test when there is no problem. So that doesn't seem better than my current method of just checking the [Parallelizable] attribute.

@CharliePoole

This comment has been minimized.

Show comment
Hide comment
@CharliePoole

CharliePoole Aug 21, 2017

Member

I don't think that's quite true. Consider these possibilities, a situation where the test case itself has no attribute. In that case, you can't tell how it will be run by looking at the attribute. But if it is running on a non-parallel queue, you know that it cannot be running with any other tests.

True, you might fail some tests unnecessarily, because no other tests are actually running at the same time, but I imagine you want them to fail, since more tests can always be added and it is possible that they would run at the same time

Member

CharliePoole commented Aug 21, 2017

I don't think that's quite true. Consider these possibilities, a situation where the test case itself has no attribute. In that case, you can't tell how it will be run by looking at the attribute. But if it is running on a non-parallel queue, you know that it cannot be running with any other tests.

True, you might fail some tests unnecessarily, because no other tests are actually running at the same time, but I imagine you want them to fail, since more tests can always be added and it is possible that they would run at the same time

@realityexists

This comment has been minimized.

Show comment
Hide comment
@realityexists

realityexists Aug 22, 2017

I agree, I want a test to fail if should not be run in parallel, but it may be when further tests are added. Doesn't checking the [Parallelizable] attribute on both the test and the fixture allow for this?

realityexists commented Aug 22, 2017

I agree, I want a test to fail if should not be run in parallel, but it may be when further tests are added. Doesn't checking the [Parallelizable] attribute on both the test and the fixture allow for this?

@CharliePoole

This comment has been minimized.

Show comment
Hide comment
@CharliePoole

CharliePoole Aug 22, 2017

Member

Yes, so long as you haven't used an attribute at some higher level that causes the test to run in parallel.

Member

CharliePoole commented Aug 22, 2017

Yes, so long as you haven't used an attribute at some higher level that causes the test to run in parallel.

@jadarnel27

This comment has been minimized.

Show comment
Hide comment
@jadarnel27

jadarnel27 Aug 23, 2017

Contributor

There has been some really great discussion here related to detecting (at runtime) whether or not a particular test is running in parallel!

@CharliePoole - it sounds like you have a longer-term solution in mind for this ("I'm afraid that will take a bit of work and probably a few releases to get to. I'm taking it into account in my next framework, however"). As it sounds like that is the real resolution to what @realityexists is asking for, will this issue be used to track that larger piece of work?

If that's the case, I'd be happy to open a separate issue to resolve the disconnect between the docs and code related to a named property called "Scope."

Contributor

jadarnel27 commented Aug 23, 2017

There has been some really great discussion here related to detecting (at runtime) whether or not a particular test is running in parallel!

@CharliePoole - it sounds like you have a longer-term solution in mind for this ("I'm afraid that will take a bit of work and probably a few releases to get to. I'm taking it into account in my next framework, however"). As it sounds like that is the real resolution to what @realityexists is asking for, will this issue be used to track that larger piece of work?

If that's the case, I'd be happy to open a separate issue to resolve the disconnect between the docs and code related to a named property called "Scope."

@CharliePoole

This comment has been minimized.

Show comment
Hide comment
@CharliePoole

CharliePoole Aug 23, 2017

Member

@jadarnel27 I think this issue is clearly defined as dealing with the "Scope" property, even though the discussion has ranged rather widely. I'm going to add the property in a PR, hopefully in time for this weekend's release. That's a trivial fix.

As our discussion here shows, it's not a simple binary. In fact, in the current NUnit, it's ternary: each test may be added to a parallel queue, to a non-parallel queue or not queued at all and run in the same thread that was used to initialize its parent.

So if we ask "How is this test being run?" the answer may be something like "It is running in parallel with other tests the same fixture; that fixture is being run independently of two other instances of the same fixture but in parallel with other fixtures." Since fixtures may nest and may be governed by SetUpFixtures, that answer could ramify without any limit!

For anyone who is following this, it might be interesting to see the logic that we currently use to decide how to run a test...

        private ParallelExecutionStrategy GetExecutionStrategy()
        {
            // If there is no fixture and so nothing to do but dispatch 
            // grandchildren we run directly. This saves time that would 
            // otherwise be spent enqueuing and dequeing items.
            if (Test.TypeInfo == null)
                return ParallelExecutionStrategy.Direct;

            // If the context is single-threaded we are required to run
            // the tests one by one on the same thread as the fixture.
            if (Context.IsSingleThreaded)
                return ParallelExecutionStrategy.Direct;

            // Check if item is explicitly marked as non-parallel
            if (ParallelScope.HasFlag(ParallelScope.None))
                return ParallelExecutionStrategy.NonParallel;

            // Check if item is explicitly marked as parallel
            if (ParallelScope.HasFlag(ParallelScope.Self))
                return ParallelExecutionStrategy.Parallel;

            // Item is not explicitly marked, so check the inherited context
            if (Context.ParallelScope.HasFlag(ParallelScope.Children) ||
                Test is TestFixture && Context.ParallelScope.HasFlag(ParallelScope.Fixtures))
                return ParallelExecutionStrategy.Parallel;

            // There is no scope specified either on the item itself or in the context.
            // In that case, simple work items are test cases and just run on the same
            // thread, while composite work items and teardowns are non-parallel.
            return this is SimpleWorkItem
                ? ParallelExecutionStrategy.Direct
                : ParallelExecutionStrategy.NonParallel;
        }

This code is executed for every workitem, i.e. assembly, namespaces, setup fixtures, test fixtures and their instances, method suites, test cases.

I think the next step in parallelism for NUnit will be to define groups of fixtures which may not be run together as described in #165.

Member

CharliePoole commented Aug 23, 2017

@jadarnel27 I think this issue is clearly defined as dealing with the "Scope" property, even though the discussion has ranged rather widely. I'm going to add the property in a PR, hopefully in time for this weekend's release. That's a trivial fix.

As our discussion here shows, it's not a simple binary. In fact, in the current NUnit, it's ternary: each test may be added to a parallel queue, to a non-parallel queue or not queued at all and run in the same thread that was used to initialize its parent.

So if we ask "How is this test being run?" the answer may be something like "It is running in parallel with other tests the same fixture; that fixture is being run independently of two other instances of the same fixture but in parallel with other fixtures." Since fixtures may nest and may be governed by SetUpFixtures, that answer could ramify without any limit!

For anyone who is following this, it might be interesting to see the logic that we currently use to decide how to run a test...

        private ParallelExecutionStrategy GetExecutionStrategy()
        {
            // If there is no fixture and so nothing to do but dispatch 
            // grandchildren we run directly. This saves time that would 
            // otherwise be spent enqueuing and dequeing items.
            if (Test.TypeInfo == null)
                return ParallelExecutionStrategy.Direct;

            // If the context is single-threaded we are required to run
            // the tests one by one on the same thread as the fixture.
            if (Context.IsSingleThreaded)
                return ParallelExecutionStrategy.Direct;

            // Check if item is explicitly marked as non-parallel
            if (ParallelScope.HasFlag(ParallelScope.None))
                return ParallelExecutionStrategy.NonParallel;

            // Check if item is explicitly marked as parallel
            if (ParallelScope.HasFlag(ParallelScope.Self))
                return ParallelExecutionStrategy.Parallel;

            // Item is not explicitly marked, so check the inherited context
            if (Context.ParallelScope.HasFlag(ParallelScope.Children) ||
                Test is TestFixture && Context.ParallelScope.HasFlag(ParallelScope.Fixtures))
                return ParallelExecutionStrategy.Parallel;

            // There is no scope specified either on the item itself or in the context.
            // In that case, simple work items are test cases and just run on the same
            // thread, while composite work items and teardowns are non-parallel.
            return this is SimpleWorkItem
                ? ParallelExecutionStrategy.Direct
                : ParallelExecutionStrategy.NonParallel;
        }

This code is executed for every workitem, i.e. assembly, namespaces, setup fixtures, test fixtures and their instances, method suites, test cases.

I think the next step in parallelism for NUnit will be to define groups of fixtures which may not be run together as described in #165.

@jadarnel27

This comment has been minimized.

Show comment
Hide comment
@jadarnel27

jadarnel27 Aug 23, 2017

Contributor

@CharliePoole Understood! If you feel that you won't have time, I'm happy to make that pull request before the weekend, assuming my understanding from this comment is correct (that the property exists, but is named "ParallelScope" rather than "Scope"). I've lost a little confidence that my understanding is, in fact, correct.

Contributor

jadarnel27 commented Aug 23, 2017

@CharliePoole Understood! If you feel that you won't have time, I'm happy to make that pull request before the weekend, assuming my understanding from this comment is correct (that the property exists, but is named "ParallelScope" rather than "Scope"). I've lost a little confidence that my understanding is, in fact, correct.

@CharliePoole

This comment has been minimized.

Show comment
Hide comment
@CharliePoole

CharliePoole Aug 23, 2017

Member

@jadarnel27 I hadn't noticed this was already assigned to you. I'll keep away then.

Your comment regarding PropertyNames.cs is not relevant to this discussion. The PropertyNames file holds the names of properties of the test, which are kept in a dictionary and are not C# properties at all. This change only involves creating a readonly property of the attribute and modifying the docs to no longer claim that there is a named property for use in the constructor. "Named property," of course, implies read/write.

Member

CharliePoole commented Aug 23, 2017

@jadarnel27 I hadn't noticed this was already assigned to you. I'll keep away then.

Your comment regarding PropertyNames.cs is not relevant to this discussion. The PropertyNames file holds the names of properties of the test, which are kept in a dictionary and are not C# properties at all. This change only involves creating a readonly property of the attribute and modifying the docs to no longer claim that there is a named property for use in the constructor. "Named property," of course, implies read/write.

@jnm2

This comment has been minimized.

Show comment
Hide comment
@jadarnel27

This comment has been minimized.

Show comment
Hide comment
@jadarnel27

jadarnel27 Aug 28, 2017

Contributor

@jnm2 I don't think so. The scope really can't be specified via a named property (only via the constructor).

Contributor

jadarnel27 commented Aug 28, 2017

@jnm2 I don't think so. The scope really can't be specified via a named property (only via the constructor).

@jnm2

This comment has been minimized.

Show comment
Hide comment
@jnm2

jnm2 Aug 28, 2017

Contributor

Oh, you're right. 😆

Contributor

jnm2 commented Aug 28, 2017

Oh, you're right. 😆

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment