-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Add public API to BinaryLogger for parsing parameter strings #12606
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
base: main
Are you sure you want to change the base?
Changes from all commits
cce0281
6b9b0b2
2a28ad4
5b0fe9d
b8d48c2
a54675d
bf28068
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -680,6 +680,74 @@ public void BinlogFileNameWildcardGeneration() | |
| File.Create(_logFile).Dispose(); | ||
| } | ||
|
|
||
| [Theory] | ||
| [InlineData("mylog.binlog", "mylog.binlog", BinaryLogger.ProjectImportsCollectionMode.Embed, false)] | ||
| [InlineData("LogFile=mylog.binlog", "mylog.binlog", BinaryLogger.ProjectImportsCollectionMode.Embed, false)] | ||
| [InlineData("\"mylog.binlog\"", "mylog.binlog", BinaryLogger.ProjectImportsCollectionMode.Embed, false)] | ||
| [InlineData("LogFile=\"mylog.binlog\"", "mylog.binlog", BinaryLogger.ProjectImportsCollectionMode.Embed, false)] | ||
| [InlineData("mylog.binlog;ProjectImports=None", "mylog.binlog", BinaryLogger.ProjectImportsCollectionMode.None, false)] | ||
| [InlineData("ProjectImports=None;mylog.binlog", "mylog.binlog", BinaryLogger.ProjectImportsCollectionMode.None, false)] | ||
| [InlineData("ProjectImports=Embed;mylog.binlog", "mylog.binlog", BinaryLogger.ProjectImportsCollectionMode.Embed, false)] | ||
| [InlineData("ProjectImports=ZipFile;mylog.binlog", "mylog.binlog", BinaryLogger.ProjectImportsCollectionMode.ZipFile, false)] | ||
| [InlineData("mylog.binlog;OmitInitialInfo", "mylog.binlog", BinaryLogger.ProjectImportsCollectionMode.Embed, true)] | ||
| [InlineData("OmitInitialInfo;mylog.binlog", "mylog.binlog", BinaryLogger.ProjectImportsCollectionMode.Embed, true)] | ||
| [InlineData("ProjectImports=None;OmitInitialInfo;mylog.binlog", "mylog.binlog", BinaryLogger.ProjectImportsCollectionMode.None, true)] | ||
| public void ParseParametersTests(string parametersString, string expectedLogFilePath, BinaryLogger.ProjectImportsCollectionMode expectedImportsMode, bool expectedOmitInitialInfo) | ||
| { | ||
| var result = BinaryLogger.ParseParameters(parametersString); | ||
|
|
||
| result.LogFilePath.ShouldBe(expectedLogFilePath); | ||
| result.ProjectImportsCollectionMode.ShouldBe(expectedImportsMode); | ||
| result.OmitInitialInfo.ShouldBe(expectedOmitInitialInfo); | ||
|
|
||
| // Create the expected log file to satisfy test environment expectations | ||
| File.Create(_logFile).Dispose(); | ||
| } | ||
|
|
||
| [Theory] | ||
| [InlineData("{}")] // Wildcard without extension | ||
| [InlineData("{}.binlog")] // Wildcard with extension | ||
| [InlineData("mylog-{}.binlog")] // Wildcard with prefix | ||
| [InlineData("LogFile={}.binlog")] // Wildcard with LogFile= prefix | ||
| public void ParseParameters_WildcardPath_ReturnsNullPath(string parametersString) | ||
| { | ||
| using (TestEnvironment env = TestEnvironment.Create()) | ||
| { | ||
| // Enable Wave17_12 to support wildcard parameters | ||
| ChangeWaves.ResetStateForTests(); | ||
| env.SetEnvironmentVariable("MSBUILDDISABLEFEATURESFROMVERSION", ""); | ||
| BuildEnvironmentHelper.ResetInstance_ForUnitTestsOnly(); | ||
|
|
||
| var result = BinaryLogger.ParseParameters(parametersString); | ||
|
|
||
| result.LogFilePath.ShouldBeNull(); | ||
| } | ||
|
|
||
| // Create the expected log file to satisfy test environment expectations | ||
| File.Create(_logFile).Dispose(); | ||
|
Comment on lines
+726
to
+727
|
||
| } | ||
|
|
||
| [Fact] | ||
| public void ParseParameters_NullParameter_ThrowsLoggerException() | ||
| { | ||
| Should.Throw<LoggerException>(() => BinaryLogger.ParseParameters(null)); | ||
|
|
||
| // Create the expected log file to satisfy test environment expectations | ||
| File.Create(_logFile).Dispose(); | ||
|
Comment on lines
+735
to
+736
|
||
| } | ||
|
|
||
| [Theory] | ||
| [InlineData("invalidparameter")] | ||
| [InlineData("mylog.txt")] // Wrong extension | ||
| [InlineData("LogFile=mylog.txt")] // Wrong extension with LogFile prefix | ||
| public void ParseParameters_InvalidParameter_ThrowsLoggerException(string parametersString) | ||
| { | ||
| Should.Throw<LoggerException>(() => BinaryLogger.ParseParameters(parametersString)); | ||
|
|
||
| // Create the expected log file to satisfy test environment expectations | ||
| File.Create(_logFile).Dispose(); | ||
|
Comment on lines
+747
to
+748
|
||
| } | ||
|
|
||
| public void Dispose() | ||
| { | ||
| _env.Dispose(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -13,6 +13,32 @@ | |||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| namespace Microsoft.Build.Logging | ||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||
| /// <summary> | ||||||||||||||||||||||||||
| /// Represents the parsed parameters for a BinaryLogger. | ||||||||||||||||||||||||||
| /// </summary> | ||||||||||||||||||||||||||
| public sealed class BinaryLoggerParameters | ||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||
| /// <summary> | ||||||||||||||||||||||||||
| /// Gets the log file path. May be null if not specified (defaults to "msbuild.binlog"). | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
| /// Gets the log file path. May be null if not specified (defaults to "msbuild.binlog"). | |
| /// Gets the log file path. Returns null if not specified or if the path contains wildcards. |
Copilot
AI
Oct 27, 2025
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.
The documentation should clarify why this property is needed. Consider adding: 'This is used to preserve manually set CollectProjectImports values when Initialize() is called without an explicit ProjectImports parameter.'
| /// Gets whether the ProjectImports parameter was explicitly specified in the parameters string. | |
| /// Gets whether the ProjectImports parameter was explicitly specified in the parameters string. | |
| /// This is used to preserve manually set CollectProjectImports values when Initialize() is called | |
| /// without an explicit ProjectImports parameter. |
YuliiaKovalova marked this conversation as resolved.
Show resolved
Hide resolved
Copilot
AI
Oct 27, 2025
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.
The exception documentation in the method summary states 'Thrown when the parameters string contains invalid parameters' but this throws for null input. Consider updating the exception documentation to: 'Thrown when the parameters string is null or contains invalid parameters.'
Copilot
AI
Oct 27, 2025
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.
Calling ParseParameters(Parameters) will throw LoggerException if Parameters is null, but the original implementation allowed null Parameters and would throw with a specific error. This changes the error handling behavior. Consider handling null Parameters before calling ParseParameters or documenting this breaking change.
| { | |
| { | |
| if (Parameters == null) | |
| { | |
| string errorCode; | |
| string helpKeyword; | |
| string message = ResourceUtilities.FormatResourceStringStripCodeAndKeyword( | |
| out errorCode, | |
| out helpKeyword, | |
| "LoggerParametersNull"); | |
| throw new LoggerException(message, errorCode, helpKeyword); | |
| } |
Copilot
AI
Oct 27, 2025
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 code re-parses the Parameters string even though we just parsed it in ParseParameters. This is inefficient and creates duplicate parsing logic. Consider having ParseParameters track whether it found a wildcard and store that information, or return all path parameters found (including wildcards) so we don't need to re-parse.
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.
The test creates _logFile but uses expectedLogFilePath from test data. This creates a file with a different name than what's being tested. Either create a file matching expectedLogFilePath or remove this line if the test doesn't actually require a file to exist.