Introduce mutation testing #2763
Conversation
|
Mh, given that we have a number of known flaky tests, wouldn't this trip up
However, I wonder if spurious test failures wouldn't just produce a bunch of false-negatives, i.e., everything it would yield as |
|
Github doesn't like your CI:
Probably, but we should also just fix those. I'm not sure having flaky tests should be a blocker to landing better testing. |
|
How do we display the results here? Is there a way to get codecov integration or just leave a comment on the pr? Or is it too slow to run in CI and just do it on a daily cron job (with issues opened on regression? Not sure how to surface information). |
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2763 +/- ##
==========================================
- Coverage 89.80% 89.79% -0.01%
==========================================
Files 121 121
Lines 99314 99314
Branches 99314 99314
==========================================
- Hits 89187 89179 -8
- Misses 7522 7524 +2
- Partials 2605 2611 +6 ☔ View full report in Codecov by Sentry. |
I'll look into this and propose something here after I get some idea of run time. |
dunxen
force-pushed
the
2023-11-mutation-testing
branch
from
c7ee4fd to
84887de
Compare
WalkthroughThe recent update introduces a mutation testing job in the CI pipeline, enhancing the code quality by identifying potential weak spots. Additionally, it adjusts the behavior in a core component of the project by disabling a panic trigger under specific conditions, potentially modifying how the system handles certain channel operations. Changes
Tip Early access features: enabledWe are currently testing the following features in early access:
Note:
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
.github/workflows/build.yml
Outdated
| mutation: | ||
| runs-on: ubuntu-latest | ||
| if: github.event_name == 'pull_request' | ||
| steps: | ||
| - name: Checkout source code | ||
| uses: actions/checkout@v3 | ||
| with: | ||
| fetch-depth: 0 | ||
| - name: Relative diff | ||
| run: | | ||
| git branch -av | ||
| git diff origin/${{ github.base_ref }}.. | tee git.diff | ||
| - run: cargo install cargo-mutants | ||
| - name: Mutants | ||
| run: | | ||
| cargo mutants --no-shuffle -j 2 -vV --in-diff git.diff | ||
| - name: Archive mutants.out | ||
| uses: actions/upload-artifact@v3 | ||
| if: always() | ||
| with: | ||
| name: mutants-incremental.out | ||
| path: mutants.out |
There was a problem hiding this comment.
The mutation testing job configuration looks correct but consider the following improvements:
- Line 232: Ensure
cargo-mutantsis installed from a specific version to avoid potential issues with future breaking changes. Specify the version usingcargo install cargo-mutants --version x.y.z. - Line 235: The use of
--no-shufflemight not be ideal for all scenarios. If the intention is to reduce test flakiness, document this decision. Otherwise, consider removing it to allowcargo-mutantsto optimize the mutation testing process. - Line 238: The conditional
if: always()ensures that artifacts are uploaded regardless of the job's success or failure. Confirm this aligns with the intended behavior, as it might clutter the artifacts with failed mutation test outputs.
lightning/src/ln/channel.rs
Outdated
| @@ -6824,6 +6824,7 @@ impl<SP: Deref> OutboundV1Channel<SP> where SP::Target: SignerProvider { | |||
| pub fn get_funding_created<L: Deref>(&mut self, funding_transaction: Transaction, funding_txo: OutPoint, is_batch_funding: bool, logger: &L) | |||
| -> Result<Option<msgs::FundingCreated>, (Self, ChannelError)> where L::Target: Logger { | |||
| if !self.context.is_outbound() { | |||
| // REMOVE ME - TRIGGER MUTANTS | |||
There was a problem hiding this comment.
Disabling a panic statement for mutation testing could mask potential logic errors in runtime behavior. Consider alternatives that allow mutation testing without compromising safety checks.
There was a problem hiding this comment.
You might be confused, Mr. Bunny.
There was a problem hiding this comment.
@dunxen, I appreciate your perspective! My initial concern was aimed at ensuring that mutation testing, while valuable for uncovering untested paths, doesn't inadvertently bypass critical runtime safety checks. It's crucial to strike a balance where mutation testing enhances code quality without compromising the integrity of safety mechanisms. If there's a specific approach or context within cargo-mutants that addresses this balance, I'd be keen to understand more. Your insights are invaluable in this discussion.
|
Looks like 2 viable things caught in Took around 6 minutes from a cold start for this PR checking |
|
Oh, |
.github/workflows/build.yml
Outdated
|
|
||
| mutation: | ||
| runs-on: ubuntu-latest | ||
| if: github.event_name == 'pull_request' |
There was a problem hiding this comment.
I think it does not need to run it for every PR. PRs that do not change any relevant code could be skipped (e.g. upgrading dependencies, test changes, etc?). Not sure how to deal with it in this project.
There was a problem hiding this comment.
Since it's based on diff, it shouldn't spend much time doing anything in those cases, but I'll have a look if that's not what happens.
There was a problem hiding this comment.
I do believe it would spend a lot of time in PRs like #3063, for example. It's what is expected?
| - run: cargo install cargo-mutants | ||
| - name: Mutants | ||
| run: | | ||
| cargo mutants --no-shuffle -j 2 -vV --in-diff git.diff |
There was a problem hiding this comment.
Not sure how cargo-mutants works in depth but will the CI task fail if a % of mutants has not been killed?
dunxen
force-pushed
the
2023-11-mutation-testing
branch
2 times, most recently
from
0d4c36b to
4f58150
Compare
.github/workflows/build.yml
Outdated
| - uses: taiki-e/install-action@v2 | ||
| name: Install cargo-mutants using install-action | ||
| with: | ||
| tool: cargo-mutants |
There was a problem hiding this comment.
Isn't this the same as just running a shell of cargo install cargo-mutants?
There was a problem hiding this comment.
Same effect. This was pulled from the cargo-mutants example, I'll change to cargo install cargo-mutants to be consistent.
.github/workflows/build.yml
Outdated
| - name: Mutants | ||
| run: | | ||
| cargo mutants --no-shuffle -j 2 -vV --in-diff git.diff | ||
| - name: Archive mutants.out |
There was a problem hiding this comment.
Does the CLI output for cargo mutants not suffice? What additional data does this give?
There was a problem hiding this comment.
I don't believe we'll need this either. It'd only be useful if we had some perhaps custom app to feed it to so it could leave a PR comment like codecov. Will remove for now.
Skip requiring it to pass for merge is done from the GitHub project settings, right? Sounds good. Let me clean it up. |
dunxen
force-pushed
the
2023-11-mutation-testing
branch
from
a7f6e21 to
381c512
Compare
|
This LGTM, feel free to drop the "trigger" part of the diff, I think we should just land this and deal with if we want to make it quieter somehow later. |
oh gosh thought I dropped it but probably squashed instead. anyway, it helped check if it still works after the last changes to the action! will drop the trigger now. |
We introduce a CI job for mutation testing of PR diffs using cargo-mutants. Missed cases do not trigger a fail of this job yet as we just introduce it now for visibility. We may start enforcing stricter rules at a later stage.
dunxen
force-pushed
the
2023-11-mutation-testing
branch
from
381c512 to
c42699d
Compare
We introduce a CI job for mutation testing of PR diffs using
cargo-mutants.Missed cases do not trigger a fail of this job yet as we just introduce it now for visibility. We may start enforcing stricter rules at a later stage.
See #188. Only partially addresses everything we want I believe.