Skip to content
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

Stryker.NET doesn't handle xUnit v3 properly #3117

Open
0xced opened this issue Nov 21, 2024 · 14 comments
Open

Stryker.NET doesn't handle xUnit v3 properly #3117

0xced opened this issue Nov 21, 2024 · 14 comments
Assignees
Labels
🐛 Bug Something isn't working

Comments

@0xced
Copy link
Contributor

0xced commented Nov 21, 2024

Describe the bug

When using xUnit v3, Stryker.NET produces warnings and can't perform proper mutations.

Reproduction steps

Checkout the xunit-v3 branch of the Serilog.Formatting.Log4Net project and run dotnet tool restore && dotnet stryker at the root of the repository.

Expected behavior

Stryker.NET performs mutations and gives the same score (100%) as when using xUnit v2.

Dashboard with xUnit v2 (on the main branch): 100%

Actual behaviour

Stryker.NET produces many warnings and eventually fails to perform the mutations.

[22:03:18 WRN] Runner 2: Initial test run encounter an unexpected test case (Serilog.Formatting.Log4Net.Tests.IndentationSettingsTest.IndentationSettingsToString), mutation tests may be inaccurate. Disable coverage analysis if you have doubts.
[22:03:18 WRN] Runner 2: Initial test run encounter an unexpected test case (Serilog.Formatting.Log4Net.Tests.Log4NetTextFormatterTest.ThreadIdProperty), mutation tests may be inaccurate. Disable coverage analysis if you have doubts.
[22:03:18 WRN] Runner 2: Initial test run encounter an unexpected test case (Serilog.Formatting.Log4Net.Tests.Log4NetTextFormatterTest.MessageCDataMode), mutation tests may be inaccurate. Disable coverage analysis if you have doubts.
[22:03:18 WRN] Runner 2: Initial test run encounter an unexpected test case (Serilog.Formatting.Log4Net.Tests.Log4NetTextFormatterTest.IndentationSettings), mutation tests may be inaccurate. Disable coverage analysis if you have doubts.
[22:03:18 WRN] Runner 2: Initial test run encounter an unexpected test case (Serilog.Formatting.Log4Net.Tests.Log4NetTextFormatterTest.DomainAndUserNameProperty), mutation tests may be inaccurate. Disable coverage analysis if you have doubts.
[22:03:18 WRN] Runner 2: Initial test run encounter an unexpected test case (Serilog.Formatting.Log4Net.Tests.Log4NetTextFormatterTest.LogEventLevel), mutation tests may be inaccurate. Disable coverage analysis if you have doubts.
[22:03:18 WRN] Runner 2: Initial test run encounter an unexpected test case (Serilog.Formatting.Log4Net.Tests.Log4NetTextFormatterTest.MachineNameProperty), mutation tests may be inaccurate. Disable coverage analysis if you have doubts.
[22:03:18 WRN] Runner 2: Initial test run encounter an unexpected test case (Serilog.Formatting.Log4Net.Tests.Log4NetTextFormatterTest.XmlElementsLineEnding), mutation tests may be inaccurate. Disable coverage analysis if you have doubts.
[22:03:18 WRN] Runner 2: Initial test run encounter an unexpected test case (Serilog.Formatting.Log4Net.Tests.Log4NetTextFormatterTest.Log4JCompatibility), mutation tests may be inaccurate. Disable coverage analysis if you have doubts.
[22:03:18 WRN] Runner 2: Initial test run encounter an unexpected test case (Serilog.Formatting.Log4Net.Tests.PublicApi.ApprovePublicApi), mutation tests may be inaccurate. Disable coverage analysis if you have doubts.

Dashboard with xUnit v3: 3.09%

Desktop

  • OS: both macOS and Linux (and probably Windows too)
  • Type of project: .NET 6 + 8 + .NET Standard 2.0
  • Framework Version .NET 8
  • Stryker Version: 4.4.1

Additional context

I have not enabled Microsoft Testing Platform support in xUnit.net v3 so I don't think this is related to #3094 although I'm not totally sure.

@0xced 0xced added the 🐛 Bug Something isn't working label Nov 21, 2024
@rouke-broersma
Copy link
Member

We have quite some custom logic for dealing with specific test framework quirks, I suspect xunit v3 has added yet another layer of quirks.

@dupdob
Copy link
Member

dupdob commented Nov 22, 2024

I predict a big one: it looks like a lot of VsTest design rules are violated here.

@dupdob dupdob self-assigned this Nov 22, 2024
@dupdob
Copy link
Member

dupdob commented Nov 22, 2024

First Analysis

Stryker is not able to run xUnit 3 theories with inline data, that is Stryker detects them correctly but they are skipped by the runner
TpTrace Warning: 0 : 18292, 12, 2024/11/22, 17:39:20.512, 224768786899, testhost.dll, [xUnit.net 00:00:00.22] Serilog.Formatting.Log4Net.Tests: Skipping test case 'Serilog.Formatting.Log4Net.Tests.IndentationSettingsTest.IndentationSettingsToString' (ID '6066754da9eb486a3555a1ff4792822d9ebe45d5ccf3d02486e8bdb36c7bed24') without serialization
it looks like Stryker should provide serialized data and somehow fails to do so.
In theory Stryker keeps all data for every test and reuses them afterward, but there is probably a bug somewhere.

The good news is that it does not look like a big issue.

@dupdob
Copy link
Member

dupdob commented Nov 23, 2024

Update

  • it looks like there are differences in how theories are captured between xUnit V2 and V3. Main (single?) impact is that theories data set are discovered at run time only
  • there was a minor bug in Stryker regarding test cases discovered during the initial test run. Not sure if it has any actual impact. Fixing it did not solve the issue
  • it looks coverage capture does not work at. The usual cause for this is failing to copy the mutated assembly in the right place
  • mutated assembly is properly copied at the right place

Conclusion at this stage

It is likely that the xUnit V3 test runner executes the tests outside the VsTest host. But, Stryker requires its VsTest extension to run inside the test process.
Without this, Stryker simply cannot run effectively: it cannot get test coverage, nor control which mutant to activate during a test session.

@dupdob
Copy link
Member

dupdob commented Nov 24, 2024

Consequences

  1. Stryker does not currently support xUnitV3 test adapter as it deviates too much from the standard behavior
  2. The main consequence is that Stryker cannot work AT ALL with xUnitV3 test base. Any attempt will result in most mutants being flag as not covered and the remaining flagged as survived.
  3. There is no clear path forward to fix this
  4. In the short term, Stryker can be modified to support a simpler approach to tests. That is one without coverage analysis and using environment variables for control. Note that it will result in very poor performances.
  5. For medium term, some basic form of coverage could be restored (is a mutation covered yes or no), that would only slightly improve performances
  6. Alternatively, we could revert to using IPC mechanisms to exchange data between coverage collector and the testing logic. But bear in mind this logic was dropped in the pas as it was complex and a recurrent source of issues.

I do not think trying to fit support for xUnitV3 via VsTest is worth the effort, I suppose the move to mstest will provide a smoother solution. But that remains to be confirmed.

The alternative is to implement a dedicated runner for xUnit V3, which would provide other interesting benefits but looks like a lot of work.

@dupdob
Copy link
Member

dupdob commented Nov 24, 2024

For context:

xunit/xunit#2337

After reading, I think the best approach would be to have a xUnit dedicated test runner...

@rouke-broersma
Copy link
Member

Why have standards.. 🫠

@dupdob
Copy link
Member

dupdob commented Nov 25, 2024

I just opened a PR that re enables basic support for xUnit V3, via disabling coverage based optimization when capture appears to have failed and the use of environment variables for mutation control.

Of course, it will result in a slow execution.
I also thought of a design to work around this issue: using pipe or file based result for coverage capture, perform per test coverage capture and disable mixing mutations when testing. This requires significant work, and the per test coverage capture when be too time costly for this approach to be beneficial.

Sill, it is a possible way forward.
The alternatives are: support for MsTest (assuming this solves this problem, which remains to be confirmed), using some form of xUnitV3 dedicated plugin (if it can solve the issue) or use a dedicated test runner, which would bring several benefits but requires dedicated effort

@dupdob
Copy link
Member

dupdob commented Nov 25, 2024

PR #3122 restore basic support for xUnit V3. Basic as no coverage based optimization possible.

@0xced
Copy link
Contributor Author

0xced commented Nov 26, 2024

Let's ping @bradwilson on this issue. He's been very helpful when I reported issues on the early xUnit v3 prereleases. Maybe he can give some guidance.

@bradwilson
Copy link

it looks like there are differences in how theories are captured between xUnit V2 and V3. Main (single?) impact is that theories data set are discovered at run time only

This is controlled by configuration and/or RunSettings. The default for whether we pre-enumerate theories depends on the runner in question: we generally disable it with our console runner, and enable it with xunit.runner.visualstudio when we believe we're inside Visual Studio (vs. running from dotnet test).

If you are invoking a v3 test assembly in automated mode, then -preEnumerateTheories will turn it on (or --pre-enumerate-theories on in Microsoft Testing Platform CLI mode).

It is likely that the xUnit V3 test runner executes the tests outside the VsTest host.

That is correct. v3 test projects are executables. When running v3 tests with xunit.runner.visualstudio, it launches those tests into a separate process and does the conversion from the xunit.v3.runner.utility object model into the VSTest object model.

Let's ping @bradwilson on this issue.

Ask away. 😄

@dupdob
Copy link
Member

dupdob commented Nov 27, 2024

Thanks @bradwilson.

Can we toggle the 'in-proc' run flag via some visualstudio runner's custom settings? Stryker currently relies on an inproc VsTest data collector to track mutation coverage and with xunit v3 running out of process, it reports no coverage; this new architecture makes. 'in proc' data collector useless in general.

Regarding the enumeration of theories during tests discovery, this is a secondary issue. But I tried modified the settings to no avail: theories in the sample projects are not pre enumerated; these are inline data theories, if this makes any difference . For context, Stryker always enables design mode, and we had no problem with the latest xUnit V2.xx.

@bradwilson
Copy link

Can we toggle the xunit/xunit#2947 via some visualstudio runner's custom settings?

No, that ability is only provided to runner authors. You'd need a replacement for xunit.runner.visualstudio that could run things in-process, and you'd be entirely responsible for the dependency resolution. In practice, this isn't something that's easy to do; I personally have tried and failed, so I wouldn't be able to help you do it. Part of the reason that this is a challenging task is that it also involves any unmanaged dependency resolution, assuming you have customers who are using unmanaged dependencies (like C++ libraries).

this new architecture makes. 'in proc' data collector useless in general.

This isn't just the architecture of xUnit.net v3; it's also the architecture of Microsoft.Testing.Platform. That means MSTest 3.6+ tests which enable MTP will be in the same situation that you find yourself with xUnit.net v3 today.

For context, Stryker always enables design mode, and we had no problem with the latest xUnit V2.xx.

There may be a bug here, then. When design mode is enabled, pre-enumeration should be on (unless explicitly turned off via config).

@rouke-broersma
Copy link
Member

rouke-broersma commented Nov 27, 2024

@bradwilson we use the vstest translation layer /test platform v2 so currently our design is not compatible with mstest testplatform anyway. I think this requires xunit.runner.visualstudio as well, so in that case shouldn't the data collector be running on proc already like we require?

Nevermind I mixed up the two different settings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 Bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants