-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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 .slnf file format in dotnet test #46392
base: main
Are you sure you want to change the base?
Conversation
…nly have 1 slnf file, then this will work
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.
Excited to see this getting support! :)
@@ -126,5 +127,23 @@ public static SolutionModel CreateFromFilteredSolutionFile(string filteredSoluti | |||
|
|||
return filteredSolution; | |||
} | |||
|
|||
public static string GetSolutionPathFromFilteredSolutionFile(string filteredSolutionPath) |
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.
I know it is just a few lines, and just tossing some thoughts around, but maybe we could create a helper method that parses the .slnf into a valid JsonElement
and have GetSolutionPathFromFilteredSolutionFile
and CreateFromFilteredSolutionFile
call it?
If possible I would advocate to avoiding reading and parsing the file more than once
src/Cli/dotnet/SlnFileFactory.cs
Outdated
using JsonDocument jsonDocument = JsonDocument.Parse(File.ReadAllText(filteredSolutionPath)); | ||
JsonElement jsonElement = jsonDocument.RootElement; | ||
JsonElement filteredSolutionJsonElement = jsonElement.GetProperty("solution"); | ||
string originalSolutionPath = filteredSolutionJsonElement.GetProperty("path").GetString(); | ||
return originalSolutionPath; |
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.
using JsonDocument jsonDocument = JsonDocument.Parse(File.ReadAllText(filteredSolutionPath)); | |
JsonElement jsonElement = jsonDocument.RootElement; | |
JsonElement filteredSolutionJsonElement = jsonElement.GetProperty("solution"); | |
string originalSolutionPath = filteredSolutionJsonElement.GetProperty("path").GetString(); | |
return originalSolutionPath; | |
JsonNode? jsonNode = JsonNode.Parse(File.ReadAllText(filteredSolutionPath)); | |
string? originalSolutionPath = jsonNode?["solution"]?["path"]?.GetValue<string>(); | |
return originalSolutionPath!; |
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.
using System.Text.Json.Nodes;
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.
It's advised to use JsonNode for scenarios where we need to create or modify JSON data and this is not the case.
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.
JsonNode is high performance api for both reading and writing. it's much better than reading a full document just to access selective field, which is this case. plus it has succinct syntax to access props.
Co-authored-by: Eduardo Villalpando Mello <[email protected]>
…ithub.com/dotnet/sdk into dev/mabdullah/support-slnf-in-dotnet-test
src/Cli/dotnet/commands/dotnet-test/SolutionAndProjectUtility.cs
Outdated
Show resolved
Hide resolved
src/Cli/dotnet/commands/dotnet-test/SolutionAndProjectUtility.cs
Outdated
Show resolved
Hide resolved
src/Cli/dotnet/commands/dotnet-test/TestApplicationEventHandlers.cs
Outdated
Show resolved
Hide resolved
…rs.cs Co-authored-by: Amaury Levé <[email protected]>
…ithub.com/dotnet/sdk into dev/mabdullah/support-slnf-in-dotnet-test
} | ||
} | ||
|
||
private static JsonElement ParseSolutionFilterFile(string solutionFilterFilePath) |
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.
Maybe
private static JsonElement ParseSolutionFilterFile(string solutionFilterFilePath) | |
private static JsonElement ParseSolutionFilterFileJson(string solutionFilterFilePath) |
just to make it super clear that it returns a json?
This pull request focuses on refactoring and improving the handling of solution and project files in the CLI, as well as enhancing the build process. The most important changes include the introduction of new methods for parsing solution filter files, simplifying the
MSBuildHandler
class, and updating constants to support solution filter files.Refactoring and Enhancements:
src/Cli/dotnet/SlnFileFactory.cs
: Introduced new methodsGetSolutionPathFromFilteredSolutionFile
andParseSolutionFilterFile
to handle solution filter files, and refactoredCreateFromFilteredSolutionFile
to use these methods. [1] [2] [3] [4]Simplification of
MSBuildHandler
:src/Cli/dotnet/commands/dotnet-test/MSBuildHandler.cs
: Removed redundant methods and simplified the build process by delegating solution and project handling toMSBuildUtility
. Also, removed the degree of parallelism parameter and the validation of build path options. [1] [2] [3] [4] [5]Support for Solution Filter Files:
src/Cli/dotnet/commands/dotnet-test/CliConstants.cs
: Added support for.slnf
files by updating theSolutionExtensions
array and introducingSolutionFilterExtensionPattern
.dotnet test --solution "TestProjects.slnf"
.dotnet test
and there exists only one.slnf
file in the directory, we will run projects inside this .slnf file. But if we have multiple.slnf
files, we will throw an error to specify the solution file. If we have 1 solution file (.sln/.slnx) and multiple .slnf files, we run only the solution file.Relates to #45927