Skip to content
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

Support escaping , in Test filter #1374

Merged
merged 13 commits into from
Jan 17, 2018
46 changes: 46 additions & 0 deletions src/Microsoft.TestPlatform.Utilities/StringUtilities.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

namespace Microsoft.VisualStudio.TestPlatform.Utilities
{
using System.Collections.Generic;
using System.Text;

public static class StringExtensions
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
public static IEnumerable<string> Tokenize(this string input, char separator, char escape)
{
if (string.IsNullOrEmpty(input)) yield break;
var buffer = new StringBuilder();
var escaping = false;
foreach (var c in input)
{
if (escaping)
{
buffer.Append(c);
escaping = false;
}
else if (c == escape)
{
escaping = true;
}
else if (c == separator)
{
yield return buffer.Flush();
}
else
{
buffer.Append(c);
}
}
if (buffer.Length > 0 || input[input.Length - 1] == separator) yield return buffer.Flush();
}

private static string Flush(this StringBuilder stringBuilder)
{
var result = stringBuilder.ToString();
stringBuilder.Clear();
return result;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,13 @@ namespace Microsoft.VisualStudio.TestPlatform.CommandLine.Processors

using Microsoft.VisualStudio.TestPlatform.Client.RequestHelper;
using Microsoft.VisualStudio.TestPlatform.CommandLine.TestPlatformHelpers;
using Microsoft.VisualStudio.TestPlatform.CommandLine.Processors.Utilities;
using Microsoft.VisualStudio.TestPlatform.CommandLineUtilities;
using Microsoft.VisualStudio.TestPlatform.Common;
using Microsoft.VisualStudio.TestPlatform.Common.Interfaces;
using Microsoft.VisualStudio.TestPlatform.ObjectModel;
using Microsoft.VisualStudio.TestPlatform.ObjectModel.Client;
using Microsoft.VisualStudio.TestPlatform.Utilities;

using CommandLineResources = Microsoft.VisualStudio.TestPlatform.CommandLine.Resources.Resources;

internal class RunSpecificTestsArgumentProcessor : IArgumentProcessor
Expand Down Expand Up @@ -83,6 +83,9 @@ internal class RunSpecificTestsArgumentProcessorCapabilities : BaseArgumentProce

internal class RunSpecificTestsArgumentExecutor : IArgumentExecutor
{
public const char SplitDelimeter = ',';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/Delimeter/Delimiter : everywhere

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

public const char EscapeDelimeter = '\\';

#region Fields

/// <summary>
Expand Down Expand Up @@ -171,7 +174,10 @@ public void Initialize(string argument)
{
if (!string.IsNullOrWhiteSpace(argument))
{
this.selectedTestNames = new Collection<string>(argument.Split(new[] { CommandLineResources.SearchStringDelimiter }, StringSplitOptions.RemoveEmptyEntries));
this.selectedTestNames = new Collection<string>(
argument.Tokenize(SplitDelimeter, EscapeDelimeter)
.Where(x => !string.IsNullOrWhiteSpace(x))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove , from resources

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

.Select(s => s.Trim()).ToList());
}

if (this.selectedTestNames == null || this.selectedTestNames.Count <= 0)
Expand All @@ -194,7 +200,7 @@ public ArgumentProcessorResult Execute()
Contract.Assert(this.testRequestManager != null);
Contract.Assert(!string.IsNullOrWhiteSpace(this.runSettingsManager.ActiveRunSettings.SettingsXml));

if (this.commandLineOptions.Sources.Count() <= 0)
if (!this.commandLineOptions.Sources.Any())
{
throw new CommandLineException(string.Format(CultureInfo.CurrentUICulture, CommandLineResources.MissingTestSourceFile));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this? Revert this

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without this VS is not recognizing tests

<AssemblyName>Microsoft.TestPlatform.Utilities.UnitTests</AssemblyName>
</PropertyGroup>
<ItemGroup>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

using System.Linq;

namespace Microsoft.TestPlatform.Utilities.UnitTests
{
using Castle.Core.Internal;
using Microsoft.VisualStudio.TestPlatform.Utilities;
using Microsoft.VisualStudio.TestTools.UnitTesting;

[TestClass]
public class StringUtilitiesTests
{
[TestMethod]
public void EmptyNullStringShouldNotSplit()
{
var argsList = string.Empty.Tokenize(SplitChar, EscapeChar);
Assert.IsTrue(argsList.IsNullOrEmpty());
}

[TestMethod]
public void SplitStringDoesntContainSplitChar()
{
var data = "foobar";
var argsList = data.Tokenize(SplitChar, EscapeChar);
var enumerable = argsList as string[] ?? argsList.ToArray();
Assert.IsTrue(enumerable.Length == 1);
Assert.IsTrue(enumerable.First().Equals(data));
}

[TestMethod]
public void SplitStringSplitsBySplitChar()
{
var data = "foo,bar";
var argsList = data.Tokenize(SplitChar, EscapeChar);
var enumerable = argsList as string[] ?? argsList.ToArray();
Assert.IsTrue(enumerable.Length == 2);
}

[TestMethod]
public void SplitStringSplitsBySplitCharWithStartEnd()
{
var data = ",foo,bar,";
var argsList = data.Tokenize(SplitChar, EscapeChar);
var enumerable = argsList as string[] ?? argsList.ToArray();
Assert.IsTrue(enumerable.Length == 4);
}

[TestMethod]
public void SplitStringSplitsWithEscapedChar()
{
var data = "foo\\,bar";
var argsList = data.Tokenize(SplitChar, EscapeChar);
var enumerable = argsList as string[] ?? argsList.ToArray();
Assert.IsTrue(enumerable.Length == 1);
Assert.IsTrue(enumerable.First().Equals("foo,bar"));
}

[TestMethod]
public void SplitStringSplitsWithEscapedCharWithSeperator()
{
var data = "foo\\,,bar";
var argsList = data.Tokenize(SplitChar, EscapeChar);
var enumerable = argsList as string[] ?? argsList.ToArray();
Assert.IsTrue(enumerable.Length == 2);
Assert.IsTrue(enumerable.First().Equals("foo,"));
}

[TestMethod]
public void SplitStringOnlyWithSplitChar()
{
var data = ",";
var argsList = data.Tokenize(SplitChar, EscapeChar);
var enumerable = argsList as string[] ?? argsList.ToArray();
Assert.IsTrue(enumerable.Length == 2);
}

[TestMethod]
public void SplitStringOnlyWithEscapeCharOnly()
{
var data = "foo\\bar";
var argsList = data.Tokenize(SplitChar, EscapeChar);
var enumerable = argsList as string[] ?? argsList.ToArray();
Assert.IsTrue(enumerable.Length == 1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in the cases where we required.

}

private const char SplitChar = ',';
private const char EscapeChar = '\\';
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,51 @@ public void CapabilitiesShouldReturnAppropriateProperties()

#region RunSpecificTestsArgumentExecutorTests

[TestMethod]
public void InitializeShouldThrowIfArguemntIsNull()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/Arguemnt/Argument/g

{
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);
Assert.ThrowsException<CommandLineException>(() => { executor.Initialize(null); });
}

[TestMethod]
public void InitializeShouldThrowIfArgumentIsEmpty()
{
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

noted :)

Assert.ThrowsException<CommandLineException>(() => { executor.Initialize(String.Empty); });
}

[TestMethod]
public void InitializeShouldThrowIfArgumentIsWhiteSpace()
{
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);
Assert.ThrowsException<CommandLineException>(() => { executor.Initialize(" "); });
}

[TestMethod]
public void InitializeShouldThrowIfArgumentsAreEmpty()
{
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);
Assert.ThrowsException<CommandLineException>(() => { executor.Initialize(" , "); });
}

[TestMethod]
public void ExecutorShouldSplitTestsSeperatedByComma()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/Seperate/Separate : everywhere

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

{
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);
Assert.ThrowsException<CommandLineException>(() => executor.Execute());
}

[TestMethod]
public void ExecutorExecuteForNoSourcesShouldThrowCommandLineException()
{
Expand Down Expand Up @@ -260,7 +305,6 @@ public void ExecutorExecuteShouldCatchInvalidOperationExceptionThrownDuringExecu
public void ExecutorExecuteShouldForValidSourcesAndNoTestsDiscoveredShouldLogWarningAndReturnSuccess()
{
var mockTestPlatform = new Mock<ITestPlatform>();
var mockTestRunRequest = new Mock<ITestRunRequest>();
var mockDiscoveryRequest = new Mock<IDiscoveryRequest>();

this.ResetAndAddSourceToCommandLineOptions();
Expand All @@ -285,7 +329,6 @@ public void ExecutorExecuteShouldForValidSourcesAndNoTestsDiscoveredShouldLogWar
public void ExecutorExecuteShouldForValidSourcesAndNoTestsDiscoveredShouldLogAppropriateWarningIfTestAdapterPathIsNotSetAndReturnSuccess()
{
var mockTestPlatform = new Mock<ITestPlatform>();
var mockTestRunRequest = new Mock<ITestRunRequest>();
var mockDiscoveryRequest = new Mock<IDiscoveryRequest>();

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

[TestMethod]
public void RunTestsWithCommaSeperatedTests()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

{
var mockTestPlatform = new Mock<ITestPlatform>();
var mockTestRunRequest = new Mock<ITestRunRequest>();
var mockDiscoveryRequest = new Mock<IDiscoveryRequest>();

this.ResetAndAddSourceToCommandLineOptions();

List<TestCase> list = new List<TestCase>();
list.Add(new TestCase("Test1", new Uri("http://FooTestUri1"), "Source1"));
list.Add(new TestCase("Test2", new Uri("http://FooTestUri1"), "Source1"));
mockDiscoveryRequest.Setup(dr => dr.DiscoverAsync()).Raises(dr => dr.OnDiscoveredTests += null, new DiscoveredTestsEventArgs(list));

mockTestPlatform.Setup(tp => tp.CreateTestRunRequest(It.IsAny<IRequestData>(), It.IsAny<TestRunCriteria>())).Returns(mockTestRunRequest.Object);
mockTestPlatform.Setup(tp => tp.CreateDiscoveryRequest(It.IsAny<IRequestData>(), It.IsAny<DiscoveryCriteria>())).Returns(mockDiscoveryRequest.Object);

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

executor.Initialize("Test1, Test2");

ArgumentProcessorResult argumentProcessorResult = executor.Execute();
this.mockOutput.Verify(o => o.WriteLine(It.IsAny<string>(), OutputLevel.Warning), Times.Never);
Assert.AreEqual(ArgumentProcessorResult.Success, argumentProcessorResult);
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do we remove the DRY violations in these tests?

var mockTestRunRequest = new Mock<ITestRunRequest>();
var mockDiscoveryRequest = new Mock<IDiscoveryRequest>();

this.ResetAndAddSourceToCommandLineOptions();

List<TestCase> list = new List<TestCase>();
list.Add(new TestCase("Test1", new Uri("http://FooTestUri1"), "Source1"));
list.Add(new TestCase("Test2", new Uri("http://FooTestUri1"), "Source1"));
mockDiscoveryRequest.Setup(dr => dr.DiscoverAsync()).Raises(dr => dr.OnDiscoveredTests += null, new DiscoveredTestsEventArgs(list));

mockTestPlatform.Setup(tp => tp.CreateTestRunRequest(It.IsAny<IRequestData>(), It.IsAny<TestRunCriteria>())).Returns(mockTestRunRequest.Object);
mockTestPlatform.Setup(tp => tp.CreateDiscoveryRequest(It.IsAny<IRequestData>(), It.IsAny<DiscoveryCriteria>())).Returns(mockDiscoveryRequest.Object);

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

executor.Initialize("Test1");

ArgumentProcessorResult argumentProcessorResult = executor.Execute();
this.mockOutput.Verify(o => o.WriteLine(It.IsAny<string>(), OutputLevel.Warning), Times.Never);
Assert.AreEqual(ArgumentProcessorResult.Success, argumentProcessorResult);
}

[TestMethod]
public void RunTestsWithNonAvailableTest()
{
var mockTestPlatform = new Mock<ITestPlatform>();
var mockTestRunRequest = new Mock<ITestRunRequest>();
var mockDiscoveryRequest = new Mock<IDiscoveryRequest>();

this.ResetAndAddSourceToCommandLineOptions();

List<TestCase> list = new List<TestCase>();
list.Add(new TestCase("Test2", new Uri("http://FooTestUri1"), "Source1"));
mockDiscoveryRequest.Setup(dr => dr.DiscoverAsync()).Raises(dr => dr.OnDiscoveredTests += null, new DiscoveredTestsEventArgs(list));

mockTestPlatform.Setup(tp => tp.CreateTestRunRequest(It.IsAny<IRequestData>(), It.IsAny<TestRunCriteria>())).Returns(mockTestRunRequest.Object);
mockTestPlatform.Setup(tp => tp.CreateDiscoveryRequest(It.IsAny<IRequestData>(), It.IsAny<DiscoveryCriteria>())).Returns(mockDiscoveryRequest.Object);

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

executor.Initialize("Test1, Test2");

ArgumentProcessorResult argumentProcessorResult = executor.Execute();
this.mockOutput.Verify(o => o.WriteLine("A total of 1 tests were discovered but some tests do not match the specified selection criteria(Test1). Use right value(s) and try again.", OutputLevel.Warning), Times.Once);
Assert.AreEqual(ArgumentProcessorResult.Success, argumentProcessorResult);
}

[TestMethod]
public void RunTestsWithEscapedCommaTests()
{
var mockTestPlatform = new Mock<ITestPlatform>();
var mockTestRunRequest = new Mock<ITestRunRequest>();
var mockDiscoveryRequest = new Mock<IDiscoveryRequest>();

this.ResetAndAddSourceToCommandLineOptions();

List<TestCase> list = new List<TestCase>();
list.Add(new TestCase("Test1(a,b)", new Uri("http://FooTestUri1"), "Source1"));
list.Add(new TestCase("Test2(c,d)", new Uri("http://FooTestUri1"), "Source1"));
mockDiscoveryRequest.Setup(dr => dr.DiscoverAsync()).Raises(dr => dr.OnDiscoveredTests += null, new DiscoveredTestsEventArgs(list));

mockTestPlatform.Setup(tp => tp.CreateTestRunRequest(It.IsAny<IRequestData>(), It.IsAny<TestRunCriteria>())).Returns(mockTestRunRequest.Object);
mockTestPlatform.Setup(tp => tp.CreateDiscoveryRequest(It.IsAny<IRequestData>(), It.IsAny<DiscoveryCriteria>())).Returns(mockDiscoveryRequest.Object);

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

executor.Initialize("Test1(a\\,b), Test2(c\\,d)");

ArgumentProcessorResult argumentProcessorResult = executor.Execute();
this.mockOutput.Verify(o => o.WriteLine(It.IsAny<string>(), OutputLevel.Warning), Times.Never);
Assert.AreEqual(ArgumentProcessorResult.Success, argumentProcessorResult);
}

#endregion

private void ResetAndAddSourceToCommandLineOptions()
Expand Down