-
Notifications
You must be signed in to change notification settings - Fork 78
Diff based cargo-mutants #910
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
Conversation
Pull Request Test Coverage Report for Build 16662599796Details
💛 - Coveralls |
1286515 to
3642002
Compare
|
|
0ffb33b to
283f6cb
Compare
3f97303 to
438cec4
Compare
|
@DanGould certainly this can vary the CI test time based on how complex the diff is but it does seem on par with our integration tests for most changes |
spacebear21
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK - 3min 30s for an isolated change sounds reasonable. I guess the worst-case would be around 40min (based on the weekly job time) if every possible mutant was touched?
.github/workflows/cargo-mutants.yml
Outdated
| - uses: dtolnay/rust-toolchain@master | ||
| with: | ||
| toolchain: beta |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use the stable toolchain to keep cache usage efficient:
| - uses: dtolnay/rust-toolchain@master | |
| with: | |
| toolchain: beta | |
| - uses: dtolnay/rust-toolchain@stable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did some additional cleanup to better match this workflow with the rest of our actions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this keep it efficient? Is it because we have a cache from using the stable toolchain already elsewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok so It is intended to use the cache from the stable toolchain but looking closer at it I am a bit confused as to whether we are using it correctly, making a more pointed issue in #912
The cache here is specific to the Diff Mutants job so we should be able to save some time for the stable toolchain with subsequent runs of this job, stable is chosen so the hash doesn't change often
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my reply here: #912 (comment)
.github/workflows/cargo-mutants.yml
Outdated
| save-if: false | ||
|
|
||
| - name: Run cargo-mutants on diff | ||
| run: > |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we update the output of this command to only print missed mutants?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer keeping the workflows separated by function rather than by tool, i.e. leave this file as-is (or rename to just cron-weekly.yml) and put the diff mutants job in rust.yml.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I like moving the diff mutant into the rust.yml it feels more correct as we use that for PR CI
This workflow is a planned replacement for the current weekly mutants that run every sunday on the rust-payjoin codebase. This will hopefully allow more continuous coverage without a weekly delay.
I added one additional test for mutation coverage on ohttp logic with my plan to create more tests for full test coverage to follow.
|
Looks good to me, pending removal of the DO NOT MERGE commit. |
|
I was confused at first that this might check the diff in mutants.out since we don't commit or cache a mutants.out for each commit on |
spacebear21
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK aeba995
There was a rebasing mistake on a previous commit as I created some exclusions but their name was changed before payjoin#910 was merged and they were not caught.
There was a rebasing mistake on a previous commit as I created some exclusions but their name was changed before payjoin#910 was merged and they were not caught. There is one exclusion that is simply renamed but another I decided to change the logic such that the exclusions are no longer possible.
This is a PR to test adding diff based mutations for future changes in the source code moving forward.
Note: this does not add mutations if the
mutants.tomlor other non.rscode is changedThe last 2 commits are simply testing commits to show diff mutations are caught or missed or the diff is ignored alltogether.
.rsfile was changed since it is in the test cfg it is ultimately ignored