8000 chore: modify escape logics and update tests · dotnet/BenchmarkDotNet@395c27a · GitHub
[go: up one dir, main page]
Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

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< 8000 /code>-
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.
10670 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)
0