Skip to content

Commit 395c27a

Browse files
filzrevfilzrev
committed
chore: modify escape logics and update tests
1 parent 4910040 commit 395c27a

File tree

4 files changed

+159
-74
lines changed

4 files changed

+159
-74
lines changed

‎src/BenchmarkDotNet/Jobs/Argument.cs

Lines changed: 31 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,9 @@
1-
using System;
2-
using JetBrains.Annotations;
1+
using JetBrains.Annotations;
2+
using System;
33

44
namespace BenchmarkDotNet.Jobs
55
{
6-
public abstract class Argument: IEquatable<Argument>
6+
public abstract class Argument : IEquatable<Argument>
77
{
88
[PublicAPI] public string TextRepresentation { get; }
99

@@ -47,13 +47,17 @@ public MonoArgument(string value) : base(value)
4747
[PublicAPI]
4848
public class MsBuildArgument : Argument
4949
{
50-
// Characters that need to be escaped.
51-
// 1. Space
52-
// 2. Comma (Special char that is used for separater for value of `-property:{name}={value}` and `-restoreProperty:{name}={value}`)
53-
// 3. Other MSBuild special chars (https://learn.microsoft.com/en-us/visualstudio/msbuild/msbuild-special-characters?view=vs-2022)
54-
private static readonly char[] MSBuildCharsToEscape = [' ', ',', '%', '$', '@', '\'', '(', ')', ';', '?', '*'];
50+
// Specisal chars that need to be wrapped with `\"`.
51+
// 1. Comma char (It's used for separater char for `-property:{name}={value}` and `-restoreProperty:{name}={ value}`)
52+
// 2. MSBuild special chars (https://learn.microsoft.com/en-us/visualstudio/msbuild/msbuild-special-characters?view=vs-2022)
53+
private static readonly char[] MSBuildSpecialChars = [',', '%', '$', '@', '\'', '(', ')', ';', '?', '*'];
5554

56-
public MsBuildArgument(string value) : base(value) { }
55+
private readonly bool escapeArgument;
56+
57+
public MsBuildArgument(string value, bool escape = false) : base(value)
58+
{
59+
escapeArgument = escape;
60+
}
5761

5862
/// <summary>
5963
/// Gets the MSBuild argument that is used for build script.
@@ -62,6 +66,9 @@ internal string GetEscapedTextRepresentation()
6266
{
6367
var originalArgument = TextRepresentation;
6468

69+
if (!escapeArgument)
70+
return originalArgument;
71+
6572
// If entire argument surrounded with double quote, returns original argument.
6673
// In this case. MSBuild special chars must be escaped by user. https://learn.microsoft.com/en-us/visualstudio/msbuild/msbuild-special-characters
6774
if (originalArgument.StartsWith("\""))
@@ -76,41 +83,27 @@ internal string GetEscapedTextRepresentation()
7683
var key = values[0];
7784
var value = values[1];
7885

79-
// If value starts with `\` char. It is expected that the escaped value is specified by the user.
80-
if (value.StartsWith("\\"))
86+
// If value starts with `\"`.
87+
// It is expected that the escaped value is specified by the user.
88+
if (value.StartsWith("\\\""))
8189
return originalArgument;
8290

83-
// If value don't contains special chars. return original value.
84-
if (value.IndexOfAny(MSBuildCharsToEscape) < 0)
85-
return originalArgument;
91+
// If value is wrapped with double quote. Trim leading/trailing double quote.
92+
if (value.StartsWith("\"") && value.EndsWith("\""))
93+
value = value.Trim(['"']);
8694

87-
return $"{key}={GetEscapedValue(value)}";
88-
}
95+
// Escape chars that need to escaped when wrapped with escaped double quote (`\"`)
96+
value = value.Replace(" ", "%20") // Space
97+
.Replace("\"", "%22") // Double Quote
98+
.Replace("\\", "%5C"); // BackSlash
8999

90-
private static string GetEscapedValue(string value)
91-
{
92-
// If value starts with double quote. Trim leading/trailing double quote
93-
if (value.StartsWith("\""))
94-
value = value.Trim(['"']);
100+
// If escaped value doesn't contains MSBuild special char, return original argument.
101+
if (value.IndexOfAny(MSBuildSpecialChars) < 0)
102+
return originalArgument;
95103

96-
bool isWindows = true;
97-
#if NET
98-
isWindows = OperatingSystem.IsWindows();
99-
#endif
100-
if (isWindows)
101-
{
102-
// On Windows environment.
103-
// Returns double-quoted value. (Command line execution and `.bat` file requires escape double quote with `\`)
104-
return $"""
105-
\"{value}\"
106-
""";
107-
}
108-
109-
// On non-Windows environment.
110-
// Returns value that surround with `'"` and `"'`. See: https://github.com/dotnet/sdk/issues/8792#issuecomment-393756980
111-
// It requires escape with `\` when running command with `.sh` file. )
104+
// Return escaped value that is wrapped with escaped double quote (`\"`)
112105
return $"""
113-
\'\"{value}\"\'
106+
{key}=\"{value}\"
114107
""";
115108
}
116109
}

‎tests/BenchmarkDotNet.IntegrationTests.ManualRunning/BenchmarkDotNet.IntegrationTests.ManualRunning.csproj

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,15 @@
1515
<PropertyGroup Condition="'$(CustomProp)'=='true'">
1616
<DefineConstants>$(DefineConstants);CUSTOM_PROP</DefineConstants>
1717
</PropertyGroup>
18-
18+
19+
<!-- Set AssemblyMetadataAttribute that is used by MsBuildArgumentTests-->
20+
<ItemGroup Condition="'$(CustomProp)' != ''">
21+
<AssemblyAttribute Include="System.Reflection.AssemblyMetadataAttribute">
22+
<_Parameter1>CustomProp</_Parameter1>
23+
<_Parameter2>$(CustomProp)</_Parameter2>
24+
</AssemblyAttribute>
25+
</ItemGroup>
26+
1927
<ItemGroup>
2028
<Compile Include="..\BenchmarkDotNet.IntegrationTests\BenchmarkTestExecutor.cs" Link="BenchmarkTestExecutor.cs" />
2129
<Compile Include="..\BenchmarkDotNet.IntegrationTests\Xunit\MisconfiguredEnvironmentException.cs" Link="MisconfiguredEnvironmentException.cs" />
@@ -41,4 +49,5 @@
4149
<IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
4250
</PackageReference>
4351
</ItemGroup>
52+
4453
</Project>

‎tests/BenchmarkDotNet.IntegrationTests.ManualRunning/MsBuildArgumentTests.cs

Lines changed: 76 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,11 @@
1-
using System;
2-
using BenchmarkDotNet.Attributes;
1+
using BenchmarkDotNet.Attributes;
32
using BenchmarkDotNet.Configs;
3+
using BenchmarkDotNet.Engines;
44
using BenchmarkDotNet.Environments;
55
using BenchmarkDotNet.Jobs;
6+
using System;
7+
using System.Linq;
8+
using System.Reflection;
69
using Xunit;
710

811
namespace BenchmarkDotNet.IntegrationTests.ManualRunning
@@ -61,5 +64,75 @@ public void ThrowWhenWrong()
6164
}
6265
}
6366
}
67+
68+
private const string AsciiChars = " !\"#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_`abcdefghijklmnopqrstuvwxyz{|}~";
69+
70+
[Theory]
71+
[InlineData("AAA;BBB")] // Contains separator char (`;`)
72+
[InlineData(AsciiChars)] // Validate all ASCII char patterns.
73+
// Following tests are commented out by default. Because it takes time to execute.
74+
//[InlineData("AAA;BBB,CCC")] // Validate argument that contains semicolon/comma separators.
75+
//[InlineData("AAA BBB")] // Contains space char
76+
//[InlineData("AAA\"BBB")] // Contains double quote char
77+
//[InlineData("AAA\\BBB")] // Contains backslash char
78+
//[InlineData("\"AAA")] // StartsWith `"` but not ends with `"`
79+
//[InlineData("AAA\"")] // EndsWith `"` but not ends with `"`
80+
//[InlineData("\"AAA;BBB\"", "AAA;BBB")] // Surrounded with `"`
81+
//[InlineData("\\\"AAA%3BBBB\\\"", "AAA;BBB")] // Surrounded with `\"` and escaped `;`
82+
public void ValidateEscapedMsBuildArgument(string propertyValue, string? expectedValue = null)
83+
{
84+
// Arrange
85+
expectedValue ??= propertyValue;
86+
var config = ManualConfig.CreateEmpty()
87+
.WithOption(ConfigOptions.DisableOptimizationsValidator, true)
88+
.AddJob(Job.Dry
89+
.WithStrategy(RunStrategy.Monitoring)
90+
.WithArguments([new MsBuildArgument($"/p:CustomProp={propertyValue}", escape: true)])
91+
.WithEnvironmentVariable(CustomPropEnvVarName, expectedValue)
92+
);
93+
94+
// Act
95+
var summary = CanExecute<ValidateEscapedValueBenchmark>(config, fullValidation: false);
96+
97+
// Assert
98+
Assert.False(summary.HasCriticalValidationErrors);
99+
Assert.True(summary.Reports.Any());
100+
foreach (var report in summary.Reports)
101+
{
102+
if (!report.GenerateResult.IsGenerateSuccess)
103+
{
104+
var message = report.GenerateResult.GenerateException?.ToString();
105+
Assert.Fail($"Failed to generate benchmark project:{Environment.NewLine}{message}");
106+
}
107+
108+
if (!report.BuildResult.IsBuildSuccess)
109+
Assert.Fail($"Failed to build benchmark project:{Environment.NewLine}{report.BuildResult.ErrorMessage}");
110+
111+
foreach (var executeResult in report.ExecuteResults)
112+
{
113+
if (!executeResult.IsSuccess)
114+
{
115+
string message = string.Join(Environment.NewLine, executeResult.Results).Trim();
116+
Assert.Fail($"Failed to run benchmark({report.BenchmarkCase.Descriptor.DisplayInfo}):{Environment.NewLine}{message}");
117+
}
118+
}
119+
}
120+
}
121+
122+
public class ValidateEscapedValueBenchmark
123+
{
124+
[Benchmark]
125+
public void Validate()
126+
{
127+
// Gets expected value from environment variable.
128+
var expected = Environment.GetEnvironmentVariable(CustomPropEnvVarName);
129+
130+
// Gets MSBuild property from AssemblyMetadataAttribute (This attribute is set by csproj)
131+
var result = typeof(MsBuildArgumentTests).Assembly.GetCustomAttributes<AssemblyMetadataAttribute>().Single(x => x.Key == "CustomProp").Value;
132+
133+
if (result != expected)
134+
throw new Exception($"Failed to run benchmark. Expected:{expected} Actual : {result}");
135+
}
136+
}
64137
}
65-
}
138+
}

‎tests/BenchmarkDotNet.Tests/Jobs/MsBuildArgumentTests.cs

Lines changed: 42 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -10,46 +10,56 @@ public class MsBuildArgumentTests
1010
[InlineData("/p:CustomProperty=100%", "100%")] // Contains percentage
1111
[InlineData("/p:CustomProperty=AAA;BBB", "AAA;BBB")] // Contains semicolon
1212
[InlineData("/p:CustomProperty=AAA,BBB", "AAA,BBB")] // Contains comma
13-
[InlineData("/p:CustomProperty=AAA BBB", "AAA BBB")] // Contains space
14-
[InlineData("/p:CustomProperty=AAA%3BBBB", "AAA%3BBBB")] // Contains escaped semicolon
15-
[InlineData("/p:CustomProperty=\"AAA%3BBBB\"", "AAA%3BBBB")] // Contains escaped semicolon. and surrounded with non-escaped double quote
13+
[InlineData("/p:CustomProperty=AAA BBB", "AAA%20BBB")] // Contains space (It's escaped to `%20`)
14+
[InlineData("/p:CustomProperty=AAA\"BBB", "AAA%22BBB")] // Contains double quote (It's escaped to `%22B`)
15+
[InlineData("/p:CustomProperty=AAA\\BBB", "AAA%5CBBB")] // Contains backslash (It's escaped to `%5C`)
16+
[InlineData("/p:CustomProperty=AAA%3BBBB", "AAA%3BBBB")] // Contains escaped semicolon (`%3B`)
17+
[InlineData("/p:CustomProperty=\"AAA%3BBBB\"", "AAA%3BBBB")] // Contains escaped semicolon (`%3B`), and surrounded with double quote
1618
[InlineData("/p:NoWarn=1591;1573;3001", "1591;1573;3001")] // NoWarn property that contains semicolon
17-
public void MSBuildArgument_ContainsSpecialChars(string argument, string expected)
19+
public void MSBuildArgument_ContainsMSBuildSpecialChars(string argument, string expected)
1820
{
19-
var result = new MsBuildArgument(argument).GetEscapedTextRepresentation();
20-
AssertEscapedValue(result, expected);
21-
}
21+
// Arrange
22+
var msbuildArgument = new MsBuildArgument(argument, escape: true);
2223

