xUnit tests may not be discovered now that the .NET SDK trims packages from deps.json when they don't have any assets

[dsplaisted]

The changes in #46218 can break xUnit tests, causing them to not be discovered at all.

This was found and documented here: dotnet/aspnetcore#62223

The issue is that xUnit looks for libraries nanmed "xunit" or "xunit.core" in the deps.json when trying to figure out whether an assembly is an xUnit test assembly. The xunit.core package doesn't include any runtime assets, only MSBuild .props and .targets files that are imported. This means that it may be removed from the deps.json file.

The xunit.core package depends on xunit.extensibility.core and xunit.extensibility.execution, both of which do have runtime assets. So if there are not references to those packages via another path, then the SDK will keep xunit.core in the deps.json. However, if there are other libraries that depend on xunit.extensibility.core and xunit.extensibility.execution, then the SDK will drop xunit.core from the deps.json and xUnit won't discover any tests.

[dsplaisted]

Some options for fixing:

• Revert Include only PackageReferences included at runtime in deps.json #46218
• Add the ability to opt in or out of the new behavior
• Hardcode "xunit.core" in DependencyContextBuilder to prevent it from being dropped
• Change the graph algorithm in DependencyContextBuilder so that we don't drop libraries with transitive dependencies on runtime assets, even if those runtime assets are referred to via a different path.

[dsplaisted]

@ericstj @jasonmalinowski @ViktorHofer @agocke FYI, The most recent deps.json changes (#46218) have the potential to prevent xUnit from discovering any tests. See above.

Feedback is welcome on how we should address this, I lean towards the last option.

[baronfel]

Another option for fixing would be to make changes to the xunit detection to be more flexible - I don't know that we have any support around the schema or structure of this file so I'm not sure we should encourage the use of it in external libraries.

[ViktorHofer]

Yes, the last option sounds best to me.

[Youssef1313]

FYI @bradwilson.

[Youssef1313]

Another option for fixing would be to make changes to the xunit detection to be more flexible - I don't know that we have any support around the schema or structure of this file so I'm not sure we should encourage the use of it in external libraries.

There is a risk with that if the user didn't notice their tests are not running and they didn't update the xunit dependency though. The failure here can be silent in many cases, as seen in aspnetcore repo.

[baronfel]

@Youssef1313 I tend to agree with you, just mentioning that option for completeness 👍

[bradwilson]

The code you linked is no longer current. Have you tried using a 3.x build of xunit.runner.visualstudio? Those use xunit.v3.runner.utility, which relies on Cecil to do the assembly probing rather than looking for the .deps.json file: https://github.com/xunit/xunit/blob/0024bf168314d7f5db0ee07da0c517f7bd377ca9/src/xunit.v3.runner.common/Utility/AssemblyUtility.cs#L41-L47

(And yes, the 3.x builds of xunit.runner.visualstudio are still able to run both v1 and v2 test projects, in addition to adding support for v3 test projects.)

[Youssef1313]

Updating aspnetcore to xunit.runner.visualstudio 3.x in dotnet/aspnetcore#62250. This issue is still relevant though for users of xunit.runner.visualstudio 2.x

[bradwilson]

This issue is still relevant though for users of xunit.runner.visualstudio 2.x

Is there any reason anybody should be stuck on this old version? If there are compatibility issues, we'd like to know about them. We currently have no plans to release any more 2.x builds of xunit.runner.visualstudio, so we would generally call this issue "won't fix", since there exists a fairly simple workaround (use a current build).

[bradwilson]

(I'm aware of the one potentially significant issues: net472 minimum. Unfortunately, the fact that netstandard2.0 can be broken on .NET Framework < 4.7.2 mandates this change for us.)

[dsplaisted]

Given that it seems like a workaround for the issue is to update to v3 of xunit.runner.visualstudio, I'm now leaning towards adressing this by adding a property to opt out of the new behavior where libraries without runtime assets are trimmed from the deps.json.

That means this issue could still be hit but it sounds like it may not have a huge impact, and if it did affect you, you could work around it by updating the xunit package or by setting a property to revert to the old behavior.

Feedback welcome.

[Youssef1313]

and if it did affect you, you could work around it by updating the xunit package or by setting a property to revert to the old behavior.

My problem here is "how" it is affecting me. It could be silently causing my tests to not run, and I could notice very late. A week of silent failures have already introduced few failing tests in aspnetcore repo. I wouldn't mind the breaking change here if it was obvious (i.e, the user is getting some error message and he will look for how to fix it)

[Evangelink]

IMO we should either:

• list the breaking change on .NET 10 announcements and ask @bradwilson if he can update his documentation to mention this issue. (that would be my pick)
• undo the breaking change

I am failing to see how the opt-out mechanism will help because I won't notice my tests are not run and when I notice, I can update to the new xunit runner that is not affected by the error.

Also note that this is affecting xunit but this could affect other libraries that are relying on the same behavior hence being explicit and calling out the "break" in .NET 10 SDK notes seems the best to me.