nunit / nunit3-vs-adapter

NUnit 3.0 Visual Studio test adapter for use under VS 2012 or later
https://nunit.org
MIT License
203 stars 105 forks source link

Category (Traits) should differentiate test cases even when they have same data name #1182

Closed PeterHevesi closed 3 months ago

PeterHevesi commented 3 months ago

Hi there, I have found, that only because same dataset has the very same name as another dataset coming from different datasource, it is not discovered in VS Test Explorer. But it should, because by putting different Category (from NUnit), I already want to clearly say, that that datasource is actually different. Even when I use Traits to group my tests, these tests are never discovered when having the same name. I have made simple NUnit Test project with this simple file, which should absolutely clearly illustrate what I mean:

https://gist.github.com/PeterHevesi/aa1392c27000985c13c35dff9653f83a

I already made and issue in vstest repo, and they told me this issue is on you side: https://github.com/microsoft/vstest/issues/5097

OsirisTerje commented 3 months ago

The behaviour is "as designed". The testhost (which feeds the Test Explorer) is based on FQN of the test. The FQN is based on MS rules, and they analyze the code and send a sequence of these down to the adapter. The results are then displayed accordingly, so when there are multiple tests with the same name, they are pooled together.

The input sent down in this case is: image

Note that the name of the parameter is part of the FQN.

The results are then: image

If you then look at what NUnit delivers back: image

You can see that all the information is there, but the Test Explorer doesn't a) use the information in that way to display it b) doesn't apply the same information to its filters. So it will not honor the categories, and that is by their design.

If you run it using the command line however, it will honor the categories> image

OsirisTerje commented 3 months ago

I have uploaded your code above to the adapter issues repo, Issue1182. The history for that folder will show you "as is" as the first commit and the code fix you should do as the second and latest commit there.

The clue to get this working is to encapsulate the yielded return data in a TestCaseData instance. The other thing you should do is to use the class + method pattern for the testcase source, aka Form 2. That gets rid of an extra node Test Explorer likes to add in that section, although it also comes up as a no-traits node. This is anyway a ghost node, thanks to the Test Explorer. image

Note that I have created my own TestCaseData class, inheriting from that, so that I can do common stuff there. I have just used the nameof class to set the name and category. You can adjust this as you like of course.

image

The point here is that you have to set the name. The Test Explorer doesn't use the category for the naming of the test itself, only for grouping in the hierarchy.

The code is image image

If you need to have both a non-unique dataName you have to send in the the name of the test, or derive it internally in the MyTestCaseData class.

Have fun!

PS. I'll move this issue into the Discussions, as this is more a How-To than an issue.

PeterHevesi commented 3 months ago

Hi, Thanks so much for you response, I really appreciate that :) So I have to use new TestCaseData(new myActualDataClass() ) instead of just new myActualDataClass()? Can I derive myActualDataClass from some baseClass or make it implement some interface, so it then acts just like this, but is simpler to write?

PS: And I actually didn't understand your first response, why my original approach can't be working. So is at the VS Test Explorer side?

OsirisTerje commented 3 months ago

I don't think you can implement it because the test method require a ctor with one parameter, and your dataclass require a ctor with multiple parameters. That's why you need that TestCaseData wrapper. Feel free to experiment though.

And yes, it is the Test Explorer that require things to be by FQN, and by their definition of that. Attempts to create your own names tends to be messy. And Category is not a part of the FQN.

About the double new statements: You can implement a factory method to handle that for you, and perhaps an extension method on that, to reduce the syntax there, or something like:

    new myActualDataClass(x,y,z).ToTestDataClass(); 

may be easier on the eye.

PeterHevesi commented 3 months ago

Ok thanks for all your tips, I really appreciate you help :) I've just asked in the vstest repo, if they could make Category to be part of FQN, I think it would still be better, if Category is part of FQN. It seems to me very strange, that you could group by it, but different category alone won't differentiate test case, so you don't even see it in Test Explorer. So I hope that guys from vstest would be willing to repair this :)

OsirisTerje commented 3 months ago

Ok, but how would you organize if a test had multiple categories? I'm pretty sure people would complain if the same test appeared multiple places in the tree, like they do now if you add Categories to the grouping. If Category was a part of FQN, it would always happen.

PeterHevesi commented 3 months ago

I am not sure. But even now, when you put multiple categories at some level and do the grouping by Category (Traits) you got the test to appear multiple times in the tree, right? And for me, it is a wrong design, to put multiple categories to one test and then group by category. I would much rather have just multiple different attributes, at least these absolutely logical:

  1. CategoryGroup
  2. Category
  3. Subcategory
  4. Tag

Now this seems much more logical to me, then I could like say, that I will use Category for example to differentiate my datasource by website version (we have separate website version for each our customer), and then I could let's say use Subcategory to group even further by lets say Browser (chrome vs edge vs firefox).

It just seems to me, that when you have multiple different types of grouping, it is not that smart to use just one attribute for all these types of grouping, but people should be able to decide what attribute out of those 4 want to use for what grouping.

In this kind of looking at it, there is no such problem as you described, and this problem would then never ever occur, while it can occur easily even now.

OsirisTerje commented 3 months ago

The Category is a very well known attribute, used by many over many years. I don't think we will change that, nor will MS.

And, to me, it makes sense as it is. To me adding it to the FQN for a test doesn't make sense. It is another dimension than a name for a test. There are enough ways to add or modify testnames, so using Category for that, and in addition having to create other similar attributes seems like a lot of work for very little purpose.

A category can be strongly typed, and thus mimic a lot of different trees. It has a very good purpose by its own.

You can argue against using multiple categories to a test, but it can actually be very useful to. It can depict environment, layers, test-levels (which is a very important aspect - Component Tests, Integration Tests, System Tests, Acceptance tests and so on).

And, there are millions using this now, for both NUnit, MSTest and XUnit.

PeterHevesi commented 3 months ago

But I don't get your point, so I tried it, I have put multiple categories to 1 test data source, and they actually are appearing multiple times in Test Explorer (even now, when it is not part of FQN). So we probably don't understand each other well, or I don't know.

There is this issue, when tests don't appear in Test Explorer, because DataName for it is the same, and you complain, that Category cannot be put into FQN, because test would appear multiple times in Test Explorer when using multiple Test Categories, but that is actually exactly what is happening now... It is the current behavior !

So I just don't understand you probably :D

And about that stuff, that instead of having just Category, I propose to create more attributes like CategoryGroup, SubCategory and even Tag, is to be able to group by a chain longer then 4.

All grouping I have now is as follows (in any order of course): Project => Namespace => Class => Category(Traits) So this grouping is only 4 nodes long, and that is the maximum you can get, because all those other attributes there are not at all important for me, I don't want to group by State, Duration, Target Framework, or Environment... I cannot probably like use Environment, to override what it means, and use this attribute to differentiate for example by browser, I mean to rewrite values that are in those attributes...

If you add 3 new attributes (CategoryGroup, SubCategory and Tag), the maximum length of this grouping chain would increase from 4 to 7, which should be enough for really like everybody.

So my proposals doesn't change the current behavior in any way, it just makes life easier for those who need it.

And believe me, there are people, that would use what I suggest.

PS: I don't get, when you say Category can be strongly typed, so it can mean different trees of categories at the same time. Surely you can do it, but the test framework just consider them to be a category, and surely is not prolonging the chain of groupings, but is indeed rather putting same test to appear multiple times in the explorer.... So it is not treated as another tree of category...

OsirisTerje commented 3 months ago

and you complain, that Category cannot be put into FQN, because test would appear multiple times in Test Explorer when using multiple Test Categories, but that is actually exactly what is happening now

Sorry for not being clear. A Category is a different dimension than a name (or part of a name) (as in FQN). It has nothing to do with the FQN. Since you can add multiple categories to a test - which is a common practice - it follows that it will be displayed multiple times. Arguing that one should not use multiple categories is simply killing the categories as what it is today. That will not happen. Category is a Property, and no one can prevent anyone from adding more properties.

It is then better to ensure you have a proper naming in place.
And then you're back to what I believe, but I may be wrong, how you go ahead setting the names of your test. You can do that with two methods, SetName and SetArgDisplayNames, for the test and arguments respectively.

It is the Test Explorer that groups on different things. Not the test framework. And not NUnit. NUnit is just providing properties, and category is a "special" type of a property. Under the hood it is just a property with name "Category" and whatever value you gave it.

If you want more category types, you can just create them, as strongly typed items. Look at the source code for the Category attribute

You can implement as that, but it is easier to just derive it directly from the CategoryAttribute. However, the Test Explorer will not, as it is now, care about any kind of relationship between your categories (which are properties, right). They will be displayed as siblings in the tree, not as parent/children as I assume you want. There is nothing that NUnit can do, except provide them, - as properties, and as you have defined them.

Example:

public class CategoryGroupAttribute : CategoryAttribute
{
    public CategoryGroupAttribute(string name) : base(nameof(CategoryGroupAttribute))
    {
    }
}

public class SubCategoryAttribute : CategoryAttribute
{
    public SubCategoryAttribute(string name) : base(nameof(SubCategoryAttribute))
    {
    }
}

public class Tag : CategoryAttribute
{
    public Tag(string name) : base(nameof(Tag))
    {
    }
}

If you use the source code for the category in NUnit as a base for these instead, you can add to the ApplyTo method, that when giving a Category it should also add the parent - your CategoryGroup I would assume, and like wise further down in your hierarchy. A Tag would then have all categorytypes, which could indicate it is itself plus its parents.

But, the Test Explorer would still display them as siblings. Nothing we can do about that.

Even IF NUnit should implement a completely new type of category, which had some hierarchy, it would be moot - as again, the Test Explorer would ignore it. And, so would all other reporting function and other runners.

There are a few cases where other hierarchies have been created and displayed, but mostly it is just inserting some of these custom types into a hierarchy based on FQN, so it is more of an extension than a separate hierarchy.

PeterHevesi commented 3 months ago

Alright, I understand that this is not about NUnit but rather about Test Explorer itself.

I am not arguing, that one should not used more categories on 1 test, I only said it once, but now I am not arguing about it anymore. It just that for my use case, I would rather use CategoryGroup, Category and Subcategory, but I am not saying, that other people should not used more Categories... So if Test Explorer makes Category (or more Categories) to be part of the FQN, it won't hurt anything, because it would behave exactly the same as is behaving now, but without that bug, that Category alone won't differentiate test...

What is the point of using Category, if I still need to categorize it inside the name? That is actually my point, if I put a category on top of a test source, that says Firefox for example, why should I put that Firefox inside the name of the dataset again. It just doesn't make any sense to put all my categories inside the name, because Category alone just won't differentiate...

So I know I should talk about this with the Microsoft guys from Visual Studio and vstest, and not with you, since it has nothing to do with NUnit.

But I just wanted to understand you arguments, about why putting categories (even more than 1) into FQN would hurt anything... Because I guess it won't change the behavior at all, but would simply repair the bug I am talking about...

Do you now agree with me on this, or I still don't understand it right?

PS: If you agree, would you please upvote this feature request? https://developercommunity.visualstudio.com/t/Traits-Category-should-differentiate-t/10685328?

OsirisTerje commented 3 months ago

As explained above, I'm afraid I can't agree at all.

As @nohwnd explains, a change as what you suggest will break existing tests. As I said above, I also claim that it will ruin the use of categories as an unrelated grouping mechanism, which is what it is today.

If I read you right, you want category to be a part of the FQN. But, why can't you then use the name that is there "as is". If you look at the repro code here: https://github.com/nunit/nunit3-vs-adapter.issues/tree/master/Issue1182 , you will see I have added your "category" names as part of the FQN. You are not using them as categories/traits, but as part of the name, so then it is better to do exactly that. That one will solve your issue.

PeterHevesi commented 3 months ago

Thank you very much for your example and your time :) But I am sorry, it is not solving my problem at all. You changed it in a way, in which you totally changed my original problem. When I have same name to the data, and it is actually in 2 different data sources, and I put category to those datasources, it won't differentiate. So I need to put it in the name exactly what I have put into the category. So Category will group me that stuff, but if I want to make that other tests to be discoverable, I still need to put the very same thing I have already written into Category inside DataSource into the name of each test case under that data source. That is what is silly and doesn't make any sense. You use Category only for grouping, because you still need to put it into the name itself to be at least discoverable.

Here is the gist once again, you see, that 3 tests are discovered from those 2 datasources, but actually, 6 are there to be discovered! You done nothing to solve this problem other then putting it into the name itself. https://gist.github.com/PeterHevesi/b2d9c2f46e2c92191fe2806638a8bc9c?ts=4 Can test case name be set from data source? This would make it at least automatic, but still would put it into name, which is unnecessary...

And then, I wrote to @nohwnd, that it could also be just an easy option inside .runsettings, that would really not make any problems to nobody, if by default, it would be turned off, and we could only turn it on, if we need to.

And adding TraitGroup or SubTraits (my second problem) would still not make anybody any harm.

But anyway, I understand you can't help me with this, because this is not NUnit, so I can only hope for guys from VS to help me. Thanks for your time anyway :)

OsirisTerje commented 3 months ago

I am closing this here.

You should look again at the code. The name property is what holds the FQN. You can put whatever you want there using the SetName method, e.g. your categories. You then build the hierarchy as you like. Have a look again.

PeterHevesi commented 3 months ago

Ok, thank you very much for you willingness to help me :) I will look again on the code you've send me :)