Skip to content

Conversation

@333fred
Copy link
Member

@333fred 333fred commented Jul 11, 2025

Address changes from dotnet/sdk#49248.

@333fred 333fred requested a review from a team as a code owner July 11, 2025 20:44
Comment on lines 156 to +158
This version needs to match the Test SDK version consumed by Arcade.
-->
<MicrosoftNETTestSdkVersion>17.5.0</MicrosoftNETTestSdkVersion>
<MicrosoftNETTestSdkVersion>17.13.0</MicrosoftNETTestSdkVersion>
Copy link
Member

@dibarbet dibarbet Jul 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NikolaMilosavljevic - we need to take this test platform update because otherwise our tests will stop running in CI (see dotnet/sdk#49248)

I don't understand what the comment above means - is this going to break anything in the VMR (based on the git blame it looks like you introduced it)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will stop

Correction: have stopped

Copy link
Member

@Youssef1313 Youssef1313 Jul 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to take this test platform update because otherwise our tests will stop running in CI

I'm surprised that this update is required for running tests. I think updating xunit.runner.visualstudio should just be sufficient. At least the issue when I investigated for aspnet core was xunit.runner.visualstudio 2.x relying on specific behavior in deps.json that .NET SDK broke, but then .NET SDK special cased xunit back again. https://github.com/dotnet/sdk/blob/460bff1c9e39948a9620f2216f428d60751395ec/src/Tasks/Microsoft.NET.Build.Tasks/DependencyContextBuilder.cs#L374-L375

If you started consuming newer SDK with the hack above, it should also just work.

It's still a good idea to update both packages for sure. But I mean, you could keep Microsoft.NET.Test.Sdk separately.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, we cannot. xunit.runner.visualstudio depends on Microsoft.TestPlatform.ObjectModel (>= 17.13.0) on framework.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I didn't realize xunit's dependency.

@dibarbet dibarbet requested a review from a team as a code owner July 11, 2025 23:04
@333fred 333fred requested a review from a team as a code owner July 12, 2025 21:49
dibarbet and others added 4 commits July 14, 2025 13:22
Will follow up with an issue but disabling for now to see if this unblocks the CoreCLR tests
This `Assert.False` call was executing directly an a thread pool thread.
That meant when it triggered it was an unhandled exception on a TPT
which crashes the process. The calling code already fails the test when
this happen hence changed this to just output the failure info to the
test logs.
@jaredpar jaredpar requested a review from a team as a code owner July 15, 2025 16:39
@dibarbet
Copy link
Member

/azp run roslyn-CI

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

throw new InvalidOperationException();

var xunitUtilities = AppDomain.CurrentDomain.GetAssemblies().Where(static assembly => assembly.GetName().Name.StartsWith("xunit.runner.utility")).ToArray();
var xunitUtilities = AppDomain.CurrentDomain.GetAssemblies().Where(static assembly => assembly.GetName().Name.StartsWith("xunit.runner.visualstudio")).ToArray();
Copy link
Member

@jjonescz jjonescz Jul 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we fail if this assembly doesn't exist to simplify catching this type of break in the future?

@dibarbet
Copy link
Member

Merging, this fixes regular CI. Integration CI is still under investigation, but we may re-open the branches first.

@dibarbet dibarbet merged commit 7461ad2 into dotnet:main Jul 16, 2025
24 of 28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants