[ric-oliv]

Runs Stryker.NET on every PR that changes C# under src/, but only mutates the lines the PR changed. A changed line no test runs (NoCoverage) fails the check. A mutant that survives the tests only warns, because some mutants change nothing a test could catch. The mutants show up as warnings on their lines in the diff, with the code they replaced, and in the job summary. On PRs from this repo they also go in a comment, which is only posted once a mutant isn't caught or Stryker didn't finish, and is kept up to date after that. Mutations of log calls and ConfigureAwait are skipped, since they change nothing a test should check.

Closes #5343

#skip-changelog

Why changed lines instead of --since

--since mutates whole files, so a PR touching SentryOptions.cs inherits the file's old survivors. On #5853, mutating the whole files gave 92 survivors, and 86 of them were in code the PR didn't touch. The script turns the diff hunks into Stryker's mutate spans instead, and Stryker keeps only mutants that sit fully inside them.

Which tests run

Other test projects cover a project too, like Sentry.AspNetCore.Tests for FormRequestPayloadExtractor in src/Sentry. So for each changed project, the script runs every test project that depends on it and can run on net10.0 on the Linux runner: 21 for src/Sentry, including the MAUI tests, and one or two for a leaf project like Sentry.Serilog. The MVVM, Android assembly reader and ASP.NET (classic) tests can't, so a project only they cover is skipped, and the summary says so. Device tests need an emulator or a simulator, so they don't run either.

Stryker also leaves out some tests, which still run in the normal CI jobs. The API approval tests would rewrite their snapshots. StringStackTraceFactoryTests.MethodGeneric, SentryStackTraceFactoryTests.MethodGeneric, and IntegrationTests.Simple in the Serilog and NLog tests snapshot stack frames, which Stryker's rewrite changes, so they would fail on every run whatever the mutation. A mutant only these tests reach shows up as NoCoverage and fails the check, even though a test does cover the line.

Impact on PR CI

It's a separate workflow that runs alongside the other checks. A change in src/Sentry takes 7 to 11 minutes and a change in a leaf project 3 to 4, both well before the build finishes, so it doesn't add to the time a PR waits. Stryker failing or timing out never fails the check, so it can't make CI less stable, and it isn't a required check. In one of five runs, Stryker's coverage phase stalled. The 25-minute step timeout ends it, and the summary and the comment say Stryker didn't finish. It installs only the SDK that global.json pins and the maui-tizen workload the MAUI tests need, not the shared setup's older SDKs and workloads.

The comment comes from a second job with a write token, the same split verify-api.yml uses, so the job that runs the PR's code keeps a read-only token. Fork PRs can't get a write token, so they only get the annotations.

The change to Directory.Build.props

Before mutating, Stryker analyzes each project with a build that doesn't build project references. UnoptimizedAssemblyDetector, which Directory.Build.props adds to every project, opens every referenced assembly and fails that build, so only Sentry itself could be mutated. The script sets SkipUnoptimizedAssemblyDetector only while it restores and runs Stryker, which leaves the detector out of that restore. Every other build keeps it.

Run it locally

dotnet tool install --global dotnet-stryker, then pwsh scripts/run-mutation-tests.ps1 -Base origin/main.

Not covered yet

It doesn't cover PRs that only change tests yet, or code that's only compiled for frameworks other than net10.0. Lines a PR only deletes can't be mutated, and neither can mutations that reach past the changed lines, like removing the whole block around one changed line. If Stryker can't run for one project, the summary says so and the other projects still get their report.

When one mutant breaks the build, Stryker drops every mutant in that method. The summary says how many mutants on the changed lines that affected, so it never reports a clean result for code it couldn't test.

How it was tested

Temporary commits on this branch, since squashed away, gave the job real changes to report on:

• All mutants caught: an equivalent rewrite in RateLimit.cs. 4 of 4 caught, so no comment was posted.
• A mutant no test catches: an equivalent rewrite of a boundary check in RetryAfterHandler.cs. It got a warning on the line and a new comment.
• The gap gone: the same comment was updated to 4 of 4, with no second comment.
• Nothing to mutate: the comment was updated to say so.
• A line no test runs: an unused method and a changed log message in Sentry.Serilog. The check failed, the comment showed value - 1 as NoCoverage, and the log call wasn't mutated.
• Projects other than Sentry: equivalent rewrites in Sentry.Extensions.Logging and Sentry.Maui, mutated once the detector was out of the way. On Linux, MAUI needs the maui-tizen workload, which the job installs in a few seconds, and only when a MAUI project changed.
• Tests in other projects: an equivalent reordering in FormRequestPayloadExtractor, whose tests are in Sentry.AspNetCore.Tests. With only Sentry.Tests, all 4 mutants showed as NoCoverage. With every dependent test project, all 4 were caught, in three runs of about 10 minutes each, and in 7 minutes once the MAUI tests were added.

Locally, a one-line change inside GlobalSessionManager.PauseSession, where Stryker can't build any mutant, reports that 2 mutants on the changed lines couldn't be built, instead of an empty 0 of 0.

The shared setup took 86 s, and its libssl1 step once failed with an apt error. Installing only the SDK that global.json pins takes 9 s.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.86%. Comparing base (7ccb1a4) to head (6ed569d).
⚠️ Report is 14 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5881      +/-   ##
==========================================
+ Coverage   76.56%   76.86%   +0.30%     
==========================================
  Files         528      533       +5     
  Lines       19624    19847     +223     
  Branches     3873     3920      +47     
==========================================
+ Hits        15025    15256     +231     
+ Misses       3713     3710       -3     
+ Partials      886      881       -5     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[github-actions]

Mutation testing breaks the lines a PR changes on purpose, one mutation at a time, and checks that a test fails each time. A changed line no test runs fails the check, and a mutant that survives the tests only warns. Full report

No lines under src/ were added or changed, so there's nothing to mutate.

[jamescrosswell]

At the moment this runs on every PR and it creates a report. I'm wondering how/when we would react to that report though... as it stands, it would be a manual process, easy to skip.

I'm thinking it might be more useful to run it periodically (e.g. nightly).

How it would work

• A scheduled job mutates only the lines merged since the last successful nightly, using the script's existing -Base. Each line gets tested once, so a closed issue stays closed unless someone changes that line again.
• If mutants survive, it opens one issue per merged PR. We either close the issue with a note saying why it's not a problem, or add a test that catches the mutation. watch-upstream.yml already works this way.

Why I prefer it

• PR CI and reviews aren't affected, which is what we asked for on the issue.
• Every survivor gets a decision on record, and the share we close as non-issues tells us how noisy this is.
• It only runs code from main, so we don't need the separate comment job or the fork limitation.
• Nobody is waiting on the run, so we can include more test projects. Today "not caught" only means not caught by test/<Name>.Tests, so a change to FormRequestPayloadExtractor, whose tests are in Sentry.AspNetCore.Tests, would probably warn falsely.

The trade-off

Gaps only show up after merge, so this wouldn't have helped during review on #5337. A missing test isn't a production bug, so I think that's acceptable, and we could add a PR run triggered by a label later if we miss it.

What it would need

• ignore-methods, probably ConfigureAwait and Log* (not tried yet), because each false alarm would now cost an issue.
• The replacement code in the report. "Equality mutation" isn't enough to act on without downloading the HTML report.
• A red run when Stryker can't run for a project. The script's catch lets the run succeed today, so the next night would skip those lines for good.
• A manual trigger that takes a base commit, because a test-only fix doesn't change src/ and wouldn't get re-checked otherwise.

Volume

43 PRs changed src/*.cs in the last 28 days, so that's at most about 11 issues a week. The 6 survivors in 456 added lines on #5853 suggest about 17 survivors a week, but that's only one sample.

What do you think?

[jamescrosswell]

Ideally, we'd probably automate creating the PR to resolve the issue as well - that means the first time you or I see it is when we have a potential problem and a suggested solution (rather than just more problems).

[ric-oliv]

Thanks for the detailed write-up! I'd suggest keeping this one at PR time, though.
The cheapest moment to add a missing test is in the PR that wrote the code, while the author still has the context and a reviewer is looking at the same lines. After merge it becomes an issue for someone else to pick up. The comment is about as actionable as Codecov's patch coverage, and to make it harder to skip, it now fails the PR instead of moving after the merge.

You were right about the other test projects. I've updated so the job now runs every test project that depends on the changed project and can run on the Linux runner.

From your list, the report now shows the code each mutant replaced, ignore-methods skips log calls and ConfigureAwait, and the check fails on NoCoverage, a changed line no test runs at all. A surviving mutant only warns, since some can't be caught by any test. Stryker failing or timing out never fails the check either. That matters because one run in five stalled in Stryker's coverage phase. When it does, the step times out after 25 minutes and the job summary says Stryker failed, so we can keep an eye on how often it happens and dig into why if it keeps coming back.

Some tests don't run under Stryker, on purpose:

• Sentry.AspNet.Tests targets only .NET Framework, Sentry.Maui.CommunityToolkit.Mvvm.Tests only net9.0, and Sentry.Android.AssemblyReader.Tests builds an Android app, so none of them run on net10.0 on the Linux runner. A project only they cover gets skipped, and the summary says so. The device tests need an emulator or a simulator, so they're out too.
• The API approval tests would rewrite their snapshots, as they would locally.
• StringStackTraceFactoryTests.MethodGeneric, SentryStackTraceFactoryTests.MethodGeneric and the Serilog and NLog IntegrationTests.Simple snapshot stack frames, which Stryker's rewrite changes, so they fail on every run. A line only they reach shows as NoCoverage and fails the check. That's the one false failure I know of.

All of these still run in the normal CI jobs.

For the code that's already there, I'd suggest to have Jr. run the whole matrix daily (or weekly) and open PRs with the missing tests instead of a nightly job 🤔

[ric-oliv]

Happy to close the PR and bring up this topic to a separate discussion, if you prefer! :)

[jamescrosswell]

I'd suggest keeping this one at PR time, though.
The cheapest moment to add a missing test is in the PR that wrote the code, while the author still has the context and a reviewer is looking at the same lines.

If you feel really strongly about it, we can leave it in the PR workflow. Just means there's nothing forcing us to address any of it's comments... we currently have that for the codecov reports and I think they largely get ignored.

Leaving it a 'report only' is the only way to test it risk free if we leave it in the PR workflow though.

[ric-oliv]

Thanks! I'd keep it on PRs, yeah. It can't block anything either way, since no check is required in this repo. What it adds over Codecov is the red check when no test runs a changed line. Codecov is informational with patch: off here, so it never goes red. If those red checks turn out to be noisy, I'll make that step warn-only.

[cursor]

Stale Bugbot comment from a previous run.

[cursor]

Cursor Bugbot has reviewed your changes and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit 9d11066. Configure here.}

[jamescrosswell]

Approving, since none of this needs to hold it up, and thanks for working through the list. One non-blocking suggestion, (fine as a follow-up if you prefer):

• Say when there's no report. When Stryker stalls, the job ends green and the comment job exits because there's no summary, so the PR shows a green tick with either no comment or the one from the previous push, as in https://github.com/getsentry/sentry-dotnet/actions/runs/42215998963. We could have the comment say that Stryker didn't finish and the PR wasn't checked... to clearly distinguish that scenario from one where Stryker ran and found no survivors.

[ric-oliv]

Good catch, thanks! You're right, a timed-out step leaves no summary, so the comment job just exits. Same when a later project stalls: the comment lists the ones that finished and says nothing about the rest. With 6ed569d, when the Stryker step doesn't finish, the summary and the comment say "Stryker didn't finish, so not every changed line was checked", and that starts a comment if there isn't one. Skipped projects get their own line now too, which Bugbot flagged and which I'd said the summary already did.

I'm also fine with merging this after the v7 stable release.