-
Notifications
You must be signed in to change notification settings - Fork 12
Xunit v3 #179
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?
Xunit v3 #179
Conversation
| <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"/> |
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 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.
| // 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. |
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.
@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?
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.
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
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.
@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?
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.
@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"); |
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 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.
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.
Thanks @bradwilson!
I'll address that and use xunit.v3.core as it is more correct.
|
@bradwilson I'm trying to revisit this. IIRC, one of the major issues I was seeing is that many types of xunit dropped |
|
Can you give me an example of some of the types you're talking about? |
|
There was something around (at least) TestContext IIRC. |
|
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. |
No description provided.