-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add unit test for Native AOT publishing with space in path #51656
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?
Conversation
Add a new test case that verifies Native AOT publishing works correctly when the project path contains a space. This test:
- Creates a test project with an identifier containing a space ("with space")
- Publishes the project with PublishAot=true
- Verifies the path actually contains a space
- Confirms the native executable is created and runs successfully
Co-authored-by: agocke <515774+agocke@users.noreply.github.com>
agocke
left a comment
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.
@copilot please fix
| } | ||
|
|
||
| [RequiresMSBuildVersionTheory("17.12.0", Skip = "https://github.com/dotnet/sdk/issues/46006")] | ||
| [MemberData(nameof(Net7Plus), MemberType = typeof(PublishTestUtils))] |
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 think we can just test the latest supported sdk (there should be a variable for this)
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.
Changed to use ToolsetInfo.CurrentTargetFramework in commit 900e4f6.
| testProject.AdditionalProperties["StripSymbols"] = "true"; | ||
| } | ||
| // Use an identifier with a space to create a path with a space | ||
| var testAsset = _testAssetsManager.CreateTestProject(testProject, identifier: "with space"); |
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.
Let’s test with a contrasting folder with a space
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.
Updated identifier to "path with spaces" in commit 900e4f6.
…path identifier - Changed from testing all Net7Plus frameworks to only CurrentTargetFramework - Removed unnecessary macOS/net7.0 platform check - Changed identifier from "with space" to "path with spaces" for better contrast Co-authored-by: agocke <515774+agocke@users.noreply.github.com>
agocke
left a comment
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.
@copilot please address
| testProject.AdditionalProperties["PublishAot"] = "true"; | ||
| // Linux symbol files are embedded and require additional steps to be stripped to a separate file | ||
| // assumes /bin (or /usr/bin) are in the PATH | ||
| if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) |
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 think this is set by default in .NET 10, we can probably remove it. It also shouldn't matter for the specific case we're testing.
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.
Removed StripSymbols setting in commit 1bcf1bd.
| } | ||
|
|
||
| [RequiresMSBuildVersionTheory("17.12.0", Skip = "https://github.com/dotnet/sdk/issues/46006")] | ||
| [InlineData(ToolsetInfo.CurrentTargetFramework)] |
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.
We can probably change this to Fact since it's not actually parameterized.
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.
Changed to Fact and removed InlineData parameter in commit 1bcf1bd.
| .And.HaveStdOutContaining("Hello World"); | ||
| } | ||
|
|
||
| [RequiresMSBuildVersionTheory("17.12.0", Skip = "https://github.com/dotnet/sdk/issues/46006")] |
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 don't think we require a specific MSBuild version anymore. This can probably be a regular Fact.
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.
Changed to [Fact] in commit 1bcf1bd.
| DoSymbolsExist(publishDirectory, testProject.Name).Should().BeTrue($"{publishDirectory} should contain {testProject.Name} symbol"); | ||
| IsNativeImage(publishedExe).Should().BeTrue(); | ||
|
|
||
| bool useRuntimePackLayout = targetFramework is not ("net7.0" or "net8.0" or "net9.0"); |
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 is unnecessary as well
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.
Removed useRuntimePackLayout logic and hardcoded to true in commit 1bcf1bd.
- Changed from RequiresMSBuildVersionTheory with InlineData to Fact - Removed StripSymbols setting (default in .NET 10) - Removed useRuntimePackLayout logic (hardcoded to true) - Set targetFramework directly to CurrentTargetFramework Co-authored-by: agocke <515774+agocke@users.noreply.github.com>
Plan: Add Native AOT test with space in path
Summary
Added a new test method
NativeAot_app_publishes_with_space_in_pathto theGivenThatWeWantToPublishAnAotApptest class. This test:"path with spaces") to ensure the project path includes spacesPublishAot=trueThe test uses
ToolsetInfo.CurrentTargetFrameworkand is a simpleFact(not parameterized).Changes from feedback
[MemberData(nameof(Net7Plus))]to[InlineData(ToolsetInfo.CurrentTargetFramework)]to test only the latest SDK"with space"to"path with spaces"for better contrastRequiresMSBuildVersionTheorywithInlineDatato simpleFactStripSymbolssetting (default in .NET 10)useRuntimePackLayoutconditional logictargetFrameworkdirectly toToolsetInfo.CurrentTargetFrameworkSecurity Summary
No security vulnerabilities were detected. The changes only add a new test case and do not modify any production code.
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.