nunit3-vs-adapter icon indicating copy to clipboard operation
nunit3-vs-adapter copied to clipboard

WIP Issue497 filter

Open OsirisTerje opened this issue 7 years ago • 8 comments

WIP *** DO NOT MERGE *** Changed test case filter to use NUnit Filter. Added a featureflag for testing purposes, have now left it default true, but keep open the possibility of defaulting to false if further tests show it is not stable. Local tests so far looks good.

OsirisTerje avatar Jun 22 '18 18:06 OsirisTerje

@OsirisTerje Re our email issues... I did get an email that you asked for my review of this.

CharliePoole avatar Jun 29 '18 20:06 CharliePoole

Ok, then that part works at least, but if you didnt get any emails from the mentions today, something is wrong with github.

OsirisTerje avatar Jun 29 '18 20:06 OsirisTerje

It's a clever idea to use text conversion rather than parsing, changing the text representation of the VS filter to the text representation of an NUnit filter.

It should work if the languages are sufficiently close. That's my area of doubt. The alternative, of course, is to create a grammar of VS filters - maybe Microsoft has that already - and parse it, generating NUnit filters directly without the intermediate step of TSL.

My general feeling is that if you want to go this way you need to throw a lot more tests at it, including some invalid input and non-translatable input if you can come up with examples. Currently, the code assumes no error will ever occur.

CharliePoole avatar Jun 30 '18 03:06 CharliePoole

@CharliePoole Yep, more tests are needed.
The VSTest filter language us very simple, and looks more or less like a subset of the NUnit TSL, which is why I thought just converting it to TSL was a simple way to go. What is not included above is the " contains " ( ~ ) operator, but that should convert into a simple regex (vstest ' ~ ' operator => TSL ' =~' operator. The logical operators are the same.

OsirisTerje avatar Jun 30 '18 12:06 OsirisTerje

@CharliePoole The simple filters are easy to just translate this way, but when it comes to terms like Name and ClassName, I start to worry a bit about these terms also being part of the RHS clauses. The translator doesn't separate between those. Since there is a difference in NUnit between the term Testname and methodname, these terns doesn't really make that much sense to translate. I have rarely seen them in filter expressions too. So, there are a few options, and some of these can be selected through feature flags:

  1. Keep the translator as a simple option, which will cover most cases
  2. Add , under a featureflag, the possibility of dropping in TSL directly. This would make the VSTest filter expressions idewntical to the nunit console filter expressions, and the syntax is not that different from the current vstest syntax. It also is richer, adding more possibilities. or
  3. Write a new parser, like the current TestSelectionParser, but just for the VSTest filter language. Just observe that we then still have the same impedance mismatch wrt to FQN, and as the adapter also presents properties to VSTest to use, we are still in a mixed world.

OsirisTerje avatar Jun 30 '18 15:06 OsirisTerje

Of course, 3 was what I originally proposed to you.

The impedance mismatch will always exist but some approaches make it easier than others to give a precise error message.

What happens if you use a non-supported rhs element? What happens if you use invalid syntax, say a bad operator or unbalanced parentheses?

I'll try to make up some test cases later at my computer.

CharliePoole avatar Jun 30 '18 15:06 CharliePoole

If I use an invalid operator, an exception is thrown from the parser; image

Invalid rhs will be handled as it always have been, that is, if it is not a valid test artifact, it will be ignored.

OsirisTerje avatar Jun 30 '18 19:06 OsirisTerje

Sounds OK then.

CharliePoole avatar Jul 01 '18 00:07 CharliePoole