Skip to content

Conversation

@Youssef1313
Copy link
Member

No description provided.

<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
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
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?

Copy link
Member Author

@Youssef1313 Youssef1313 Mar 2, 2025

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

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
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");

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
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
Copy link
Member Author

Youssef1313 commented Jul 1, 2025

@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

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

@Youssef1313
Copy link
Member Author

There was something around (at least) TestContext IIRC.

@bradwilson
Copy link

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