WIP Issue497 filter #522

OsirisTerje · 2018-06-22T18:11:07Z

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.

…flag in runsettings #497

CharliePoole · 2018-06-29T20:01:29Z

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

OsirisTerje · 2018-06-29T20:04:26Z

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

CharliePoole · 2018-06-29T22:42:28Z

src/NUnitTestAdapter/NUnitTestFilterBuilder.cs

+            if (tfsFilter!=null && settings.UseTestCaseFilterConverter)
+            {
+                var nunitfilter = new VsTest2NUnitFilterConverter(tfsFilter.TfsTestCaseFilterExpression.TestCaseFilterValue);
+                filterBuilder.SelectWhere(nunitfilter.ToString());


Since there is no error detection, It seems like there is an underlying assumption here that the converter will always produce a valid where clause. Do we know that's true for all possible values of the original filter?

jnm2 · 2018-06-29T23:05:40Z

src/NUnitTestAdapter/AdapterSettings.cs

+        bool UseTestCaseFilterConverter { get; }
+    }
+
+    public class FeatureFlags : IFeatureFlags


Is this alternate implementation of IFeatureFlags public by design, or can it be private and maybe even a struct?
I have to think about why both AdapterSettings and FeatureFlags implement IFeatureFlags and where in the codebase FeatureFlags might be used versus AdapterSettings.

This FeatureFlags class is used for the runsettings, so that we can separate out this block of settings. Since the other ones are options, so to speak, I find the featureflags different, and therefore easier to have them in a separate block. In the code though, it is enough that we can see them in the adaptersettings, therefore the interface is implemented there too, and implementation just passed down.

src/NUnitTestAdapter/NUnit3TestExecutor.cs

jnm2 · 2018-06-29T23:22:46Z

src/NUnitTestAdapter/NUnit3TestExecutor.cs

@@ -136,7 +130,7 @@ public void RunTests(IEnumerable<TestCase> tests, IRunContext runContext, IFrame
                    var assemblyPath = Path.IsPathRooted(assemblyName) ? assemblyName : Path.Combine(Directory.GetCurrentDirectory(), assemblyName);

                    var filterBuilder = CreateTestFilterBuilder();
-                    var filter = filterBuilder.MakeTestFilter(assemblyGroup);
+                    var filter = filterBuilder.MakeTestFilter(assemblyGroup,new TfsTestFilter(runContext));


Crazy nitpick: space after comma?

jnm2 · 2018-06-29T23:23:42Z

src/NUnitTestAdapter/NUnit3TestExecutor.cs

 #else
-            return new NUnitTestFilterBuilder(TestEngine.Services.GetService<ITestFilterService>());
+            return new NUnitTestFilterBuilder(TestEngine.Services.GetService<ITestFilterService>(),Settings,TestLog);


Spaces after commas?

jnm2 · 2018-06-29T23:24:30Z

src/NUnitTestAdapter/NUnitTestFilterBuilder.cs

        }

        public TestFilter ConvertTfsFilterToNUnitFilter(TfsTestFilter tfsFilter, List<TestCase> loadedTestCases)
        {
            var filteredTestCases = tfsFilter.CheckFilter(loadedTestCases);
            var testCases = filteredTestCases as TestCase[] ?? filteredTestCases.ToArray();
            //TestLog.Info(string.Format("TFS Filter detected: LoadedTestCases {0}, Filterered Test Cases {1}", loadedTestCases.Count, testCases.Count()));
-            return MakeTestFilter(testCases);
+            return MakeTestFilter(testCases,tfsFilter);


Space after comma?

jnm2 · 2018-06-29T23:28:30Z

src/NUnitTestAdapter/NUnitTestFilterBuilder.cs

+            {
+                var nunitfilter = new VsTest2NUnitFilterConverter(tfsFilter.TfsTestCaseFilterExpression.TestCaseFilterValue);
+                filterBuilder.SelectWhere(nunitfilter.ToString());
+                if (settings?.Verbosity>4)


What do you think of putting spaces around the > operator and the != operator above?
Apologies for the nitpickiness today!

Is Verbosity > 4 different from skipping the if and using logger.Debug?

jnm2 · 2018-06-29T23:35:40Z

src/NUnitTestAdapter/TfsTestFilter.cs

-        {
-            get { return TfsTestCaseFilterExpression == null || TfsTestCaseFilterExpression.TestCaseFilterValue == string.Empty; }
-        }
+        public bool IsEmpty => TfsTestCaseFilterExpression == null || TfsTestCaseFilterExpression.TestCaseFilterValue == string.Empty;


Some of these lines get pretty long...

One option for long getters would also be

{ get => TfsTestCaseFilterExpression == null || TfsTestCaseFilterExpression.TestCaseFilterValue == string.Empty; }

jnm2 · 2018-06-29T23:42:20Z

src/NUnitTestAdapter/VSTest2NUnitFilterConverter.cs

+        public VsTest2NUnitFilterConverter(string vstestfilter)
+        {
+            NUnitFilter = vstestfilter.Replace("TestCategory", "Category");
+        }


This might just be personal preference but doing the heavy lifting of the class doesn't feel very "constructory" to me.
The name is Converter but once you have an instance, you can't convert things with it. You can only access a conversion result.
What would you think of introducing a static string (string) method?

jnm2 · 2018-06-29T23:45:48Z

src/NUnitTestAdapterTests/VSTest2NUnitFilterConverterTests.cs

+    public class VsTest2NUnitFilterConverterTests
+    {
+        [TestCase("TestCategory!=Whatever", "Category!=Whatever")]
+        [TestCase("TestCategory=Whatever","Category=Whatever")]


Can you add FullyQualifiedName = SomeNamespace.TestCategory.SomeMethod?
What else should we add from https://github.com/Microsoft/vstest-docs/blob/master/docs/filter.md?

build.cake

CharliePoole · 2018-06-30T03:09:49Z

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.

OsirisTerje · 2018-06-30T12:26:36Z

@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 · 2018-06-30T15:23:43Z

@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:

Keep the translator as a simple option, which will cover most cases
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
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.

CharliePoole · 2018-06-30T15:45:48Z

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.

OsirisTerje · 2018-06-30T19:25:39Z

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

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

CharliePoole · 2018-07-01T00:26:33Z

Sounds OK then.

OsirisTerje added 6 commits April 4, 2018 22:20

Added vstest->nunit test filter converter

735f456

Merge branch 'master' into Issue497-Filter

adc80ee

csproj change for test project

ba4ced0

Added first version of vstest filter parsing, hidden under a feature …

0a316a9

…flag in runsettings #497

Change default to true.

6343c83

Merge branch 'master' into Issue497-Filter

e0c447c

OsirisTerje requested review from rprouse and jnm2 June 22, 2018 18:11

Merged

7a7425c

OsirisTerje requested a review from CharliePoole June 29, 2018 19:58

CharliePoole reviewed Jun 29, 2018

View reviewed changes

jnm2 requested changes Jun 29, 2018

View reviewed changes

Updated translation

2b1c2e6

OsirisTerje added 6 commits October 7, 2018 10:30

Filter update #497

40ffe28

Merged

849a8f6

Merged from latest master

4539f6c

Merge changes and test fixes #497

2dd6a81

Updating filters

0a29319

Removed debugging

0a8e58a

OsirisTerje changed the title ~~Issue497 filter~~ WIP Issue497 filter Oct 27, 2020

OsirisTerje closed this Jul 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP Issue497 filter #522

WIP Issue497 filter #522

OsirisTerje commented Jun 22, 2018 •

edited

CharliePoole commented Jun 29, 2018

OsirisTerje commented Jun 29, 2018

CharliePoole Jun 29, 2018

jnm2 Jun 29, 2018

OsirisTerje Feb 15, 2019

jnm2 Jun 29, 2018

jnm2 Jun 29, 2018

jnm2 Jun 29, 2018

jnm2 Jun 29, 2018 •

edited

jnm2 Jun 29, 2018

jnm2 Jun 29, 2018

jnm2 Jun 29, 2018

CharliePoole commented Jun 30, 2018

OsirisTerje commented Jun 30, 2018 •

edited

OsirisTerje commented Jun 30, 2018

CharliePoole commented Jun 30, 2018

OsirisTerje commented Jun 30, 2018

CharliePoole commented Jul 1, 2018

WIP Issue497 filter #522

WIP Issue497 filter #522

Conversation

OsirisTerje commented Jun 22, 2018 • edited

CharliePoole commented Jun 29, 2018

OsirisTerje commented Jun 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnm2 Jun 29, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CharliePoole commented Jun 30, 2018

OsirisTerje commented Jun 30, 2018 • edited

OsirisTerje commented Jun 30, 2018

CharliePoole commented Jun 30, 2018

OsirisTerje commented Jun 30, 2018

CharliePoole commented Jul 1, 2018

OsirisTerje commented Jun 22, 2018 •

edited

jnm2 Jun 29, 2018 •

edited

OsirisTerje commented Jun 30, 2018 •

edited