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

Pitest: Kill all surviving mutations #11720

Closed
nrmancuso opened this issue Jun 11, 2022 · 9 comments · Fixed by #11972
Closed

Pitest: Kill all surviving mutations #11720

nrmancuso opened this issue Jun 11, 2022 · 9 comments · Fixed by #11972

Comments

@nrmancuso
Copy link
Contributor

nrmancuso commented Jun 11, 2022

This will be main tracker issue for killing surviving mutations before we begin work in #11719

Now that we have diff report generation as github action, let's modify the procedure from #7797 (comment) a bit to speed this up:

  1. Disable all CI first as separate commit(See example at infra: test disabling all CI #11721). This will help to not waste credits.
  2. Pick one single surviving mutation from pitest.sh.
  3. Send PR with a single hardcoded mutation only.
  4. Generate diff report with hardcoded mutation.
  5. Share link to initial commit with hardcoded mutation and initial diff report in PR description. This is in lieu of separate branch/ etc. in previous issue at Resolve Pitest Issues #7797.

HARDCODED MUTATION WILL NOT BE IN FINAL PR.

We will have one of three outcomes for each mutation (in order of preference):

  1. Find a case that kills the mutation via diff report, add new test in existing PR
  2. Safely (no regressions) modify code to be covered, in existing PR
  3. Safely (no regressions) remove code to be covered, in existing PR

Case (2) and (3) required a detailed analysis and discussion in issue before making any changes.


We will not be pursuing surviving mutations in IndentationCheck or gui package .

@nrmancuso
Copy link
Contributor Author

nrmancuso commented Jun 11, 2022

Once we approve this issue, we should close #7797 and any child/ associated issues.

@romani
Copy link
Member

romani commented Jun 11, 2022

approved.

it would be good to see html report(s) where ALL is activated to see visually scope of work. REpost should be on github.io to let mentors review it and discuss at any time and from phones.

we need to have commit that disables all CIs. to make sure we will not eat all credits for CI provided, and we are not banned from CI usage due to misbehavior (overusage).
so PRs for search of killer will have two commits:

  1. disable all CIs jobs
  2. hardcoded survival mutation

PR should have no CI jobs started or all of them do nothing (skip all jobs).

the only activity happens by comment to trigger Github Action.

@rnveach rnveach removed their assignment Jun 13, 2022
@Vyom-Yadav
Copy link
Member

Vyom-Yadav commented Jun 18, 2022

@rnveach @nick-mancuso
The new pitest method only examines mutations.xml which isn't appropriate to determine lines that are uncovered and are not mutated (due to the absence of some mutator). Such lines can only be taken from the HTML file.

An example of such lines is present over here (modified a bit for demonstrating).

Though uncovered lines will result in low line coverage, it is rounded up in certain cases (example) and the line coverage becomes 100% :( hcoles/pitest#375

The final resort is to create some custom method, we can either check line coverage 212/213 in this sense or we can, do what we used to do, add suppression, but in that case, our suppression model needs to be different because current one operates on mutations.

We can create a new class named UncoveredLines and add that to the suppression model, ideas are welcome :)

@rnveach
Copy link
Member

rnveach commented Jun 18, 2022

Missing code coverage would also impact jacoco, which is also checking code coverage. Is Jacoco reporting similar missing code coverage is pitest only alone?

@nrmancuso
Copy link
Contributor Author

nrmancuso commented Jun 18, 2022

This seems like an improvement to me, as long as coverage is properly reported by Jacoco. Do we currently have suppressions in pitest.sh like this?

@Vyom-Yadav
Copy link
Member

Vyom-Yadav commented Jun 22, 2022

This seems like an improvement to me, as long as coverage is properly reported by Jacoco.

@nick-mancuso Yes, coverage is supported by jacoco, where pitest rounds off, jacoco doesn't so the result is more accurate. See this report, see blocks package, it shows 99% whereas pitest turns it to 100%.

Do we currently have suppressions in pitest.sh like this?

We do but that suppression is a bug, see hcoles/pitest#688

checkstyle/.ci/pitest.sh

Lines 61 to 67 in e8b63a1

pitest-main)
mvn -e --no-transfer-progress -P"$1" clean test-compile org.pitest:pitest-maven:mutationCoverage;
declare -a ignoredItems=(
"Main.java.html:<td class='uncovered'><pre><span class=''> }</span></pre></td></tr>"
);
checkPitestReport ignoredItems
;;

@nrmancuso
Copy link
Contributor Author

nrmancuso commented Jun 23, 2022

@Vyom-Yadav let's do this: since we have separated execution of pitest and generation of pitest report out of your groovy script, let's also remove pitest execution and report generation from pitest.sh, and have both scripts run for duration of GSOC as an ongoing "regression" test for your new script.

