Adds Explicit as a trait

[jnm2]

Part of the 3.10 work for #47

@OsirisTerje this was way easier than I thought it was going to be!

[CharliePoole]

You appear to have added a method to the adapter, which is only their for tests to use. Am I missing something?

[jnm2]

I did. This was in hopes that it would be canonicalized and that the adapter might end up using it. If there are three places that require you to know to use .SelectNodes("//test-case") in order to use ConvertTestCase, what happens when there's a subtle change in one of the three? (An example, I wondered about .SelectNodes("test-case")?) I was hoping that would potentially end up encapsulated here.

[CharliePoole]

But your test is testing a method that is not used in the adapter itself... right now anyway. So you are not fully testing the application as it exists.

A refactoring of the code is another matter, but we should have a reason to undertake that refactoring and we should do it separately from adding a feature. All IMO, of course.

Searching the code online, I only found two places in the adapter where the converter is called, plus a bunch in the tests. I could have missed one. The two calls I found are in the Discoverer and the Executor. It could be factored into a method of the base adapter class.

[jnm2]

The test usage made three, and I didn't look for others.

So you are not fully testing the application as it exists.

I'm missing something here. Since it purely delegates to the method I would be calling anyway, I'm covering no less than I would be covering without having added this method. Right?

I'll go ahead and move this into the test layer since it's too speculative.

[CharliePoole]

I continue to see this as a bad idea. In NUnit, Explicit is not a category. It's a thinkg in itself... a C# property of the test case or suite. Making it look like a Trait for Visual Studio is fine because NUnit itself does not have Traits. OTOH, NUnit does have Categories. Making a Property of tests in NUnit appear to be a Category for all users of the adapter is bound to cause confusion.

Further, I don't like seeing this slip in under #47, which talks about refactoring. According to Fowler, refactoring is "a disciplined technique for restructuring an existing body of code, altering its internal structure without changing its external behavior." (Italics mine)

We've always tried to separate refactoring from enhancements and bug fixes. Putting this in a separate PR gives a certain degree of separation, of course. But it skips the parts where we create an enhancement issue, decide whether we like the change or not and conduct any design discussions that seem to be necessary.

Personally, I like the idea that the user should be able to select Explicit tests as a trait in both the IDE and through a filter. I don't like it masquerading as a category because it misrepresents how NUnit works to the VS adapter user. It also means that our internal Explicit will now be indistinguishable from any user-created "Explicit" Category.

If this were an issue, I'd give it the Design category, meaning that we need to discuss the user-facing design of how we will present Explicit.

[jnm2]

In #47 we discussed the ability to include or exclude explicit tests via TestCategory = Explicit. I believe Explicit is shorthand for that.

We could easily refactor this to become a plain trait (rather than a property-as-a-trait) via testCase.Traits.Add(new Trait("Explicit", value: "")); and the UI would look the same. In 3.10, we're not planning to enable you to run explicit tests through the adapter anyway. Let me see if TestCategory = Explicit and Explicit work with that, out of curiosity.

[CharliePoole]

Sorry, I was actually looking at issue #457, which is about refactoring, not #47.

In #47, we discussed making it a trait. I'm against it but I believe @OsirisTerje accepted it. You gave some examples where it was used as a Category, but I don't believe we agreed to that.

I'll reiterate: I don't want TestCategory="Explicit" to ever work unless the user has put [Category("Explicit")] on a test. That's a perfectly legal thing to do and should work - even if it is potentially confusing to the user who does it. Further, I think that such a case should be distinguishable from an actual [Explicit] test if we are going to do this. One way I can think of is to have a Trait RunState with value Explicit. Another might be a pseudo-property Explicit with value true or false - or just true and omitted for false.

[jnm2]

Oops! That was actually my fault. I thought I had typed #47 and edited it.

[jnm2]

Adding it via testCase.Traits.Add(new Trait("Explicit", string.Empty)) also results in the UI we want but without filters TestCategory = Explicit or Explicit working. That is fine for 3.10 since we're temporarily blocking all explicit tests in 3.10.

[jnm2]

The trickiness I was afraid of earlier just came into play. Almost done. Need to figure out why it applies to some child test cases and not others when I move it from a trait property to a pure trait.

[jnm2]

Oops, missed reverting a file. Now ready for review.

[OsirisTerje]

I believe the issue we're trying to fix here is about how to make it visible in the Test Explorer.
Given that the TE and VSTest dont have any concept like Explicit.
I made a simple set of tests which have both categories, properties and explicit tests. Now, after discovery it looks like this:

Agree it is impossible to figure out what is the Explicit ones?
Now if we run it, it becomes a bit more possible , at least to guess what can be explicit, but we cant be sure:

Also notice that the concept "Trait" in TE/VSTS covers both properties and categories.

So in order to make Explicit visible in TE, we must either define it as a property or a category. Since it is a singular value, it either exists or not, it doesn't really suit the property format, thus, the category format seems better. But we need to document and evangelize the way this will work in VS, because it cant work the same way ordinary categories do (it has to be explicitly set on the test method it should act on).

There doesn't seem to be any obvious right or wrong solution here, the best would be that TE/VSTest was changed so that Explicit tests became first class citizens.

[CharliePoole]

@OsirisTerje I think you said it just right. Best would be for Test Explorer to recognize certain native concepts. But that's unlikely. They give us two ways to model, as a VS Property or a VS Category. I use the VS prefix, because NUnit also has Properties, and implements Category as a special Property.

Let's look at some possibilities...

[Test, Explicit]
public void SomeTest() { }

[Test, Category("Explicit")]
public void AnotherTest() { }

[Test, Property("Explicit", "True")]
public void ThirdTest() { }

[Test, Property("RunState", "Explicit")]
public void FinalTest() { }

All of these are possible and perfectly valid. They can even appear on the same test! The first can only be run in the console runner by name. The other three can be run using

1. --where "cat==Explicit"
2. --where "Explicit==True"
3. --where "RunState==Explicit"

Ideally, we would like all four examples to show up differently in the IDE and work differently on the command-line. However, if we have to sacrifice one of the options - making it appear identical to the built-in Explicit attribute - then it should not be Category that is sacrificed IMO.

Use of properties in NUnit is pretty advanced and may even not be known to many users. Explicit itself is not known to all users. Categories are something pretty much everyone has heard about and uses. Creating something that can be confused with a category is probably the worst thing we can do.

Can you think of something that is unlikely for users to use and also distinguishable from the others?

[jnm2]

Just so we're all on the same page, I don't believe Test Explorer calls them categories. To Test Explorer all we can do is add what they call Traits which may or may not have a value, and what they call Properties.

testCase.Traits.Add(new Trait("Hello", string.Empty)); produces the UI grouping Hello.
testCase.Traits.Add(new Trait("Hello", "val")); produces the UI grouping Hello [val].

testCase.SetPropertyValue(NUnitTestCategoryProperty, new string[] { "Hello" }); also produces the UI grouping Hello, just like the Trait.
(This is because NUnitTestCategoryProperty is created with TestPropertyAttributes.Trait which gives us a warning because it is marked obsolete. Otherwise it would only show up as the UI grouping Category [Hello].)

As far as I can see we can use either Traits or Properties and it makes no difference in the UI. It also would not cause TestCategory = Explicit to start matching if we add a new Property rather than using the NUnitTestCategoryProperty we set up. Therefore, we can use either Traits or Properties and it would make no difference in the UI and it would not necessarily make any difference to command line filtering.

[jnm2]

The only place I can think of where it's possible to block explicit tests for 3.10 is in TfsTestFilter.CheckFilter.

But that pretty much requires us to add a property or trait so that we can identify which TestCases are explicit.

So unless you have an idea, all the 3.10 filter work is held up by this PR which actually adds the property or trait.

[jnm2]

Maybe I'll add a dummy property and mark the other PR WIP, to be changed once this PR is in.

[CharliePoole]

Having written the code, I do know how traits work. 😄

Properties in Test Explorer serve a broader purpose. As you found out, they are not all marked as traits. For example, xunit.net uses properties to save information between the discovery and execution phases that VS has saddled us with. We use a different approach. Both have pros and cons.

If this is for the IDE, we just want a trait. NUnit categories display as traits in the IDE because we translate them to traits with the name "Category." That's different from the filter issue where the (relatively) new TestCategory is recognized as a thing in itself.

In any case, my question is really about what we all want to see, both in the IDE and when using the command-line. The command-line is critical for categories, since it's where they exist as first-class entities. I'd prefer not to lock the user out of creating a Category with the name "Explicit" even though it's likely to be fairly rare.

[jnm2]

Is there any problem with approving this PR then, since it currently adds a trait?

[CharliePoole]

It seems OK to me. Waiting for @OsirisTerje

[OsirisTerje]

Ok with me too :-)