Fix ConditionalFact not working with new xunit.runner.visualstudio version#15667
Merged
ViktorHofer merged 1 commit intomainfrom Mar 25, 2025
Merged
Fix ConditionalFact not working with new xunit.runner.visualstudio version#15667ViktorHofer merged 1 commit intomainfrom
ViktorHofer merged 1 commit intomainfrom
Conversation
akoeplinger
added a commit
to dotnet/sdk
that referenced
this pull request
Mar 25, 2025
ViktorHofer
approved these changes
Mar 25, 2025
This was referenced Mar 25, 2025
dotnet-maestro bot
added a commit
to dotnet/sdk
that referenced
this pull request
Mar 25, 2025
[main] Update dependencies from dotnet/arcade - Remove overriding cert for .msi arcade now provides the same default. - Merge branch 'main' into darc-main-c7b122ee-7487-4482-8d37-948de8d73a9f - Add emsdk patch - Add backport link - Add more patches - Update aspire patch - Add efcore patch - Port dotnet/arcade#15667 to the local TestUtilities copy
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
After #15651 I noticed that tests would fail that shouldn't even be executed because of a ConditionalFact that was false.
It turned out that with the new xunit.runner.visualstudio version these tests were always running regardless of the condition.
The reason is that we were setting the
_skipReasonafter calling the base Deserialize(), but that one sets the Timeout property which in turn calls Initialize which calls GetSkipReason() which then calls into our overridden GetSkipReason method in SkippedTestCase and at that point we don't have _skipReason yet so we return null which means the test case is not skipped.The fix is to set _skipReason before calling the base Deserialize. This is actually exactly what aspnetcore discovered ages ago in dotnet/aspnetcore@b1987c7 / xunit/visualstudio.xunit#266 (comment).
So why did we not notice this until now? It turns out the old version of xunit.runner.visualstudio does not go through the Serialize/Deserialize codepath when running on the command line. If I run with the old version in VS Test Explorer however I see exactly the same issue:
The new version seems to use the Serialize/Deserialize protocol also on the command line so that's why we hit it there.
Additionally, the tests in arcade didn't catch this because they were never testing that a ConditionalFact=false method is not executed, only the inverse. Added tests for that scenario too.
It works correctly now both inside and outside of VS with both the old and new version of xunit.runner.visualstudio:
