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

Unexpected loss of coverage for PropertyCacheFile.java #3594

Closed
romani opened this Issue Nov 29, 2016 · 4 comments

Comments

Projects
None yet
2 participants
@romani
Member

romani commented Nov 29, 2016

https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/PropertyCacheFile.java

Catch block is not covered :

    private static byte[] loadExternalResource(String location) throws CheckstyleException {
        byte[] content = null;
        final URI uri = CommonUtils.getUriByFilename(location);

        try {
            .....
        }
        catch (IOException ex) {
            throw new CheckstyleException("Unable to load external resource file " + location, ex);
        }

        return content;
    }

noticed at https://travis-ci.org/checkstyle/checkstyle/builds/179591339

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Nov 29, 2016

Member

Travis: https://travis-ci.org/checkstyle/checkstyle/builds/179591339

Travis is failing because we unexpectedly lost code coverage of the above mentioned catch.
We had this covered before. It was covered by CheckerTest.testCacheIoExceptionWhenReadingExternalResource.

The reason for the change was due to an external URL not throwing the exception anymore when loading http://mock.sourceforge.net/suppressions_none.xml. Reason for no exception being thrown now is unknown atm.

The biggest thing to take note of is when this external change happened, code coverage failed but our tests still passed. Ideally, both should fail not one. For our tests not to fail, it means we are missing testing something. So I consider this to be a secondary failure.
The reason for this secondary failure is because we are calling methods and not verifying any type of result.
Example:

verify(checker, pathToEmptyFile, pathToEmptyFile, expected);
// One more time to use cahce.
verify(checker, pathToEmptyFile, pathToEmptyFile, expected);

Even though we are expecting and testing an exception, the test in question doesn't throw an exception to junit because PropertyCacheFile.loadExternalResources(Set<String> resourceLocations) hides the exception and does other processing. No where in those lines in CheckerTest are we testing and verifying to make sure the exception happens.

There are more tests like this for the cache in CheckerTest that could cause the same type of failure. Some examples:

verify(checker, pathToEmptyFile, expected);
// Once again to invalidate cache because in the second run IOException will happen
// on different line for DummyFileSetCheck and it will change the content
verify(checker, pathToEmptyFile, expected);

verify(checker, pathToEmptyFile, expected);
// One more time to use cache.
verify(checker, pathToEmptyFile, expected);

verify(checker, pathToEmptyFile, expected);
// Change a list of external resources which are used by the check
check.setSecondExternalResourceLocation("checks" + File.separator
+ "imports" + File.separator + "import-control_one-re.xml");
verify(checker, pathToEmptyFile, expected);

Member

rnveach commented Nov 29, 2016

Travis: https://travis-ci.org/checkstyle/checkstyle/builds/179591339

Travis is failing because we unexpectedly lost code coverage of the above mentioned catch.
We had this covered before. It was covered by CheckerTest.testCacheIoExceptionWhenReadingExternalResource.

The reason for the change was due to an external URL not throwing the exception anymore when loading http://mock.sourceforge.net/suppressions_none.xml. Reason for no exception being thrown now is unknown atm.

The biggest thing to take note of is when this external change happened, code coverage failed but our tests still passed. Ideally, both should fail not one. For our tests not to fail, it means we are missing testing something. So I consider this to be a secondary failure.
The reason for this secondary failure is because we are calling methods and not verifying any type of result.
Example:

verify(checker, pathToEmptyFile, pathToEmptyFile, expected);
// One more time to use cahce.
verify(checker, pathToEmptyFile, pathToEmptyFile, expected);

Even though we are expecting and testing an exception, the test in question doesn't throw an exception to junit because PropertyCacheFile.loadExternalResources(Set<String> resourceLocations) hides the exception and does other processing. No where in those lines in CheckerTest are we testing and verifying to make sure the exception happens.

There are more tests like this for the cache in CheckerTest that could cause the same type of failure. Some examples:

verify(checker, pathToEmptyFile, expected);
// Once again to invalidate cache because in the second run IOException will happen
// on different line for DummyFileSetCheck and it will change the content
verify(checker, pathToEmptyFile, expected);

verify(checker, pathToEmptyFile, expected);
// One more time to use cache.
verify(checker, pathToEmptyFile, expected);

verify(checker, pathToEmptyFile, expected);
// Change a list of external resources which are used by the check
check.setSecondExternalResourceLocation("checks" + File.separator
+ "imports" + File.separator + "import-control_one-re.xml");
verify(checker, pathToEmptyFile, expected);

romani added a commit that referenced this issue Nov 29, 2016

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Dec 9, 2016

Member

fix is merged, no problems with coverage

Member

romani commented Dec 9, 2016

fix is merged, no problems with coverage

@romani romani closed this Dec 9, 2016

@romani romani added this to the 7.4 milestone Dec 9, 2016

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Dec 9, 2016

Member

@romani We aren't going to examine/fix any of the other areas I identified in my comment that could cause similar issues?

Member

rnveach commented Dec 9, 2016

@romani We aren't going to examine/fix any of the other areas I identified in my comment that could cause similar issues?

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Dec 9, 2016

Member

please open new issue on this with clear description what to do

Member

romani commented Dec 9, 2016

please open new issue on this with clear description what to do

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