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

Issue #7801: Resolve Pitest Issues - AvoidStaticImportCheck #7828

Merged
merged 1 commit into from Mar 12, 2020

Conversation

gaurabdg
Copy link
Contributor

@gaurabdg gaurabdg commented Mar 11, 2020

Fixes #7801

Adds an UT for fixing the issue mentioned in #7801 (comment):

4. Regression and Code Logic Analysis

Although the results of the regression report wasn't useful, I have discovered an UT by analysing the code which tests a particular edge case of if(exclude.endsWith(".*")) . The reason the absence of the if statement didn't break the test codes:
if(exclude.endsWith(".*")) is redundant, since even if this check wasn't there and let's assume the exclude was a random member, it would yield true for the second condition but not for the first condition in if (classOrStaticMember.startsWith(excludeMinusDotStar)&& !classOrStaticMember.equals(excludeMinusDotStar)). and hence not move forward.

But if a specific exclude had a pattern like package.path.class.<member> where was a single letter ( for e.g. java.lang.Math.E) the absence of the if statement would yield that as if .E is .* and give a false positive. Hence, including this test case has forced the if statement to do an important check and the mutation was killed.

@strkkk
Copy link
Member

strkkk commented Mar 11, 2020

@gaurabdg I guess you should remove the line with exclusion from pitest.sh which was in original issue to verify that you fixed it.

"AvoidStaticImportCheck.java.html:<td class='covered'><pre><span class='survived'> if (exclude.endsWith(&#34;.*&#34;)) {</span></pre></td></tr>"

@gaurabdg
Copy link
Contributor Author

gaurabdg commented Mar 11, 2020

@strkkk Sorry, totally missed that, but nevertheless I tested locally and had uploaded the pitest-report in the issue, the mutation is taken care of. Here is the link: https://gaurabdg.github.io/checkstyle-tester-reports/pitest/AvoidStaticImport-fixed/index.html

@strkkk strkkk self-assigned this Mar 11, 2020
@strkkk strkkk requested a review from romani March 11, 2020 17:27
@strkkk strkkk assigned romani and unassigned strkkk Mar 11, 2020
@rnveach
Copy link
Member

rnveach commented Mar 11, 2020

I guess you should remove the line with exclusion from pitest.sh which was in original issue to verify that you fixed it.

CI will fail if mutation is killed but left in file that only lists surviving mutants. The only thing that needs to be watched out for is CI passing and the file not being modified. Then this means the mutation was not killed and still survives. When this happens, the new test case that was added is not fulfilling its role that it was designed for.

I will add a new step in the main issue for this, but you should post the new pitest report when you add the test case to show the mutation is actually being killed now as a final confirmation.

Copy link
Member

@rnveach rnveach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything I have seen satisfies me. CI will find any remaining issues I would have.

@romani romani merged commit 555199e into checkstyle:master Mar 12, 2020
@gaurabdg gaurabdg deleted the pitest-fix branch March 12, 2020 03:47
@gaurabdg gaurabdg restored the pitest-fix branch March 12, 2020 03:47
@gaurabdg
Copy link
Contributor Author

gaurabdg commented Mar 12, 2020

I will add a new step in the main issue for this, but you should post the new pitest report when you add the test case to show the mutation is actually being killed now as a final confirmation.

I did it in the #7801 (comment):

Pitest Report after the fix: https://gaurabdg.github.io/checkstyle-tester-reports/pitest/AvoidStaticImport-fixed/index.html

@gaurabdg gaurabdg deleted the pitest-fix branch March 12, 2020 05:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Resolve Pitest Issues - AvoidStaticImportCheck
4 participants