Add support to escape/unescape testcase filter strings #1627

genlu · 2018-05-31T23:44:30Z

This change adds support of special character escape/unescape to testcase filter, so they can be used as part of filter condition value. Those special characters and their escape sequence are:

% -> %0 This is used as the prefix for escape sequence.
( -> %1
) -> %2
& -> %3
| -> %4
= -> %5
! -> %6
~ -> %7

The escape sequences I chose here might not be the most intuitive ones for manually constructing filter strings. But it minimizes the change to existing paring code. My assumption is, there's very limited usage of filter that contains those special characters from command-line, otherwise we'd run into this problem earlier.

Here's an example that shows a currently broken scenario, the fully qualified name for the NUnit test below is Class1.HelloMethod(1.5), which contains special characters '(' and ')'. When this name is used to construct a testcase filter, parsing will fail.

    [TestFixture]
    public class Class1
    {
        [Test]
        [TestCase(1.5)]
        public void HelloMethod(decimal randomAmount)
        {  
            Assert.AreEqual(1, 1);
        }
    }

I have done some quick test to ensure that this doesn't regress perf for regular scenario: the filter parser was invoked 10 times on a filter string in the form of FullyQualifiedName=...|FullyQualifiedName=...|..., which consists of 6987 conditions with total length of 792243 (this is the filter Test Explorer generated from running all tests in Roslyn C# semantic tests project). As you can see from the results below, no impact on perf was observed.

Before:

Avg: 91.1 ms Max: 223 ms Min: 72 ms

After:

Avg: 92.9 ms Max: 219 ms Min: 75 ms

UPDATE:

I made additional change to the parser so now we can use typical escape syntax instead, which will make it easier in CLI scenario. Another benefit of this is improved perf, since we get rid of Regex matching. Now the same perf test above takes about 50 ms in average.

New syntax:

\ -> \\ This is used as the prefix for escape sequence.
( -> \(
) -> \)
& -> \&
| -> \|
= -> \=
! -> \!
~ -> \~

Fixes: https://developercommunity.visualstudio.com/content/problem/257779/live-unit-testing-does-not-work-with-nunit-testcas.html

genlu · 2018-05-31T23:45:20Z

FYI @ManishJayaswal @AbhitejJohn @Shyam-Gupta @drognanar @shyamnamboodiripad

smadala · 2018-06-01T10:03:55Z

src/Microsoft.TestPlatform.Common/Filtering/FilterHelpers.cs

+    using System.Text;
+    using Microsoft.VisualStudio.TestPlatform.Common.Resources;
+
+    public static class FilterHelpers


Can this be internal? LUT has dependency on Microsoft.TestPlatform.Common?

Yes, made this public for LUT. LUT has dependency on TranslationLayer, which pulls in Common as well.

Got it. However not all clients(VS code, Rider,..) uses TranslationLayer. We should rename this to FilterEscapeHelper and move this to OM?

I guess the tokenizer code should remain private to FilterExpression/Condition in this case, don't see a point of exposing them.

smadala · 2018-06-01T10:05:13Z

src/Microsoft.TestPlatform.Common/Filtering/FilterHelpers.cs

+
+    public static class FilterHelpers
+    {
+        private const char EscapePrefix = '%';  


Can we use \(backslash) as escape character convert ( ( -> \(, & -> \&,etc..) and write the our own parsing logic rather than depending upon regex? To make easy to construct the filter from CLI?

I agree with you that this is hard to use in CLI scenario. As you can see from the out-of-sync comment you pointed out below, I actually started with exactly what you suggested, but quickly realized that it would involve a bigger change, which I was reluctant to do this late and I really want this fix for preview 3, as it's a serious regression for LUT. Let me give this a try today and see how it goes.

smadala · 2018-06-01T10:15:15Z

src/Microsoft.TestPlatform.Common/Filtering/FilterHelpers.cs

+                {
+                    if (++i == str.Length || !ReverseLookUpMap.TryGetValue(currentChar = str[i], out var specialChar))
+                    {
+                        // "\" should be followed by a valid escape seq. i.e. '0' - '7'.


Nit: % should be followed.

smadala · 2018-06-01T10:28:52Z

test/Microsoft.TestPlatform.Common.UnitTests/Filtering/FilterStringEscapeTests.cs

+    using Microsoft.VisualStudio.TestTools.UnitTesting;
+
+    [TestClass]
+    public class FilterStringEscapeTests


Nit: FilterHelpersTests

smadala · 2018-06-01T10:31:54Z

test/Microsoft.TestPlatform.Common.UnitTests/Filtering/FilterStringEscapeTests.cs

+        public void UnescapeInvalid1()
+        {
+            var invalidString = @"TestClass%8""a %4 b""%2.TestMethod";     
+            Assert.ThrowsException<ArgumentException>(() => FilterHelpers.Unescape(invalidString), string.Format(CultureInfo.CurrentCulture, Resources.TestCaseFilterEscapeException, invalidString));


Can we hardcode the expected string? So that if anyone accidentally changes it then test fails. Any change in product code behavior should fail a test.

Good point.

singhsarab · 2018-06-01T10:37:32Z

src/Microsoft.TestPlatform.Common/Filtering/FilterHelpers.cs

+        {
+            LookUpMap = new Dictionary<char, char>()
+            {
+                { '%', '0'},


Is this exhaustive list of special characters we expect ? If not, this won't scale as we can add just 2 more with current design as %9 will be the last supported value.

I don't know if there's plan to expand operator/operation support for filter, but we can always use "%a", "%b", etc

singhsarab · 2018-06-01T10:52:20Z

src/Microsoft.TestPlatform.Common/Filtering/FilterHelpers.cs

+        /// </summary>
+        /// <param name="str">The input string that contains the text to convert.</param>
+        /// <returns>A string of characters with special characters converted to their escaped form.</returns>
+        public static string Escape(string str)


I am guessing. we are just going to replicate this code on the IDE side. If yes, it is problematic, maintenance issues and what not.
Also, we would need to document this for other consumers of TP to use this escaping logic

I made the helper class public, so it can be used by other components. But I'm not sure if this is the right place for public helpers.

singhsarab · 2018-06-01T10:57:55Z

src/Microsoft.TestPlatform.Common/Filtering/FilterHelpers.cs

+        /// <summary>
+        /// Converts any escaped characters in the input filter string.
+        /// </summary>
+        /// <param name="str"></param>


Nit: doc for str

singhsarab · 2018-06-01T11:03:15Z

test/Microsoft.TestPlatform.Common.UnitTests/Filtering/FilterStringEscapeTests.cs

+        }
+
+        [TestMethod]
+        public void EscapeStringWithSpecialCharacters()


Add test to cover '&' and '~' as well.

…ter.

genlu · 2018-06-01T23:23:06Z

@shyamnamboodiripad @AbhitejJohn @drognanar Could you folks please take a look at this as well?

smadala

Good job 👍 Approving with suggestions.

smadala · 2018-06-04T08:06:03Z

src/Microsoft.TestPlatform.Common/Filtering/FilterHelpers.cs

+    using System.Text;
+    using Microsoft.VisualStudio.TestPlatform.Common.Resources;
+
+    public static class FilterHelpers


Got it. However not all clients(VS code, Rider,..) uses TranslationLayer. We should rename this to FilterEscapeHelper and move this to OM?

smadala · 2018-06-04T08:06:49Z

src/Microsoft.TestPlatform.Common/Filtering/FilterHelpers.cs

+
+    public static class FilterHelpers
+    {
+        private const char EscapePrefix = '\\';    


Nit:EscapeCharacter makes more sense?

smadala · 2018-06-04T08:11:58Z

src/Microsoft.TestPlatform.Common/Filtering/FilterHelpers.cs

+            for (int i = 0; i < str.Length; ++i)
+            {
+                var currentChar = str[i];
+                if (SpecialCharacters.Contains(currentChar))


Can we use HashSet for SpecialCharacters. It may improve perf?

smadala · 2018-06-04T08:15:25Z

src/Microsoft.TestPlatform.Common/Filtering/FilterHelpers.cs

+                {
+                    if (++i == str.Length || !SpecialCharacters.Contains(currentChar = str[i]))
+                    {
+                        // "\" should be followed by a special character.


Show the position and character in error message and log same error message to EqtTrace.

smadala · 2018-06-04T09:09:56Z

src/Microsoft.TestPlatform.Common/Filtering/FilterHelpers.cs

+                                tokenBuilder.Clear();
+                            }
+                            // Determine if this is a "!=" or just a single "!".
+                            var next = str[i + 1];


This will cause System.IndexOutOfRangeException for "FullyQualifiedName!", Make sure we display or throw proper message.

Good catch! Fixed

smadala · 2018-06-04T09:17:43Z

src/Microsoft.TestPlatform.Common/Filtering/FilterExpression.cs

            var invalidInput = Regex.Match(filterString, @"\(\s*\)");
            if (invalidInput.Success)
            {
                throw new FormatException(string.Format(CultureInfo.CurrentCulture, CommonResources.TestCaseFilterFormatException, CommonResources.EmptyParenthesis));
            }

-            var tokens = Regex.Split(filterString, filterExpressionSeperatorString);
+            var tokens = FilterHelpers.TokenizeFilterExpressionString(filterString);


Nit: Delete not required fields filterExpressionSeperatorString and propertyNameValueSeperatorString.

singhsarab · 2018-06-04T15:13:47Z

src/Microsoft.TestPlatform.Common/Filtering/Condition.cs

@@ -44,7 +45,7 @@ internal class Condition
        /// String seperator used for parsing input string of format '<propertyName>Operation<propertyValue>'
        /// ! is not a valid operation, but is required to filter the invalid patterns.
        /// </summary>
-        private static string propertyNameValueSeperatorString = @"(\!\=)|(\=)|(\~)|(\!)";
+        //private static string propertyNameValueSeperatorString = @"(\!\=)|(\=)|(\~)|(\!)";


Delete this please

singhsarab · 2018-06-04T15:14:42Z

src/Microsoft.TestPlatform.Common/Filtering/FilterHelpers.cs

+
+        internal static IEnumerable<string> TokenizeFilterExpressionString(string str)
+        {
+            if (str == null)


Use string.IsNullOrEmpty() instead everywhere.

This is to ensure the exact same behavior as before (with regex). we might be able to do further adjustment in Parse methods but I think it can be done in a separate PR.

singhsarab · 2018-06-04T15:30:30Z

test/Microsoft.TestPlatform.Common.UnitTests/Filtering/FilterHelpersTests.cs

+    public class FilterHelpersTests
+    {
+        [TestMethod]
+        public void EscapeNull()


Null tests missing for other Unescape, Tokenize* methods

singhsarab · 2018-06-04T15:31:01Z

test/Microsoft.TestPlatform.Common.UnitTests/Filtering/FilterHelpersTests.cs

+        }
+
+        [TestMethod]
+        public void EscapeStringWithParaenthesis()


Nit: Parenthesis*

Use pattern EscapeUnescapeString* because we are checking Unescape as well

singhsarab · 2018-06-04T15:36:19Z

test/Microsoft.TestPlatform.Common.UnitTests/Filtering/FilterHelpersTests.cs

+        }
+
+        [TestMethod]
+        public void UnescapeInvalid1()


Nit: We usually follow the convention: "NameOfMethod"+"ConditionBeingTested"+"ExpectedBehavior"
Eg: UnescapeForInvalidStringThrowsArgumentException

singhsarab · 2018-06-04T16:05:14Z

src/Microsoft.TestPlatform.Common/Filtering/FilterHelpers.cs

+                }
+                else
+                {
+                    switch (current)


Just put a comment telling the allowed special tokens, eg "=", "!=", "!", "~"

When do we expect just "!" ? Can you share an example, also add to tests.

We don't expect '!', but it's not handled in tokenizer. Instead, an exception is thrown in Condition.GetOperator.

singhsarab · 2018-06-04T16:11:53Z

src/Microsoft.TestPlatform.Common/Filtering/FilterHelpers.cs

+            }
+        }
+
+        internal static IEnumerable<string> TokenizeFilterConditionString(string str)


Nit: Please add documentation for these as well.

singhsarab · 2018-06-04T16:13:40Z

src/Microsoft.TestPlatform.Common/Filtering/FilterHelpers.cs

+                {
+                    // Don't check if `current` is one of the special characters here.
+                    // Instead, we blindly let any character follows '\' pass though and 
+                    // relies on `FiltetrHelpers.Unescape` to report such errors.


Nit: pass through*/ FilterHelpers* (Same in the other method)
I guess you'd change the name though.

singhsarab · 2018-06-04T16:16:46Z

test/Microsoft.TestPlatform.Common.UnitTests/Filtering/ConditionTests.cs

+        [TestMethod]
+        public void ParseStringWithSingleUnescapedBangShouldFail1()
+        {
+


Nit: extra lines

AbhitejJohn

LGTM

AbhitejJohn · 2018-06-04T23:45:27Z

src/Microsoft.TestPlatform.Common/Filtering/FilterExpression.cs


-            // below parsing doesn't error out on pattern (), so explicitly search for that (empty parethesis).
+            //// below parsing doesn't error out on pattern (), so explicitly search for that (empty parethesis).


nit nit nit: might want to undo the changes to this comment (remove the extra //).

AbhitejJohn · 2018-06-04T23:58:18Z

src/Microsoft.TestPlatform.Common/Filtering/Condition.cs

+                        if (current == FilterHelper.EscapeCharacter)
+                        {
+                            // We just encountered "\\" (escaped '\'), this will set last to '\0' 
+                            // so the next char will not be treated as a suffix of escape sequence.


nit: treated as a prefix of escape sequence

I think this should be "suffix". But there's definitely issue with my wording so I rewrote it :)

AbhitejJohn · 2018-06-05T00:01:30Z

src/Microsoft.TestPlatform.Common/Resources/Resources.Designer.cs

@@ -107,7 +107,7 @@ internal class Resources {
        }

        /// <summary>
-        ///   Looks up a localized string similar to The diagnostic data adapter &apos;{0}&apos; requested environment variable &apos;{1}&apos; with value &apos;{2}&apos; to be set in test execution environment, but another diagnostic data adapter &apos;{3}&apos; has already requested same environment variable with different value &apos;{4}&apos;..
+        ///   Looks up a localized string similar to The data collector &apos;{0}&apos; requested environment variable &apos;{1}&apos; with value &apos;{2}&apos; to be set in test execution environment, but another data collector &apos;{3}&apos; has already requested same environment variable with different value &apos;{4}&apos;..


question: Are all these resource changes required too?

AbhitejJohn · 2018-06-05T00:02:06Z

src/Microsoft.TestPlatform.Common/Resources/Resources.Designer.cs

        /// <summary>
-        ///   Looks up a localized string similar to There are multiple configurations that have diagnostic data adapter  type &apos;{0}&apos; or Uri &apos;{1}&apos;. Duplicate configurations will be ignored in the test run..
+        ///   Looks up a localized string similar to There are multiple configurations that have data collector FriendlyName as &apos;{0}&apos;. Duplicate configurations will be ignored in the test run..


this removed a {1} too - Might want to edit the Format message?

Hmm, those aren't from my change, not sure what happened here...

Add support to escape/unescape testcase filter strings

3059910

genlu requested review from singhsarab and acesiddhu May 31, 2018 23:44

genlu and others added 3 commits May 31, 2018 16:47

Merge branch 'master' into FilterEscape

9657101

Add a filter escape test

57ef9c5

Merge remote-tracking branch 'origin/FilterEscape' into FilterEscape

2d032d2

genlu force-pushed the FilterEscape branch from cdafc57 to 2d032d2 Compare June 1, 2018 00:50

smadala reviewed Jun 1, 2018

View reviewed changes

singhsarab reviewed Jun 1, 2018

View reviewed changes

genlu added 2 commits June 1, 2018 16:06

Change escape sequences to be backslash as prefix plus escaped charac…

f36ee5d

…ter.

Remove pref tests

9f92a13

smadala approved these changes Jun 4, 2018

View reviewed changes

Merge branch 'master' into FilterEscape

94a9011

singhsarab reviewed Jun 4, 2018

View reviewed changes

genlu added 2 commits June 4, 2018 14:03

Move FilterHelper to ObjectModel

8181492

Merge remote-tracking branch 'Microsoft/master' into FilterEscape

42caef2

ManishJayaswal approved these changes Jun 4, 2018

View reviewed changes

AbhitejJohn approved these changes Jun 5, 2018

View reviewed changes

genlu added 2 commits June 4, 2018 17:38

Revert unintentional change to resource

7af070d

Address review comments

e16762f

smadala merged commit e1d4b3b into microsoft:master Jun 5, 2018

genlu deleted the FilterEscape branch June 5, 2018 17:25

AbhitejJohn mentioned this pull request May 12, 2020

Added support for debugging external test processes #2325

Merged


		// below parsing doesn't error out on pattern (), so explicitly search for that (empty parethesis).
		//// below parsing doesn't error out on pattern (), so explicitly search for that (empty parethesis).

Add support to escape/unescape testcase filter strings #1627

Add support to escape/unescape testcase filter strings #1627

Conversation

genlu commented May 31, 2018 • edited by smadala Loading

UPDATE:

genlu commented May 31, 2018 • edited Loading

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

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

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

genlu commented Jun 1, 2018

smadala left a 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

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

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

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

AbhitejJohn left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

genlu commented May 31, 2018 •

edited by smadala

Loading

genlu commented May 31, 2018 •

edited

Loading