Vyom-Yadav added a commit to Vyom-Yadav/checkstyle that referenced this issue Jun 27, 2022
Vyom-Yadav added a commit to Vyom-Yadav/checkstyle that referenced this issue Jun 28, 2022
Vyom-Yadav added a commit to Vyom-Yadav/checkstyle that referenced this issue Jun 29, 2022
Vyom-Yadav added a commit to Vyom-Yadav/checkstyle that referenced this issue Jun 29, 2022
Vyom-Yadav added a commit to Vyom-Yadav/checkstyle that referenced this issue Jun 29, 2022
Vyom-Yadav added a commit to Vyom-Yadav/checkstyle that referenced this issue Jun 29, 2022
Vyom-Yadav added a commit to Vyom-Yadav/checkstyle that referenced this issue Jun 29, 2022
Vyom-Yadav added a commit to Vyom-Yadav/checkstyle that referenced this issue Jun 29, 2022
Vyom-Yadav added a commit to Vyom-Yadav/checkstyle that referenced this issue Jun 29, 2022
baratali pushed a commit that referenced this issue Jul 27, 2022
baratali pushed a commit that referenced this issue Jul 27, 2022
Vyom-Yadav added a commit to Vyom-Yadav/checkstyle that referenced this issue Jul 27, 2022
…nUsageDistanceCheck related to usage distance
baratali pushed a commit that referenced this issue Jul 27, 2022
baratali pushed a commit that referenced this issue Jul 27, 2022
…anceCheck in getFirstNodeInsideForWhileDoWhileBlocks
Vyom-Yadav added a commit to Vyom-Yadav/checkstyle that referenced this issue Jul 27, 2022
…nUsageDistanceCheck related to usage distance
Vyom-Yadav added a commit to Vyom-Yadav/checkstyle that referenced this issue Jul 27, 2022
…nUsageDistanceCheck in getFirstNodeInsideIfBlock
Vyom-Yadav added a commit to Vyom-Yadav/checkstyle that referenced this issue Jul 27, 2022
…nUsageDistanceCheck in searchVariableUsageExpressions
Vyom-Yadav added a commit to Vyom-Yadav/checkstyle that referenced this issue Jul 27, 2022
…nUsageDistanceCheck related to usage distance
nrmancuso pushed a commit that referenced this issue Jul 28, 2022
Vyom-Yadav added a commit to Vyom-Yadav/checkstyle that referenced this issue Jul 28, 2022
…nUsageDistanceCheck in searchVariableUsageExpressions
Vyom-Yadav added a commit to Vyom-Yadav/checkstyle that referenced this issue Jul 28, 2022
…nUsageDistanceCheck in getFirstNodeInsideIfBlock
Vyom-Yadav added a commit to Vyom-Yadav/checkstyle that referenced this issue Jul 28, 2022
…nUsageDistanceCheck in searchVariableUsageExpressions
Vyom-Yadav added a commit to Vyom-Yadav/checkstyle that referenced this issue Jul 28, 2022
…nUsageDistanceCheck in getFirstNodeInsideIfBlock
Vyom-Yadav added a commit to Vyom-Yadav/checkstyle that referenced this issue Jul 28, 2022
…nUsageDistanceCheck in getFirstNodeInsideIfBlock
Vyom-Yadav added a commit to Vyom-Yadav/checkstyle that referenced this issue Jul 29, 2022
…nUsageDistanceCheck in getFirstNodeInsideIfBlock
Vyom-Yadav added a commit to Vyom-Yadav/checkstyle that referenced this issue Jul 29, 2022
…nUsageDistanceCheck in getFirstNodeInsideIfBlock
@romani
Copy link
Member

romani commented Jul 31, 2022

@nick-mancuso , do we need this mega issue ?
I think it is better to create more precise issue with more clear scope and target.

baratali pushed a commit that referenced this issue Jul 31, 2022
@nrmancuso
Copy link
Contributor Author

nrmancuso commented Aug 1, 2022

We will allow #11972 to close this issue, and open more granular issues as we need to.

Vyom-Yadav added a commit to Vyom-Yadav/checkstyle that referenced this issue Aug 2, 2022
…nUsageDistanceCheck in getFirstNodeInsideIfBlock
Vyom-Yadav added a commit to Vyom-Yadav/checkstyle that referenced this issue Aug 2, 2022
…nUsageDistanceCheck in getFirstNodeInsideIfBlock
Vyom-Yadav added a commit to Vyom-Yadav/checkstyle that referenced this issue Aug 2, 2022
…nUsageDistanceCheck in getFirstNodeInsideIfBlock
Vyom-Yadav added a commit to Vyom-Yadav/checkstyle that referenced this issue Aug 2, 2022
…nUsageDistanceCheck in getFirstNodeInsideIfBlock
Vyom-Yadav added a commit to Vyom-Yadav/checkstyle that referenced this issue Aug 10, 2022
…nUsageDistanceCheck in getFirstNodeInsideIfBlock
Vyom-Yadav added a commit to Vyom-Yadav/checkstyle that referenced this issue Aug 15, 2022
…nUsageDistanceCheck in getFirstNodeInsideIfBlock
Vyom-Yadav added a commit to Vyom-Yadav/checkstyle that referenced this issue Aug 20, 2022
…nUsageDistanceCheck in getFirstNodeInsideIfBlock
baratali pushed a commit that referenced this issue Aug 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants