Skip to content

Xunit v3#179

Draft
Youssef1313 wants to merge 39 commits into
microsoft:mainfrom
Youssef1313:xunit-v3
Draft

Xunit v3#179
Youssef1313 wants to merge 39 commits into
microsoft:mainfrom
Youssef1313:xunit-v3

Conversation

@Youssef1313

Copy link
Copy Markdown
Member

No description provided.

Comment thread NuGet.config
<packageSources>
<!--To inherit the global NuGet package sources remove the <clear/> line below -->
<clear />
<add key="nuget.org" value="https://api.nuget.org/v3/index.json"/>

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This must NOT be merged. I'll mirror the packages following https://github.com/dotnet/arcade/blob/main/Documentation/MirroringPackages.md once the PR is closer.

@Youssef1313 Youssef1313 closed this Mar 1, 2025
@Youssef1313 Youssef1313 reopened this Mar 1, 2025
@Youssef1313 Youssef1313 closed this Mar 1, 2025
@Youssef1313 Youssef1313 reopened this Mar 1, 2025
Comment on lines +423 to +426
// TODO: Reminder to self. This is where things go wrong currently.
// Here, TestContext.Current is correct, but once we go to RunTestCollection (in-process), we lose it.
// The first thought is to pass it through a MarshalByRefObject, and set it again inside RunTestCollection.
// However, xUnit doesn't seem to allow setting TestContext.Current.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@bradwilson Sorry for the direct ping here. I'm trying to add xUnit v3 support to this repo. This is for VS testing where the tests are run in devenv. We currently fail with this exception:

Cannot get KeyValueStorage on the idle test context

which happens because TestContext.Current wouldn't be preserved when jumping from the original process initiating the test to VS process. How to best approach this in xUnit 3?

@Youssef1313 Youssef1313 Mar 2, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Pushed a hacky fix 73b2077. There are likely more issues to investigate though:

    [FATAL ERROR] System.InvalidOperationException
      System.InvalidOperationException : Key '168a9711c591b86ee8c00e2c3b45adf0e346d8873775862cd4c18bdccda53f42' already exists in the message metadata cache.
      Old item: TestAssemblyStarting("168a9711c591b86ee8c00e2c3b45adf0e346d8873775862cd4c18bdccda53f42") name="Microsoft.VisualStudio.Extensibility.Testing.Xunit.v3.IntegrationTests" path="C:\Users\ygerges\Desktop\vs-extension-testing\bin\Microsoft.VisualStudio.Extensibility.Testing.Xunit.v3.IntegrationTests\Debug\net472\Microsoft.VisualStudio.Extensibility.Testing.Xunit.v3.IntegrationTests.exe" config=null seed=1193172899
      New item: TestAssemblyStarting("168a9711c591b86ee8c00e2c3b45adf0e346d8873775862cd4c18bdccda53f42") name="Microsoft.VisualStudio.Extensibility.Testing.Xunit.v3.IntegrationTests" path="C:\Users\ygerges\Desktop\vs-extension-testing\bin\Microsoft.VisualStudio.Extensibility.Testing.Xunit.v3.IntegrationTests\Debug\net472\Microsoft.VisualStudio.Extensibility.Testing.Xunit.v3.IntegrationTests.exe" config=null seed=1193172899

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@Youssef1313 There is an alternative: use ITestContextAccessor.Current, which could be what you wrap, and then have the tests use that instead of TestContext.Current. Is that a sufficient alternative?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@bradwilson Probably not. I think the main issue here is the less-level of support for cross-AppDomain scenarios. In this specific case for vs-extension-testing, that's even an out-of-process communication. The test executable starts, and then it launches devenv, and then we want to execute tests inside of devenv. So, when crossing from the test executable process to devenv, static state like TestContext.Current is lost. In 73b2077, what I'm trying to do is basically have a MarshalByRef wrapper around TestContext so that we query TestContext properties from the original process, and then re-construct things while in devenv process. It's less than ideal, but seems possible.

For the other InvalidOperationException I haven't yet investigated, so not sure yet.

}
}

var isXunit3 = compilation.ReferencedAssemblyNames.Any(assemblyName => assemblyName.Name == "xunit.v3.assert");

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This isn't a good choice typically as a rule. Looking for a reference to xunit.v3.core is more likely correct, though in your case perhaps you don't have people using third party assertion libraries so it doesn't matter as much.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks @bradwilson!

I'll address that and use xunit.v3.core as it is more correct.

@Youssef1313

Youssef1313 commented Jul 1, 2025

Copy link
Copy Markdown
Member Author

@bradwilson I'm trying to revisit this. IIRC, one of the major issues I was seeing is that many types of xunit dropped MarshalByRefObject, so it's becoming hard that we offload test execution to devenv process. To my knowledge, the drop of this is related to appdomain feature that was dropped. I assume the appdomain feature you had is kinda similar to MSTest, where test execution runs on its own app domain? I don't care about having such app domain feature in xunit.v3, but it may help us be able to move forward here if MarshalByRefObject is back (even if xunit itself doesn't need it). Would that make sense to you? This library is used by multiple repos and will be a blocker for moving such repos to xunit.v3.

@bradwilson

Copy link
Copy Markdown

Can you give me an example of some of the types you're talking about?

@Youssef1313

Copy link
Copy Markdown
Member Author

There was something around (at least) TestContext IIRC.

@bradwilson

Copy link
Copy Markdown

Suffice to say, we have no interest in supporting what you're trying to do. App Domains are in our past. I would look for alternative options to achieve what you want to achieve, and if that means you need to pepper MarshalByRefObject onto a bunch of things, you're probably going to end up needing to fork the main project and build your own packages.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants