-
Notifications
You must be signed in to change notification settings - Fork 39
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
Report mutation test coverage of proposed changes #346
Conversation
Looks good. No mutations were possible for these changes. |
|
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed, or the full report under Checks -> Mutation testing. |
1 similar comment
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed, or the full report under Checks -> Mutation testing. |
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
1 similar comment
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
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.
Before reviewing this PR, consider reading the corresponding blog post.
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.
Started reviewing this one, cool stuff!
Added a commit.
This might break things, let's see later when testing the changes 👀.
cdg-pitest-licence.txt
Outdated
@@ -0,0 +1,7 @@ | |||
#Licence file for pitest extensions |
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.
We use License
everywhere with a "s", let's see if that works here as well.
- name: Run Pitest | ||
run: mvn test pitest:mutationCoverage -DargLine.xmx=2048m -Dverification.skip -Dfeatures="+GIT(from[HEAD~1]), +gitci" | ||
- name: Aggregate Pitest reports | ||
run: mvn pitest-git:aggregate -DtrailingText="Mutation testing report by [Pitest](https://pitest.org/). Review any surviving mutants by inspecting the line comments under [_Files changed_](${{ github.event.number }}/files)." |
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.
Seems fun to me to add some values for killedEmoji
and mutantEmoji
. Will propose something 👀.
Relevant documentation here.
db9c60c
to
649066f
Compare
Rebased :). (Ouch, my bad, I think not rebasing was a deliberate choice, seeing the commits and comments after the commits. Sorry 😞). Clicking the commits shows the comments on the specific commits, that might help. |
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'll let you do the revert and test the other changes 🙃
cdg-pitest-license.txt
Outdated
#License file for pitest extensions | ||
#Mon Nov 07 08:41:17 GMT 2022 | ||
expires=07/11/2023 | ||
keyVersion=1 | ||
signature=MhZxMbnO6UovNfllM0JuVWkZyvRT3/G5o/uT0Mm36c7200VpZNVu03gTAGivnl9W5RzvZhfpIHccuQ5ctjQkrqhsFSrl4fyqPqu3y5V2fsHIdFXP/G72EGj6Kay9ndLpaEHalqE0bEwxdnHMzEYq5y3O9vUPv8MhUl57xk+rvBo\= | ||
packages=tech.picnic.errorprone.* | ||
type=OSSS |
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.
@rickie following your last commit the codes doesn't work anymore; the plugin looks for the exact original file. Now all analysis is skipped. From the logs:
Licenced for evaluation and demonstration purposes only.
Expires: 2023-09-20
For use with packages: com.example.*
# uploads the results. The associated PR is subsequently updated by the | ||
# `pitest-update-pr.yml` workflow. See https://blog.pitest.org/oss-pitest-pr/ | ||
# for details. | ||
name: "Mutation testing: analysis" |
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 had that : analysis
suffix in an earlier commit, and I removed it on purpose, IIRC. Something to do with how the result is displayed in the GitHub UI. (But would have to revert the last commit to see how/what.)
name: "Mutation testing: analysis" | |
name: "Mutation testing" |
cdg-pitest-license.txt
Outdated
@@ -0,0 +1,7 @@ | |||
#License file for pitest extensions |
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.
To be sure: I'd also revert the change to this file. It doesn't follow our standards anyway (missing dot at end of sentence, auto-generated timestamp), so I'd rather keep it exactly as provided.
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.
Or we should replace all comments with something like:
# Arcmutate license for Error Prone Support, requested by sending an email to
# [email protected].
(Assuming the plugin doesn't care about the comments 👀)
bfcbc3b
to
c13fd6d
Compare
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
1 similar comment
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
Looks good. No mutations were possible for these changes. |
1 similar comment
Looks good. No mutations were possible for these changes. |
Looks good. All 2 mutations in this change were killed.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
1 similar comment
Looks good. All 2 mutations in this change were killed.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
Looks good. No mutations were possible for these changes. |
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.
Added a commit. Also reverted the test changes.
The setup looks good to me, very cool to have this as part of the build 😄.
Doubting a little about the suggested commit message, we could provide some more information about Arcmutate and the setup we now have. I'm thinking there is a reason that you kept it high-level, so first asking whether there is a particular reason for it 👀 @Stephan202.
- name: Set up JDK | ||
uses: actions/[email protected] | ||
with: | ||
java-version: 17 |
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.
Here we should use the same version as in the existing workflow 17.0.4
, right?
Looks good. No mutations were possible for these changes. |
Latest changes LGTM! Also updated the suggested commit message. PTAL :) |
Nice, looks splendid 😄! |
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.
Looking good. Nice suggested commit message too. Didn't dive to deep into all the possbile configuration to see if we can improve further. I've got two comments, but will approve nonetheless as they are not blocking this PR for me.
7b609a5
to
1cc5e8c
Compare
Suggested commit message: