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: increase mutation coverage for pitest-checkstyle-common profile to 100% #4400

Closed
Nimfadora opened this Issue May 31, 2017 · 18 comments

Comments

Projects
2 participants
@Nimfadora
Contributor

Nimfadora commented May 31, 2017

We created pitest profiles for non-checks code in #4367. Currently, we should increase coverage for pitest-checkstyle-common profile up to 100%.
This issue is a subtask of #3708

Current threshold of pitest-checkstyle-common profile: 78

@Nimfadora

This comment has been minimized.

Show comment
Hide comment
@Nimfadora

Nimfadora Jun 1, 2017

Contributor

@romani, I encountered an issue when fixing mutation on this profile:
for e.z. in CheckstyleAntTask we have
log("To locate the files took " + (endTime - startTime) + TIME_SUFFIX
and pitest generates violation:
removed call to com/puppycrawl/tools/checkstyle/ant/CheckstyleAntTask::log → SURVIVED

I think we should use avoidCallsTo in such cases, do you agree?

Contributor

Nimfadora commented Jun 1, 2017

@romani, I encountered an issue when fixing mutation on this profile:
for e.z. in CheckstyleAntTask we have
log("To locate the files took " + (endTime - startTime) + TIME_SUFFIX
and pitest generates violation:
removed call to com/puppycrawl/tools/checkstyle/ant/CheckstyleAntTask::log → SURVIVED

I think we should use avoidCallsTo in such cases, do you agree?

@Nimfadora

This comment has been minimized.

Show comment
Hide comment
@Nimfadora

Nimfadora Jun 1, 2017

Contributor

@romani, also there is an issue with PackageObjectFactory#fillShortToFullModuleNamesMapSent.
Pitest shows that mutation survives if we remove any of the method calls, but actually it is not true. Looks like Pitest during its mutations does not reload class in the classloader, so the actual content of NAME_TO_FULL_MODULE_NAME map stay the same. I think this should be also excluded from pitest mutation, ok?

Contributor

Nimfadora commented Jun 1, 2017

@romani, also there is an issue with PackageObjectFactory#fillShortToFullModuleNamesMapSent.
Pitest shows that mutation survives if we remove any of the method calls, but actually it is not true. Looks like Pitest during its mutations does not reload class in the classloader, so the actual content of NAME_TO_FULL_MODULE_NAME map stay the same. I think this should be also excluded from pitest mutation, ok?

Nimfadora added a commit to Nimfadora/checkstyle that referenced this issue Jun 1, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jun 4, 2017

Member

I think we should use avoidCallsTo in such cases, do you agree?

next time please share request for smth new as link. Example http://pitest.org/quickstart/maven/#avoidcallsto . I will speed up my reply.
But this already looks like defaults of pitest for loggers.
This logging in checkstyle is reporting to output some text when ANT is working, so looks like we never check output of this class. So we need to make a test that validate output of CheckstyleAntTask.

I think this should be also excluded from pitest mutation, ok?

please share report, please always do this.

Pitest shows that mutation survives if we remove any of the method calls, but actually it is not true.

did you test this by manual removal of line of code ?
if yes , and UTs are failing - it is bug in pitest.
We can use pitest's tag avoitCallTo for this method till issue in pitest is not resolved.
BUT you have to make issue against pitest project and reference it in comment as reason of avoitCallTo.

Member

romani commented Jun 4, 2017

I think we should use avoidCallsTo in such cases, do you agree?

next time please share request for smth new as link. Example http://pitest.org/quickstart/maven/#avoidcallsto . I will speed up my reply.
But this already looks like defaults of pitest for loggers.
This logging in checkstyle is reporting to output some text when ANT is working, so looks like we never check output of this class. So we need to make a test that validate output of CheckstyleAntTask.

I think this should be also excluded from pitest mutation, ok?

please share report, please always do this.

Pitest shows that mutation survives if we remove any of the method calls, but actually it is not true.

did you test this by manual removal of line of code ?
if yes , and UTs are failing - it is bug in pitest.
We can use pitest's tag avoitCallTo for this method till issue in pitest is not resolved.
BUT you have to make issue against pitest project and reference it in comment as reason of avoitCallTo.

Nimfadora pushed a commit to Nimfadora/checkstyle that referenced this issue Jun 5, 2017

Nimfadora pushed a commit to Nimfadora/checkstyle that referenced this issue Jun 5, 2017

Nimfadora added a commit to Nimfadora/checkstyle that referenced this issue Jun 5, 2017

Nimfadora pushed a commit to Nimfadora/checkstyle that referenced this issue Jun 6, 2017

Nimfadora pushed a commit to Nimfadora/checkstyle that referenced this issue Jun 6, 2017

Nimfadora pushed a commit to Nimfadora/checkstyle that referenced this issue Jun 6, 2017

Nimfadora pushed a commit to Nimfadora/checkstyle that referenced this issue Jun 6, 2017

Nimfadora pushed a commit to Nimfadora/checkstyle that referenced this issue Jun 6, 2017

Nimfadora added a commit to Nimfadora/checkstyle that referenced this issue Jun 6, 2017

rnveach added a commit that referenced this issue Jun 7, 2017

Nimfadora added a commit to Nimfadora/checkstyle that referenced this issue Jun 7, 2017

Nimfadora added a commit to Nimfadora/checkstyle that referenced this issue Jun 7, 2017

Nimfadora added a commit to Nimfadora/checkstyle that referenced this issue Jun 7, 2017

Nimfadora added a commit to Nimfadora/checkstyle that referenced this issue Jun 8, 2017

Nimfadora added a commit to Nimfadora/checkstyle that referenced this issue Jun 12, 2017

Nimfadora pushed a commit to Nimfadora/checkstyle that referenced this issue Jun 13, 2017

Nimfadora pushed a commit to Nimfadora/checkstyle that referenced this issue Jun 13, 2017

Nimfadora added a commit to Nimfadora/checkstyle that referenced this issue Jun 14, 2017

Nimfadora added a commit to Nimfadora/checkstyle that referenced this issue Jun 16, 2017

Nimfadora added a commit to Nimfadora/checkstyle that referenced this issue Jul 3, 2017

Nimfadora added a commit to Nimfadora/checkstyle that referenced this issue Jul 3, 2017

romani added a commit that referenced this issue Jul 3, 2017

@romani romani moved this from To Do to In Progress in Practice What You Preach Jul 4, 2017

Nimfadora pushed a commit to Nimfadora/checkstyle that referenced this issue Jul 5, 2017

@Nimfadora

This comment has been minimized.

Show comment
Hide comment
@Nimfadora

Nimfadora Jul 5, 2017

Contributor

@romani
I'm stuck with this package at this stage, please provide assistance on fixing mutations remained

Contributor

Nimfadora commented Jul 5, 2017

@romani
I'm stuck with this package at this stage, please provide assistance on fixing mutations remained

Nimfadora pushed a commit to Nimfadora/checkstyle that referenced this issue Jul 5, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 5, 2017

Member

https://nimfadora.github.io/problems/com.puppycrawl.tools.checkstyle.ant/CheckstyleAntTask.java.html

CheckstyleAntTask.java//createClasspath().setRefid(classpathRef);

It is known ANT Task property
http://checkstyle.sourceforge.net/anttask.html#Parameters
https://github.com/apache/ant/blob/master/src/main/org/apache/tools/ant/types/Reference.java
https://ant.apache.org/manual/using.html#references

all you need to do is too call setRefId and then check that getRefId return you the same value.

CheckstyleAntTask.java//log(String.format(Locale.ROOT, "%d) Adding %d files from directory %s",

it put smth to output, some test need to check that such sting exists in ant task output

CheckstyleAntTask.java//scanner.scan();

https://github.com/apache/ant/blob/master/src/main/org/apache/tools/ant/DirectoryScanner.java#L822
if we remove it, we should receive exception from https://github.com/apache/ant/blob/master/src/main/org/apache/tools/ant/DirectoryScanner.java#L1538
please find a reason why this not happening .

CheckstyleAntTask.java//if (toFile == null || !useFile) {

we have the same condition at createDefaultLogger please recheck why coverage is full for it.

Member

romani commented Jul 5, 2017

https://nimfadora.github.io/problems/com.puppycrawl.tools.checkstyle.ant/CheckstyleAntTask.java.html

CheckstyleAntTask.java//createClasspath().setRefid(classpathRef);

It is known ANT Task property
http://checkstyle.sourceforge.net/anttask.html#Parameters
https://github.com/apache/ant/blob/master/src/main/org/apache/tools/ant/types/Reference.java
https://ant.apache.org/manual/using.html#references

all you need to do is too call setRefId and then check that getRefId return you the same value.

CheckstyleAntTask.java//log(String.format(Locale.ROOT, "%d) Adding %d files from directory %s",

it put smth to output, some test need to check that such sting exists in ant task output

CheckstyleAntTask.java//scanner.scan();

https://github.com/apache/ant/blob/master/src/main/org/apache/tools/ant/DirectoryScanner.java#L822
if we remove it, we should receive exception from https://github.com/apache/ant/blob/master/src/main/org/apache/tools/ant/DirectoryScanner.java#L1538
please find a reason why this not happening .

CheckstyleAntTask.java//if (toFile == null || !useFile) {

we have the same condition at createDefaultLogger please recheck why coverage is full for it.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 5, 2017

Member

https://nimfadora.github.io/problems/com.puppycrawl.tools.checkstyle/XMLLogger.java.html

XMLLogger.java//printer.flush();

strange that withtout this method we do have content in output.
autoflush is not enabled - http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/8u40-b25/java/io/PrintWriter.java#99
Ok, lets remove it.

XMLLogger.java//nextSemi < 0

remove it. Looks like should be covered by isReference.

Member

romani commented Jul 5, 2017

https://nimfadora.github.io/problems/com.puppycrawl.tools.checkstyle/XMLLogger.java.html

XMLLogger.java//printer.flush();

strange that withtout this method we do have content in output.
autoflush is not enabled - http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/8u40-b25/java/io/PrintWriter.java#99
Ok, lets remove it.

XMLLogger.java//nextSemi < 0

remove it. Looks like should be covered by isReference.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 5, 2017

Member

https://nimfadora.github.io/problems/com.puppycrawl.tools.checkstyle/DefaultConfiguration.java.html

If line is covered by test .. it is a bug in pitest, as mutation should be specified for this line. Please open issue on pitest.
As workround for us, can you make UT that call this method only and check value is set.

Member

romani commented Jul 5, 2017

https://nimfadora.github.io/problems/com.puppycrawl.tools.checkstyle/DefaultConfiguration.java.html

If line is covered by test .. it is a bug in pitest, as mutation should be specified for this line. Please open issue on pitest.
As workround for us, can you make UT that call this method only and check value is set.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 5, 2017

Member

https://nimfadora.github.io/problems/com.puppycrawl.tools.checkstyle/ConfigurationLoader.java.html

all cases are from private static void parsePropertyString that is copy from ANT project, now it looks like https://github.com/apache/ant/blob/master/src/main/org/apache/tools/ant/PropertyHelper.java#L1021 a bit different.
Please make extra UTs that call this private static method by reflection and give it all required input string to cover that cases.

Member

romani commented Jul 5, 2017

https://nimfadora.github.io/problems/com.puppycrawl.tools.checkstyle/ConfigurationLoader.java.html

all cases are from private static void parsePropertyString that is copy from ANT project, now it looks like https://github.com/apache/ant/blob/master/src/main/org/apache/tools/ant/PropertyHelper.java#L1021 a bit different.
Please make extra UTs that call this private static method by reflection and give it all required input string to cover that cases.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 5, 2017

Member

https://nimfadora.github.io/problems/com.puppycrawl.tools.checkstyle/Checker.java.html

.forEach(filter -> {

please remove the same block at fileSetChecks.stream().filter(check -> check instanceof ExternalResourceHolder) and see who is failed. And how that do asserts.

cache.remove(fileName);

looks like we missed assert on file content of created cache. Please review cache related UTs. Call fireError to make hasNonFilteredViolations=true, and check cache status by reflection.

for all other ... looks like completely missed coverage , please prove that coverege exists, and lets report this to pitest project.

Member

romani commented Jul 5, 2017

https://nimfadora.github.io/problems/com.puppycrawl.tools.checkstyle/Checker.java.html

.forEach(filter -> {

please remove the same block at fileSetChecks.stream().filter(check -> check instanceof ExternalResourceHolder) and see who is failed. And how that do asserts.

cache.remove(fileName);

looks like we missed assert on file content of created cache. Please review cache related UTs. Call fireError to make hasNonFilteredViolations=true, and check cache status by reflection.

for all other ... looks like completely missed coverage , please prove that coverege exists, and lets report this to pitest project.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 5, 2017

Member
  1. Pitest will be is updated to 1.2.1 so suppression that we did at https://github.com/checkstyle/checkstyle/blob/master/pom.xml#L2011 should be revered after merge of PR #4627
Member

romani commented Jul 5, 2017

  1. Pitest will be is updated to 1.2.1 so suppression that we did at https://github.com/checkstyle/checkstyle/blob/master/pom.xml#L2011 should be revered after merge of PR #4627

romani added a commit that referenced this issue Jul 6, 2017

@Nimfadora

This comment has been minimized.

Show comment
Hide comment
@Nimfadora

Nimfadora Jul 6, 2017

Contributor
  1. all you need to do is too call setRefId and then check that getRefId return you the same value.

I've tried to do so, but it doen't working. E.g. such test gives me NPE on refid.getRefId()

    @Test
    public void testSetRefId(){
        final CheckstyleAntTask antTask = new CheckstyleAntTask();
        antTask.setClasspathRef(new Reference("id"));
        final Path classpath = (Path) Whitebox.getInternalState(antTask, "classpath");
        final Reference refid = classpath.getRefid();
        assertEquals("id,", refid.getRefId());
    }

I have a feeling that smth wrong with createClasspath method. It has such line - return classpath.createPath(); I looked deeper in the source of this method and it creates a nested Path. So, the refId is set not to the classpath, but to the some nested path in the classpath.

Contributor

Nimfadora commented Jul 6, 2017

  1. all you need to do is too call setRefId and then check that getRefId return you the same value.

I've tried to do so, but it doen't working. E.g. such test gives me NPE on refid.getRefId()

    @Test
    public void testSetRefId(){
        final CheckstyleAntTask antTask = new CheckstyleAntTask();
        antTask.setClasspathRef(new Reference("id"));
        final Path classpath = (Path) Whitebox.getInternalState(antTask, "classpath");
        final Reference refid = classpath.getRefid();
        assertEquals("id,", refid.getRefId());
    }

I have a feeling that smth wrong with createClasspath method. It has such line - return classpath.createPath(); I looked deeper in the source of this method and it creates a nested Path. So, the refId is set not to the classpath, but to the some nested path in the classpath.

@Nimfadora

This comment has been minimized.

Show comment
Hide comment
@Nimfadora

Nimfadora Jul 6, 2017

Contributor
  1. CheckstyleAntTask.java//scanner.scan(); if we remove it, we should receive exception from
    please find a reason why this not happening .

It is not happenning cause scanner.scan() is executed before return in AbstractFileSet.getDirectoryScanner method.
I guess this line is redundant, isn't it?

Contributor

Nimfadora commented Jul 6, 2017

  1. CheckstyleAntTask.java//scanner.scan(); if we remove it, we should receive exception from
    please find a reason why this not happening .

It is not happenning cause scanner.scan() is executed before return in AbstractFileSet.getDirectoryScanner method.
I guess this line is redundant, isn't it?

@Nimfadora

This comment has been minimized.

Show comment
Hide comment
@Nimfadora

Nimfadora Jul 6, 2017

Contributor

6 XMLLogger.java//nextSemi < 0
remove it. Looks like should be covered by isReference.

It not working. Tests are failing. Pitest mutation that survived changed < to <= . I guess we can replace nextSemi < 0 to nextSemi != -1, cause, as I understand, it is actually what we wanna check - if there any semicolon after &. Is it ok?

Contributor

Nimfadora commented Jul 6, 2017

6 XMLLogger.java//nextSemi < 0
remove it. Looks like should be covered by isReference.

It not working. Tests are failing. Pitest mutation that survived changed < to <= . I guess we can replace nextSemi < 0 to nextSemi != -1, cause, as I understand, it is actually what we wanna check - if there any semicolon after &. Is it ok?

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 6, 2017

Member

if there any semicolon after &. Is it ok?

lets try.

Member

romani commented Jul 6, 2017

if there any semicolon after &. Is it ok?

lets try.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 6, 2017

Member

So, the refId is set not to the classpath, but to the some nested path in the classpath.

    /** This test is created to satisfy pitest, it is hard to emulate Referece by Id */
    @Test
    public void testSetClasspath() {
        final CheckstyleAntTask antTask = new CheckstyleAntTask();
        final Project project = new Project();
        antTask.setClasspath(new Path(project, "firstPath"));
        antTask.setClasspathRef(new Reference(project,"idXX"));

        try {
            assertNotNull("Classpath should not be null",
                    Whitebox.getInternalState(antTask, "classpath"));
            final Path classpath = (Path) Whitebox.getInternalState(antTask, "classpath");
            classpath.list();
        } catch (BuildException ex) {
            assertEquals("unexpected exception message", "Reference idXX not found.", ex.getMessage());
        }
    }
Member

romani commented Jul 6, 2017

So, the refId is set not to the classpath, but to the some nested path in the classpath.

    /** This test is created to satisfy pitest, it is hard to emulate Referece by Id */
    @Test
    public void testSetClasspath() {
        final CheckstyleAntTask antTask = new CheckstyleAntTask();
        final Project project = new Project();
        antTask.setClasspath(new Path(project, "firstPath"));
        antTask.setClasspathRef(new Reference(project,"idXX"));

        try {
            assertNotNull("Classpath should not be null",
                    Whitebox.getInternalState(antTask, "classpath"));
            final Path classpath = (Path) Whitebox.getInternalState(antTask, "classpath");
            classpath.list();
        } catch (BuildException ex) {
            assertEquals("unexpected exception message", "Reference idXX not found.", ex.getMessage());
        }
    }
@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 6, 2017

Member

In CheckstyleAntTask, we do:

            final FileSet fileSet = fileSets.get(i);
            final DirectoryScanner scanner = fileSet.getDirectoryScanner(getProject());
            scanner.scan();

FileSet is extending AbstractFileSet so we run AbstractFileSet.getDirectoryScanner, but this method do scan() :

.....
            ds.scan();
            return ds;

so our scan() is redundant, please remove.
@Nimfadora , please do explanation as I do with all details, in other case I need to redo whole investigation as you did to confirm.

Member

romani commented Jul 6, 2017

In CheckstyleAntTask, we do:

            final FileSet fileSet = fileSets.get(i);
            final DirectoryScanner scanner = fileSet.getDirectoryScanner(getProject());
            scanner.scan();

FileSet is extending AbstractFileSet so we run AbstractFileSet.getDirectoryScanner, but this method do scan() :

.....
            ds.scan();
            return ds;

so our scan() is redundant, please remove.
@Nimfadora , please do explanation as I do with all details, in other case I need to redo whole investigation as you did to confirm.

@Nimfadora

This comment has been minimized.

Show comment
Hide comment
@Nimfadora

Nimfadora Jul 6, 2017

Contributor

please do explanation as I do with all details, in other case I need to redo whole investigation as you did to confirm.

Ok.

/** This test is created to satisfy pitest, it is hard to emulate Referece by Id */

This test doesn't kill this mutation... Test is green, even if line CheckstyleAntTask.java#L209 is removed

Contributor

Nimfadora commented Jul 6, 2017

please do explanation as I do with all details, in other case I need to redo whole investigation as you did to confirm.

Ok.

/** This test is created to satisfy pitest, it is hard to emulate Referece by Id */

This test doesn't kill this mutation... Test is green, even if line CheckstyleAntTask.java#L209 is removed

Nimfadora added a commit to Nimfadora/checkstyle that referenced this issue Jul 6, 2017

romani added a commit that referenced this issue Jul 7, 2017

@romani romani added this to the 8.1 milestone Jul 7, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 7, 2017

Member

last fix is merged.

Member

romani commented Jul 7, 2017

last fix is merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment