Change from "sln" to "solution" as primary, and add "sln" as alias

[SourceCodeWhen]

This is in respect to issue #40817

Changes have been made to tests that relied on certain outputs using "sln" and hard-baked calls to "sln" from internal code/inlined test params.

I also pulled out the command name to a const string and the aliases to a readonly array to make it more in-line with other command implementations.

There's a surprising amount of things that relied somewhat on the "sln" keyword being returned for parts of testing, etc. So if preferred I can strip back the PR to leave "sln" as the primary and add "solution" as an alias, reverting the changes to the tests that needed changing.

Cheers!

[SourceCodeWhen]

@dotnet-policy-service agree

[kasperk81]

is it compatible with .sln vs. .slnx future plan #18739?

[SourceCodeWhen]

There's no changes in it to parsing of .sln/.slnx files, only for "dotnet sln {args}" to also allow "dotnet solution {args}".

Specifically, "dotnet solution {args}" being the default, and "dotnet sln {args}" now being an alias. So this shouldn't conflict with #18739 from what I understand. 😊

[kasperk81]

i can't wait to switch to slnx. keeping a close eye on #40817 progress 🔭

[marcpopMSFT]

since there's only one alias, probably could just make the Alias a string and call add to clean this up

[marcpopMSFT]

Triage: This change is actually a prereq we had in mind for doing the new slnx support. Were you going to publish this PR and add tests as well? We'll want to add a doc bug once this merges and we'll probably want this to target 9.0.2xx (not sure when slnx is coming but hopefully before 10).

[SourceCodeWhen]

Triage: This change is actually a prereq we had in mind for doing the new slnx support. Were you going to publish this PR and add tests as well? We'll want to add a doc bug once this merges and we'll probably want this to target 9.0.2xx (not sure when slnx is coming but hopefully before 10).

Hi there!

Yep! I plan on incorporating that change you mentioned and writing up some new tests this weekend so I can swap to Ready for Review. :)

Cheers!

[baronfel]

Wonderful news @SourceCodeWhen - let us know if you need any help/guidance getting this ready.

[nagilson]

This shouldn't change if the sln parameter on this function is not changing.

[nagilson]

Thank you for your contribution to .NET 😊 Your changes look great to me. I do have one nit pick comment. The tests are failing but it looks like only due to an issue on the repo and not your fault. Hopefully that issue will get cleaned up soon and we can merge!

[SourceCodeWhen]

Thank you for your contribution to .NET 😊 Your changes look great to me. I do have one nit pick comment. The tests are failing but it looks like only due to an issue on the repo and not your fault. Hopefully that issue will get cleaned up soon and we can merge!

Thanks!

I develop on Linux so ive been trying to work out what was happening for a while now, haha.

I thought it could have potentially been due to the test run effectively going twice for the same folder destination, one with "sln" and one with "solution", and the call on windows throwing an error compared to other platforms.

If it's a bug on the repo however then that makes life easier, haha.

Cheers!

[nagilson]

@SourceCodeWhen This PR is almost there now! :)

There are 3 remaining test failures mostly related to dotnet solution add.
The failure is due to a missing argument. When you call CopyTestAsset, you should provide a 2nd parameter when migrating from a fact to a theory. That parameter should be the parameter to the test.

Example

        [Theory]
        [InlineData(true)]
        [InlineData(false)]
        public void It_publishes_on_release_if_PublishRelease_property_set(bool optedOut)
        {
            var helloWorldAsset = _testAssetsManager
               .CopyTestAsset("HelloWorld", $"{optedOut}")
               .WithSource();

optedOut is passed as a 2nd parameter to copy test asset to prevent test folders from having a clobbering race condition

[SourceCodeWhen]

@SourceCodeWhen This PR is almost there now! :)

There are 3 remaining test failures mostly related to dotnet solution add. The failure is due to a missing argument. When you call CopyTestAsset, you should provide a 2nd parameter when migrating from a fact to a theory. That parameter should be the parameter to the test.

Example

        [Theory]
        [InlineData(true)]
        [InlineData(false)]
        public void It_publishes_on_release_if_PublishRelease_property_set(bool optedOut)
        {
            var helloWorldAsset = _testAssetsManager
               .CopyTestAsset("HelloWorld", $"{optedOut}")
               .WithSource();

optedOut is passed as a 2nd parameter to copy test asset to prevent test folders from having a clobbering race condition

Aha! That makes a ton of sense! :)

I've gone ahead and added the solution command to the identifier parameter of CopyTestAsset method wherever I've seen it crop up. In the case that something was already there before, I've concatenated the command on so as to be unique. Will see how the tests go on the pipeline now!

Thanks a lot for the point in the right direction. :)

[nagilson]

Thanks, it looks good now but there's a new repo issue 😅 Waiting for it to be fixed.
I really appreciate your fast upkeep on this and great work on the fix! 😊

[nagilson]

/backport to release/9.0.2xx

[github-actions]

Started backporting to release/9.0.2xx: https://github.com/dotnet/sdk/actions/runs/11354060685

[nagilson]

So that last repo issue was fixed but theres a new one in the same spot so I still dont think it's going to be mergeable but we'll pay attention!

[SourceCodeWhen]

So that last repo issue was fixed but theres a new one in the same spot so I still dont think it's going to be mergeable but we'll pay attention!

All good, haha. Looks like it's just about done on this one! 🤞