-
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
sln-add: Support for slnx #44570
base: main
Are you sure you want to change the base?
sln-add: Support for slnx #44570
Conversation
d397257
to
b6aa066
Compare
9f9f687
to
6e7f552
Compare
Refactor code Refactor code Add translations Fix typo Fix issues with --solution-folders Standarize error messages and tests Fix solution folder formatting Nit: Address pr comments
bf6d349
to
90f4248
Compare
@edvilme checkout the complete feedback. using solution.FindProject makes the regex usage irrelevant |
} | ||
|
||
if (_relativeRootSolutionFolders != null) | ||
foreach (var buildType in new List<string> { "Debug", "Release" }) |
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.
foreach (var buildType in new List<string> { "Debug", "Release" }) | |
foreach (var buildType in (string[])["Debug", "Release"]) |
24c9885
to
a4a9f74
Compare
that will hopefully fix errors like |
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.
Here, const strings for expected sln results were moved to a folder under TestAssets, with their .slnx counterparts generated by dotnet sln migrate
.
Additional test cases were created for sln.
SlnFile
was removed from these tests as well
the identifier doesn't help because testmanager's GetTestDestinationDirectoryPath shortens the directory name to 24 characters and we end up having the same errors. short-term solution is #45130 which goes beyond _2 in the ci. long-term solution is to make GetTestDestinationDirectoryPath stateless and use Path.GetTempFileName without any hashing or ci specific complexity; right now there are tests like |
Yes, it pushed before I was able to see your pr, bit of a sync issue there... I like the idea on your PR as it allows us to have a greater variety of different names for different cases (of which there are many on this pr) |
getting closer! 👍 these tests are failing: sdk/test/dotnet-new.Tests/PostActionTests.cs Line 782 in e524077
sdk/test/dotnet-new.Tests/PostActionTests.cs Line 818 in e524077
sdk/test/dotnet-new.Tests/PostActionTests.cs Line 854 in e524077
test is adding |
Hii, thanks and thanks for your input. Yes, those tests are currently failing on several PRs and should be unrelated to the changes here. When they're fixed, I will update the branch :) |
can you point me to it?
main:
with 5 retries back and forth, those three tests 100% of the times fail with your branch locally and pass with main |
You're right. There were some other tests from templating failing, so I got them mixed up. The issue here is when adding projects from a different directory, e.g., |
…o edvilme-slnx-add
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.
lgtm (one nit)
@@ -4,7 +4,6 @@ | |||
<!-- Using multiple feeds isn't supported by Maestro: https://github.com/dotnet/arcade/issues/14155. --> | |||
<NoWarn>$(NoWarn);NU1507</NoWarn> | |||
</PropertyGroup> | |||
|
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 file only has whitespace change git checkout upstream/main :/Directory.Packages.props
Contributes to #40913
This adds
dotnet sln add
support for .slnx files