Support escaping , in Test filter #1374

nigurr · 2018-01-15T11:02:02Z

Currently vstest console runner doesn't support escaping of , in test filter.
This PR adds support of escaping this by adding \.

smadala · 2018-01-16T06:40:28Z

src/vstest.console/Processors/RunSpecificTestsArgumentProcessor.cs

+                //todo fix the resource file
+                this.selectedTestNames = new Collection<string>(
+                    argument.Tokenize(CommandLineResources.SearchStringDelimiter.ToCharArray()[0], '\\')
+                        .Where(x => !string.IsNullOrWhiteSpace(x))


remove , from resources

mayankbansal018 · 2018-01-16T08:02:05Z

We support "," delimiter, are we now supporting "," & "," delimiter? Why do we need to do so?

mayankbansal018 · 2018-01-16T08:29:46Z

src/vstest.console/Processors/Utilities/StringUtilities.cs

+
+    internal static class StringExtensions
+    {
+        public static IEnumerable<string> Tokenize(this string input, char separator, char escape)


Please add test cases for this class.

Check if input = "\" or "\\" or "\,"

Or make this a private static method until we need another consumer for this.

Please do not make it private, once you do it, it will forever remain in this dll :(. Please move it to Utilities dll, under String Utilities class

mayankbansal018 · 2018-01-16T08:31:25Z

src/vstest.console/Processors/Utilities/StringUtilities.cs

+                    buffer.Append(c);
+                }
+            }
+            if (buffer.Length > 0 || input[input.Length - 1] == separator) yield return buffer.Flush();


What happens if buffer length is 0?

it will not flush. that's an empty stringbuilder

You are returning something back if buffer length > 0, what will it return if it is 0? An empty string, null string? If it returns null will the method invoking it crash?

Please check the UTs.

mayankbansal018 · 2018-01-16T10:51:27Z

src/vstest.console/Processors/Utilities/StringUtilities.cs

@@ -0,0 +1,46 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.


Move this class to Testplatfom.Utilities https://github.com/Microsoft/vstest/tree/master/src/Microsoft.TestPlatform.Utilities

codito · 2018-01-16T13:23:23Z

test/vstest.console.UnitTests/Processors/RunSpecificTestsArgumentProcessorTests.cs

@@ -101,6 +101,51 @@ public void CapabilitiesShouldReturnAppropriateProperties()

        #region RunSpecificTestsArgumentExecutorTests

+        [TestMethod]
+        public void InitializeShouldThrowIfArguemntIsNull()


s/Arguemnt/Argument/g

codito · 2018-01-16T13:24:33Z

test/vstest.console.UnitTests/Processors/RunSpecificTestsArgumentProcessorTests.cs

+        {
+            CommandLineOptions.Instance.Reset();
+            var testRequestManager = new TestRequestManager(CommandLineOptions.Instance, new TestPlatform(), TestLoggerManager.Instance, TestRunResultAggregator.Instance, this.mockTestPlatformEventSource.Object, this.inferHelper, this.mockMetricsPublisherTask);
+            var executor = GetExecutor(testRequestManager);


nit: single blank line between arrange, act and assert == awesome readability :)

codito · 2018-01-16T13:26:48Z

test/vstest.console.UnitTests/Processors/RunSpecificTestsArgumentProcessorTests.cs

@@ -328,6 +371,113 @@ public void ExecutorExecuteShouldForValidSourcesAndValidSelectedTestsRunsTestsAn
            Assert.AreEqual(ArgumentProcessorResult.Success, argumentProcessorResult);
        }

+        [TestMethod]
+        public void RunTestsWithCommaSeperatedTests()


nit: naming should follow <UnitOfWork>Should<Behavior>When<Condition>
nit: every test should have only one reason to fail

These two statements will help understand what feature is broken by just looking at the failed test case, adds to productivity.

codito · 2018-01-16T13:27:45Z

test/vstest.console.UnitTests/Processors/RunSpecificTestsArgumentProcessorTests.cs

