-
Notifications
You must be signed in to change notification settings - Fork 319
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support to escape/unescape testcase filter strings #1627
Conversation
using System.Text; | ||
using Microsoft.VisualStudio.TestPlatform.Common.Resources; | ||
|
||
public static class FilterHelpers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be internal? LUT has dependency on Microsoft.TestPlatform.Common
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, made this public for LUT. LUT has dependency on TranslationLayer, which pulls in Common as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. However not all clients(VS code, Rider,..) uses TranslationLayer. We should rename this to FilterEscapeHelper and move this to OM?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the tokenizer code should remain private to FilterExpression/Condition in this case, don't see a point of exposing them.
|
||
public static class FilterHelpers | ||
{ | ||
private const char EscapePrefix = '%'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
{ | ||
if (++i == str.Length || !ReverseLookUpMap.TryGetValue(currentChar = str[i], out var specialChar)) | ||
{ | ||
// "\" should be followed by a valid escape seq. i.e. '0' - '7'. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: %
should be followed.
using Microsoft.VisualStudio.TestTools.UnitTesting; | ||
|
||
[TestClass] | ||
public class FilterStringEscapeTests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: FilterHelpersTests
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)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point.
{ | ||
LookUpMap = new Dictionary<char, char>() | ||
{ | ||
{ '%', '0'}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if there's plan to expand operator/operation support for filter, but we can always use "%a", "%b", etc
/// </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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
/// <summary> | ||
/// Converts any escaped characters in the input filter string. | ||
/// </summary> | ||
/// <param name="str"></param> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: doc for str
} | ||
|
||
[TestMethod] | ||
public void EscapeStringWithSpecialCharacters() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add test to cover '&' and '~' as well.
@shyamnamboodiripad @AbhitejJohn @drognanar Could you folks please take a look at this as well? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job 👍 Approving with suggestions.
using System.Text; | ||
using Microsoft.VisualStudio.TestPlatform.Common.Resources; | ||
|
||
public static class FilterHelpers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. However not all clients(VS code, Rider,..) uses TranslationLayer. We should rename this to FilterEscapeHelper and move this to OM?
|
||
public static class FilterHelpers | ||
{ | ||
private const char EscapePrefix = '\\'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit:EscapeCharacter
makes more sense?
for (int i = 0; i < str.Length; ++i) | ||
{ | ||
var currentChar = str[i]; | ||
if (SpecialCharacters.Contains(currentChar)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use HashSet
for SpecialCharacters. It may improve perf?
{ | ||
if (++i == str.Length || !SpecialCharacters.Contains(currentChar = str[i])) | ||
{ | ||
// "\" should be followed by a special character. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Show the position and character in error message and log same error message to EqtTrace.
tokenBuilder.Clear(); | ||
} | ||
// Determine if this is a "!=" or just a single "!". | ||
var next = str[i + 1]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will cause System.IndexOutOfRangeException
for "FullyQualifiedName!"
, Make sure we display or throw proper message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! Fixed
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Delete not required fields filterExpressionSeperatorString
and propertyNameValueSeperatorString
.
@@ -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 = @"(\!\=)|(\=)|(\~)|(\!)"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delete this please
|
||
internal static IEnumerable<string> TokenizeFilterExpressionString(string str) | ||
{ | ||
if (str == null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use string.IsNullOrEmpty() instead everywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
public class FilterHelpersTests | ||
{ | ||
[TestMethod] | ||
public void EscapeNull() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Null tests missing for other Unescape, Tokenize* methods
} | ||
|
||
[TestMethod] | ||
public void EscapeStringWithParaenthesis() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Parenthesis*
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use pattern EscapeUnescapeString* because we are checking Unescape as well
} | ||
|
||
[TestMethod] | ||
public void UnescapeInvalid1() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: We usually follow the convention: "NameOfMethod"+"ConditionBeingTested"+"ExpectedBehavior"
Eg: UnescapeForInvalidStringThrowsArgumentException
} | ||
else | ||
{ | ||
switch (current) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just put a comment telling the allowed special tokens, eg "=", "!=", "!", "~"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When do we expect just "!" ? Can you share an example, also add to tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't expect '!', but it's not handled in tokenizer. Instead, an exception is thrown in Condition.GetOperator
.
} | ||
} | ||
|
||
internal static IEnumerable<string> TokenizeFilterConditionString(string str) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Please add documentation for these as well.
{ | ||
// 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: pass through*/ FilterHelpers* (Same in the other method)
I guess you'd change the name though.
[TestMethod] | ||
public void ParseStringWithSingleUnescapedBangShouldFail1() | ||
{ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: extra lines
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
||
// 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). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit nit nit: might want to undo the changes to this comment (remove the extra //
).
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: treated as a prefix of escape sequence
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be "suffix". But there's definitely issue with my wording so I rewrote it :)
@@ -107,7 +107,7 @@ internal class Resources { | |||
} | |||
|
|||
/// <summary> | |||
/// Looks up a localized string similar to The diagnostic data adapter '{0}' requested environment variable '{1}' with value '{2}' to be set in test execution environment, but another diagnostic data adapter '{3}' has already requested same environment variable with different value '{4}'.. | |||
/// Looks up a localized string similar to The data collector '{0}' requested environment variable '{1}' with value '{2}' to be set in test execution environment, but another data collector '{3}' has already requested same environment variable with different value '{4}'.. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: Are all these resource changes required too?
/// <summary> | ||
/// Looks up a localized string similar to There are multiple configurations that have diagnostic data adapter type '{0}' or Uri '{1}'. 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 '{0}'. Duplicate configurations will be ignored in the test run.. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this removed a {1}
too - Might want to edit the Format message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, those aren't from my change, not sure what happened here...
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.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:
After:
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