23-
[Theory]
24-
[InlineData("/p:CustomProperty=AAA_BBB")] // Argument that don't contains special chars
25-
[InlineData("\"/p:CustomProperty=AAA%3BBBB\"")] // Argument is surrounded with double quote and value is escaped
26-
[InlineData("/p:CustomProperty=\\\"100%3B200\\\"")] // Value is surrounded with double quote and value is escaped (For Windows environment)
27-
[InlineData("/p:CustomProperty=\\\'\\\"100%3B200\\\"\\\'")] // Value is surrounded with double quote and value is escaped (For non-Windows environment)
28-
[InlineData("/noWarn:1591;1573;3001")] // Other argument should not be escaped
29-
public void MSBuildArgument_ShouldNotBeEscaped(string originalArg)
30-
{
31-
var result = new MsBuildArgument(originalArg).GetEscapedTextRepresentation();
32-
Assert.Equal(expected: originalArg, result);
24+
// Act
25+
var result = msbuildArgument.GetEscapedTextRepresentation();
26+
27+
// Assert
28+
AssertEscapedValue(expected, result);
29+
30+
// Helper method
31+
static void AssertEscapedValue(string expectedValue, string argument)
32+
{
33+
var values = argument.Split(['='], 2);
34+
Assert.Equal(2, values.Length);
35+
36+
var key = values[0];
37+
var value = values[1];
38+
39+
// Assert value is wrapped with `\"`
40+
Assert.StartsWith("\\\"", value);
41+
Assert.EndsWith("\\\"", value);
42+
43+
value = value.Substring(2, value.Length - 4);
44+
Assert.Equal(expectedValue, value);
45+
}
3346
}
3447

35-
private static void AssertEscapedValue(string escapedArgument, string expectedValue)
48+
[Theory]
49+
[InlineData("/p:CustomProperty=AAA_BBB")] // Argument that don't contains special chars
50+
[InlineData("\"/p:CustomProperty=AAA%3BBBB\"")] // Argument is surrounded with double quote and value is escaped
51+
[InlineData("/p:CustomProperty=\\\"100%3B200\\\"")] // Value is surrounded with double quote and value is escaped (For Windows environment)
52+
[InlineData("/noWarn:1591;1573;3001")] // Other argument that don't contains `=` should not be escaped
53+
public void MSBuildArgument_ShouldNotBeEscaped(string argument)
3654
{
37-
var values = escapedArgument.Split(['='], 2);
38-
Assert.Equal(2, values.Length);
55+
// Arrange
56+
var msbuildArgument = new MsBuildArgument(argument, escape: true);
3957

40-
var key = values[0];
41-
var value = values[1];
58+
// Act
59+
var result = msbuildArgument.GetEscapedTextRepresentation();
4260

43-
#if NET
44-
// On non-Windows environment value should be surrounded with escaped `'"` and `"'`
45-
if (!OperatingSystem.IsWindows())
46-
{
47-
Assert.Equal("\\\'\\\"" + expectedValue + "\\\"\\\'", value);
48-
return;
49-
}
50-
#endif
51-
// On Windows environment. Value should be quote with escaped `\"`
52-
Assert.Equal("\\\"" + expectedValue + "\\\"", value);
61+
// Assert
62+
Assert.Equal(argument, result);
5363
}
5464
}
5565
}

0 commit comments

Comments
 (0)