+        [TestMethod]
+        public void RunTestsWithFilteredTest()
+        {
+            var mockTestPlatform = new Mock<ITestPlatform>();


How do we remove the DRY violations in these tests?

codito · 2018-01-16T13:32:20Z

src/vstest.console/Processors/Utilities/StringUtilities.cs

+    {
+        public static IEnumerable<string> Tokenize(this string input, char separator, char escape)
+        {
+            if (input == null) yield break;


nit: logic should be in separate line within { } for conditionals. R# with stylecop extension helps catch some of these.

singhsarab · 2018-01-17T09:29:45Z

...Microsoft.TestPlatform.Utilities.UnitTests/Microsoft.TestPlatform.Utilities.UnitTests.csproj

@@ -7,7 +7,7 @@
  <Import Project="$(TestPlatformRoot)scripts/build/TestPlatform.Settings.targets" />
  <PropertyGroup>
    <OutputType Condition=" '$(TargetFramework)' == 'netcoreapp1.0' ">Exe</OutputType>
-    <TargetFrameworks>netcoreapp1.0;net451</TargetFrameworks>
+    <TargetFrameworks>net451;netcoreapp1.0</TargetFrameworks>


Why do we need this? Revert this

Without this VS is not recognizing tests

singhsarab · 2018-01-17T09:34:10Z

test/vstest.console.UnitTests/Processors/RunSpecificTestsArgumentProcessorTests.cs

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


s/Seperate/Separate : everywhere

singhsarab · 2018-01-17T09:40:11Z

test/vstest.console.UnitTests/Processors/RunSpecificTestsArgumentProcessorTests.cs

+            executor.Initialize("Test1, Test2");
+            ArgumentProcessorResult argumentProcessorResult = executor.Execute();
+
+            mockOutput.Verify(o => o.WriteLine(It.IsAny<string>(), OutputLevel.Warning), Times.Never);


Do we need to check for error as well ?

not required unless we are expecting an error

singhsarab · 2018-01-17T09:46:12Z

src/vstest.console/Processors/RunSpecificTestsArgumentProcessor.cs

@@ -83,6 +83,9 @@ internal class RunSpecificTestsArgumentProcessorCapabilities : BaseArgumentProce

    internal class RunSpecificTestsArgumentExecutor : IArgumentExecutor
    {
+        public const char SplitDelimeter = ',';


s/Delimeter/Delimiter : everywhere

singhsarab · 2018-01-17T09:50:06Z

test/Microsoft.TestPlatform.Utilities.UnitTests/StringUtilitiesTests.cs

+            var argsList = data.Tokenize(SplitChar, EscapeChar);
+            var enumerable = argsList as string[] ?? argsList.ToArray();
+
+            Assert.IsTrue(enumerable.Length == 1);


Should we use SequenceEqual instead or have another assert? just length check isn't good.

Added in the cases where we required.

singhsarab · 2018-01-17T09:53:32Z

src/Microsoft.TestPlatform.Utilities/StringUtilities.cs

+    using System.Collections.Generic;
+    using System.Text;
+
+    public static class StringExtensions


There is already vstest/src/Microsoft.TestPlatform.CoreUtilities/Utilities/StringExtensions.cs
Can we move this there ?

🏆Nitin Gurram🏆 added 5 commits January 8, 2018 15:00

Adding Category to Test Category mapping for ListFullyQualifiedTests

c0a0705

Merge branch 'master' into users/nigurr/Fix_Nunit_Xunit_filter

dcc12e8

support for Escaping test cases with comma

437c527

Fix project

da0b940

Merge branch 'master' of https://github.com/microsoft/vstest

14538c8

nigurr requested review from codito and abhishkk January 15, 2018 11:02

Merge branch 'master' into master

990c114

smadala reviewed Jan 16, 2018

View reviewed changes

mayankbansal018 reviewed Jan 16, 2018

View reviewed changes

codito reviewed Jan 16, 2018

View reviewed changes

🏆Nitin Gurram🏆 added 3 commits January 17, 2018 12:58

UTs

41c4aeb

Merge branch 'master' of https://github.com/nigurr/vstest

88ed8da

UTs

1b7279f

mayankbansal018 approved these changes Jan 17, 2018

View reviewed changes

🏆Nitin Gurram🏆 added 2 commits January 17, 2018 14:30

review comments

c341a56

Merge branch 'master' into master

6e6014c

abhishkk approved these changes Jan 17, 2018

View reviewed changes

singhsarab suggested changes Jan 17, 2018

View reviewed changes

🏆Nitin Gurram🏆 added 2 commits January 17, 2018 16:24

CRC

18e6683

Merge branch 'master' of https://github.com/nigurr/vstest

7b31bd5

singhsarab approved these changes Jan 17, 2018

View reviewed changes

nigurr merged commit 46f1cae into microsoft:master Jan 17, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support escaping , in Test filter #1374

Support escaping , in Test filter #1374

nigurr commented Jan 15, 2018

smadala Jan 16, 2018

nigurr Jan 17, 2018

mayankbansal018 commented Jan 16, 2018

mayankbansal018 Jan 16, 2018 •

edited

Loading

codito Jan 16, 2018

mayankbansal018 Jan 17, 2018

nigurr Jan 17, 2018

mayankbansal018 Jan 16, 2018

nigurr Jan 16, 2018

mayankbansal018 Jan 16, 2018

nigurr Jan 17, 2018

mayankbansal018 Jan 16, 2018 •

edited

Loading

nigurr Jan 17, 2018

codito Jan 16, 2018

codito Jan 16, 2018

nigurr Jan 17, 2018

codito Jan 16, 2018

nigurr Jan 17, 2018

codito Jan 16, 2018

codito Jan 16, 2018

singhsarab Jan 17, 2018

nigurr Jan 17, 2018

singhsarab Jan 17, 2018

nigurr Jan 17, 2018

singhsarab Jan 17, 2018

nigurr Jan 17, 2018

singhsarab Jan 17, 2018

nigurr Jan 17, 2018

singhsarab Jan 17, 2018

nigurr Jan 17, 2018

singhsarab Jan 17, 2018

		@@ -0,0 +1,46 @@
		// Copyright (c) Microsoft Corporation. All rights reserved.

Support escaping , in Test filter #1374

Support escaping , in Test filter #1374

Conversation

nigurr commented Jan 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayankbansal018 commented Jan 16, 2018

mayankbansal018 Jan 16, 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

mayankbansal018 Jan 16, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayankbansal018 Jan 16, 2018 •

edited

Loading

mayankbansal018 Jan 16, 2018 •

edited

Loading