Avoid catching of InvalidPathException and AccessDeniedException in PropertyCacheFile#persist #3259

Merged
merged 1 commit into from Jun 17, 2016

Conversation

Projects
None yet
4 participants
@MEZk
Contributor

MEZk commented Jun 8, 2016

#3259

There is no need in catching of InvalidPathException since it extends RuntimeException and we just wrap it in IllegalStateException which extends RuntimeException too without additional information.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jun 8, 2016

Member

If problem on travis reproduce and you will need to change commit, please make reference to pullrequest itself as it not related to issue that much and could be independed fix

Member

romani commented Jun 8, 2016

If problem on travis reproduce and you will need to change commit, please make reference to pullrequest itself as it not related to issue that much and could be independed fix

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Jun 8, 2016

Contributor

@romani

Let's take a look at CheckerTest#testDestroyCacheFileWithInvalidPath. On Linux file name which length is 300 chars causes FileNotFoundException which is caught by catch (IOException ex) block in Checker#destroy. On Windows OS file name C:invalid causes InvalidPathException which extends RuntimeException and is not caught. If we remove caught of InvalidPathException we will have broken UTs on WIndows.

Do we really need such tests?

This should work for both OSs:

    @Test
    public void testDestroyCacheFileWithInvalidPath() throws Exception {
        final Checker checker = new Checker();
        final PackageObjectFactory factory = new PackageObjectFactory(
            new HashSet<String>(), Thread.currentThread().getContextClassLoader());
        checker.setModuleFactory(factory);
        checker.configure(new DefaultConfiguration("default config"));
        if (System.getProperty("os.name")
            .toLowerCase(Locale.ENGLISH).startsWith("windows")) {
            // https://support.microsoft.com/en-us/kb/177506 but this only for NTFS
            // WindowsServer 2012 use Resilient File System (ReFS), so any name is ok
            final File file = new File("C\\:invalid");
            checker.setCacheFile(file.getAbsolutePath());
        }
        else {
            checker.setCacheFile("\0file:F\\0server.properties");
        }
        try {
            checker.destroy();
            fail("Exception did not happen");
        }
        catch (RuntimeException ex) {
            assertThat(ex, instanceOf(InvalidPathException.class));
        }
    }

But I don't like the idea of catching RuntimeExceptions even for UTs. By the way, if we delete testDestroyCacheFileWithInvalidPath it will not affect coverage rate.

Contributor

MEZk commented Jun 8, 2016

@romani

Let's take a look at CheckerTest#testDestroyCacheFileWithInvalidPath. On Linux file name which length is 300 chars causes FileNotFoundException which is caught by catch (IOException ex) block in Checker#destroy. On Windows OS file name C:invalid causes InvalidPathException which extends RuntimeException and is not caught. If we remove caught of InvalidPathException we will have broken UTs on WIndows.

Do we really need such tests?

This should work for both OSs:

    @Test
    public void testDestroyCacheFileWithInvalidPath() throws Exception {
        final Checker checker = new Checker();
        final PackageObjectFactory factory = new PackageObjectFactory(
            new HashSet<String>(), Thread.currentThread().getContextClassLoader());
        checker.setModuleFactory(factory);
        checker.configure(new DefaultConfiguration("default config"));
        if (System.getProperty("os.name")
            .toLowerCase(Locale.ENGLISH).startsWith("windows")) {
            // https://support.microsoft.com/en-us/kb/177506 but this only for NTFS
            // WindowsServer 2012 use Resilient File System (ReFS), so any name is ok
            final File file = new File("C\\:invalid");
            checker.setCacheFile(file.getAbsolutePath());
        }
        else {
            checker.setCacheFile("\0file:F\\0server.properties");
        }
        try {
            checker.destroy();
            fail("Exception did not happen");
        }
        catch (RuntimeException ex) {
            assertThat(ex, instanceOf(InvalidPathException.class));
        }
    }

But I don't like the idea of catching RuntimeExceptions even for UTs. By the way, if we delete testDestroyCacheFileWithInvalidPath it will not affect coverage rate.

MEZk added a commit to MEZk/checkstyle that referenced this pull request Jun 8, 2016

@MEZk MEZk changed the title from Issue #3177: Avoid catching of run-time exception to Avoid catching of run-time exception Jun 8, 2016

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Jun 8, 2016

Current coverage is 76.76%

Merging #3259 into master will decrease coverage by 23.23%

@@           master      #3259   diff @@
========================================
  Files         276        276           
  Lines       14008      13989     -19   
  Methods         0          0           
  Messages        0          0           
  Branches     3261       3251     -10   
========================================
- Hits        14008      10739   -3269   
  Misses          0          0           
- Partials        0       3250   +3250   

Powered by Codecov. Last updated by 40a0b5d...d7cb7ac

codecov-io commented Jun 8, 2016

Current coverage is 76.76%

Merging #3259 into master will decrease coverage by 23.23%

@@           master      #3259   diff @@
========================================
  Files         276        276           
  Lines       14008      13989     -19   
  Methods         0          0           
  Messages        0          0           
  Branches     3261       3251     -10   
========================================
- Hits        14008      10739   -3269   
  Misses          0          0           
- Partials        0       3250   +3250   

Powered by Codecov. Last updated by 40a0b5d...d7cb7ac

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jun 8, 2016

Member

Do we really need such tests?

It was created to make coverage as 100%, if coverage is not changing and code code still be reasonable - I am ok with changes.

But I don't like the idea of catching RuntimeExceptions even for UTs.

Do you know alternative ? if method throw runtime exception by design - UT have to catch it and verify its content to make sure it is the same exception as it expect .

Member

romani commented Jun 8, 2016

Do we really need such tests?

It was created to make coverage as 100%, if coverage is not changing and code code still be reasonable - I am ok with changes.

But I don't like the idea of catching RuntimeExceptions even for UTs.

Do you know alternative ? if method throw runtime exception by design - UT have to catch it and verify its content to make sure it is the same exception as it expect .

MEZk added a commit to MEZk/checkstyle that referenced this pull request Jun 9, 2016

MEZk added a commit to MEZk/checkstyle that referenced this pull request Jun 9, 2016

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Jun 11, 2016

Contributor

@romani

if method throw runtime exception by design - UT have to catch it and verify its content to make sure it is the same exception as it expect .

Ok.

  1. We need to cover catching IOException in Checker#destroy by UT, thus, I reimplemented CheckerTest#testDestroyNonExistingCache in order to test catching of IOException in Checker#destroy. Works fine on both Linux and Windows.
  2. Nevertheless, coverage rate of PropertyCacheFile decreased on OS Windows (98%). See Cobertura report. As you can see from the report AccessDeniedException never happens, because we cannot set directory permissions from Java on OS Windows. More info: http://stackoverflow.com/a/4354686 and google/google-oauth-java-client#55 (comment) . Once again, do we really need to catch AccessDeniedException in PropertyCacheFile#persist? It extends IOException which is already caught in Checker#destroy. If yes, why don't we catch FileNotFoundException, etc in PropertyCacheFile#persist?
  3. All the changes in the PR do not fail UTs at Wercker, Docker.
Contributor

MEZk commented Jun 11, 2016

@romani

if method throw runtime exception by design - UT have to catch it and verify its content to make sure it is the same exception as it expect .

Ok.

  1. We need to cover catching IOException in Checker#destroy by UT, thus, I reimplemented CheckerTest#testDestroyNonExistingCache in order to test catching of IOException in Checker#destroy. Works fine on both Linux and Windows.
  2. Nevertheless, coverage rate of PropertyCacheFile decreased on OS Windows (98%). See Cobertura report. As you can see from the report AccessDeniedException never happens, because we cannot set directory permissions from Java on OS Windows. More info: http://stackoverflow.com/a/4354686 and google/google-oauth-java-client#55 (comment) . Once again, do we really need to catch AccessDeniedException in PropertyCacheFile#persist? It extends IOException which is already caught in Checker#destroy. If yes, why don't we catch FileNotFoundException, etc in PropertyCacheFile#persist?
  3. All the changes in the PR do not fail UTs at Wercker, Docker.
@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jun 12, 2016

Member

Once again, do we really need to catch AccessDeniedException in PropertyCacheFile#persist?

Yes, only if there is no "throws" for this type of Checked exception.
That is not a case now.
https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/PropertyCacheFile.java#L140
public void persist() throws IOException {

https://docs.oracle.com/javase/7/docs/api/java/nio/file/AccessDeniedException.html
so catching for AccessDeniedException that is child class of IOException

... 
   java.io.IOException
        java.nio.file.FileSystemException
            java.nio.file.AccessDeniedException 

does not make that much sense if no extra context is reported in wrap exception.

Resolution: I am ok to remove that catch.

Member

romani commented Jun 12, 2016

Once again, do we really need to catch AccessDeniedException in PropertyCacheFile#persist?

Yes, only if there is no "throws" for this type of Checked exception.
That is not a case now.
https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/PropertyCacheFile.java#L140
public void persist() throws IOException {

https://docs.oracle.com/javase/7/docs/api/java/nio/file/AccessDeniedException.html
so catching for AccessDeniedException that is child class of IOException

... 
   java.io.IOException
        java.nio.file.FileSystemException
            java.nio.file.AccessDeniedException 

does not make that much sense if no extra context is reported in wrap exception.

Resolution: I am ok to remove that catch.

MEZk added a commit to MEZk/checkstyle that referenced this pull request Jun 13, 2016

@MEZk MEZk changed the title from Avoid catching of run-time exception to Avoid catching of InvalidPathException and AccessDeniedException in PropertyCacheFile#persist Jun 13, 2016

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Jun 13, 2016

Contributor
  1. The catch block in PropertyCacheFile was removed.
  2. There are 2 UTs remain broken at Docker, Wercker CI:
MainTest.testExistingFilePlainOutputToFileWithoutReadAndRwPermissions System.exit has not been called.
MainTest.testExistingTargetFilePlainOutputToFileWithoutRwPermissions System.exit has not been called.
Contributor

MEZk commented Jun 13, 2016

  1. The catch block in PropertyCacheFile was removed.
  2. There are 2 UTs remain broken at Docker, Wercker CI:
MainTest.testExistingFilePlainOutputToFileWithoutReadAndRwPermissions System.exit has not been called.
MainTest.testExistingTargetFilePlainOutputToFileWithoutRwPermissions System.exit has not been called.

MEZk added a commit to MEZk/checkstyle that referenced this pull request Jun 13, 2016

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jun 13, 2016

Member

Travis:

Caused by: com.puppycrawl.tools.checkstyle.api.CheckstyleException: Exception was thrown while processing /home/travis/build/checkstyle/checkstyle/contribution/checkstyle-tester/src/main/java/checkstyle/src/test/resources/com/puppycrawl/tools/checkstyle/grammars/InputRegressionJava8Class1.java
Caused by: java.lang.NullPointerException
at com.puppycrawl.tools.checkstyle.checks.coding.ParameterAssignmentCheck.visitMethodParameters(ParameterAssignmentCheck.java:242)

Issue is caused by the addition of receiver parameters from Issue #3239.
Reason why the receiver PR didn't catch this is because Travis' NoExceptionTest is always 1 commit behind by testing master, instead of testing the PR. Another reason is no regression was done on the grammar change with all checks on the JDK8.
I am looking into the failing checks with the new code.

Member

rnveach commented Jun 13, 2016

Travis:

Caused by: com.puppycrawl.tools.checkstyle.api.CheckstyleException: Exception was thrown while processing /home/travis/build/checkstyle/checkstyle/contribution/checkstyle-tester/src/main/java/checkstyle/src/test/resources/com/puppycrawl/tools/checkstyle/grammars/InputRegressionJava8Class1.java
Caused by: java.lang.NullPointerException
at com.puppycrawl.tools.checkstyle.checks.coding.ParameterAssignmentCheck.visitMethodParameters(ParameterAssignmentCheck.java:242)

Issue is caused by the addition of receiver parameters from Issue #3239.
Reason why the receiver PR didn't catch this is because Travis' NoExceptionTest is always 1 commit behind by testing master, instead of testing the PR. Another reason is no regression was done on the grammar change with all checks on the JDK8.
I am looking into the failing checks with the new code.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jun 15, 2016

Member

@rnveach,

Reason why the receiver PR didn't catch this is because Travis' NoExceptionTest is always 1 commit behind by testing master, instead of testing the PR

Can you point to the problem ? I do see it in travis configuration.

Member

romani commented Jun 15, 2016

@rnveach,

Reason why the receiver PR didn't catch this is because Travis' NoExceptionTest is always 1 commit behind by testing master, instead of testing the PR

Can you point to the problem ? I do see it in travis configuration.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jun 15, 2016

Member

@romani
PR = master (most of the time) commits + X number of PR commits
NoExceptionTest just runs on master (https://github.com/checkstyle/contribution/blob/master/checkstyle-tester/projects-for-travis.properties#L6), ignoring the PR files.

if we change any files in the PR not ignored by tester, NoExceptionTest doesn't pick them up until after the PR is merged.
This is why the PR #3260 is all green and the tests pass, but we are seeing master and all our subsequent PRs fail on NoExceptionTest. Had NoExceptionTest run with the PR files instead of master (or with both), it would have failed in the PR before merging.
This clear?

Member

rnveach commented Jun 15, 2016

@romani
PR = master (most of the time) commits + X number of PR commits
NoExceptionTest just runs on master (https://github.com/checkstyle/contribution/blob/master/checkstyle-tester/projects-for-travis.properties#L6), ignoring the PR files.

if we change any files in the PR not ignored by tester, NoExceptionTest doesn't pick them up until after the PR is merged.
This is why the PR #3260 is all green and the tests pass, but we are seeing master and all our subsequent PRs fail on NoExceptionTest. Had NoExceptionTest run with the PR files instead of master (or with both), it would have failed in the PR before merging.
This clear?

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Jun 15, 2016

Contributor

@romani
The test should be run with both as sometimes it is required to relaunch Travis CI node on master

Contributor

MEZk commented Jun 15, 2016

@romani
The test should be run with both as sometimes it is required to relaunch Travis CI node on master

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jun 17, 2016

Member

@MEZk , master build is green, please rebase changes on master to make sure that your changes are green by CIs.

Member

romani commented Jun 17, 2016

@MEZk , master build is green, please rebase changes on master to make sure that your changes are green by CIs.

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Jun 17, 2016

Contributor

@romani
Done

Contributor

MEZk commented Jun 17, 2016

@romani
Done

@romani romani merged commit d8e7cbd into checkstyle:master Jun 17, 2016

6 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codacy/pr Good work! The project quality is stable.
Details
codecov/project 100% (+0.00%) compared to 9dfa5a9
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/teamcity Finished TeamCity Build Checkstyle :: IDEA Inspections Pull Request : Inspections total: 0, errors: 0
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@romani romani added this to the 7.0 milestone Jun 17, 2016

@MEZk MEZk deleted the MEZk:issue#3177-rte branch Jun 18, 2016

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