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

Two unit tests for SuppressionCommentFilterTest do not fail if CheckstyleException is not thrown #5210

Closed
MEZk opened this Issue Oct 21, 2017 · 6 comments

Comments

Projects
None yet
3 participants
@MEZk
Contributor

MEZk commented Oct 21, 2017

@Test
public void testInvalidCheckFormat() throws Exception {
final DefaultConfiguration filterConfig =
createFilterConfig(SuppressionCommentFilter.class);
filterConfig.addAttribute("checkFormat", "e[l");
try {
final String[] suppressed = CommonUtils.EMPTY_STRING_ARRAY;
verifySuppressed(filterConfig, suppressed);
}
catch (CheckstyleException ex) {
final IllegalArgumentException cause = (IllegalArgumentException) ex.getCause();
assertEquals("Invalid exception message",
"unable to parse expanded comment e[l", cause.getMessage());
}
}
@Test
public void testInvalidMessageFormat() throws Exception {
final DefaultConfiguration filterConfig =
createFilterConfig(SuppressionCommentFilter.class);
filterConfig.addAttribute("messageFormat", "e[l");
try {
final String[] suppressed = CommonUtils.EMPTY_STRING_ARRAY;
verifySuppressed(filterConfig, suppressed);
}
catch (CheckstyleException ex) {
final IllegalArgumentException cause = (IllegalArgumentException) ex.getCause();
assertEquals("Invalid exception message",
"unable to parse expanded comment e[l", cause.getMessage());
}
}

Both testInvalidCheckFormat and testInvalidMessageFormat tests will not fail if exception is not thrown.

fail() method should be invoked after verifySuppressed.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Oct 23, 2017

Member

I agree with this as I have seen this before.
We might want to think of an automated way to catch these like with a new check only for tests.

Member

rnveach commented Oct 23, 2017

I agree with this as I have seen this before.
We might want to think of an automated way to catch these like with a new check only for tests.

@rnveach rnveach added the approved label Oct 23, 2017

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Oct 23, 2017

Contributor

@rnveach
We can control that each test contains verify, fail, or assert statement.

Contributor

MEZk commented Oct 23, 2017

@rnveach
We can control that each test contains verify, fail, or assert statement.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Oct 23, 2017

Member

@MEZk Look at the snippet you gave. It does contain an assert statement in the catch. Technically it does contain a verify in the try. The problem is a try/catch needs a fail in the try. For tests there is no need to catch exceptions if they are not examined and should be passed up the chain so the main junit runner gets it.

Member

rnveach commented Oct 23, 2017

@MEZk Look at the snippet you gave. It does contain an assert statement in the catch. Technically it does contain a verify in the try. The problem is a try/catch needs a fail in the try. For tests there is no need to catch exceptions if they are not examined and should be passed up the chain so the main junit runner gets it.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Oct 26, 2017

Member

These are the 32 violations produced by the custom check at sevntu-checkstyle/sevntu.checkstyle#619 :

[ERROR] src/test/java/com/puppycrawl/tools/checkstyle/DetailNodeTreeStringPrinterTest.java:[140,9] (extension) RequireFailForTryCatchInJunit: try/catch requires a fail at the end of the try clause for junit tests.
[ERROR] src/test/java/com/puppycrawl/tools/checkstyle/DetailNodeTreeStringPrinterTest.java:[155,9] (extension) RequireFailForTryCatchInJunit: try/catch requires a fail at the end of the try clause for junit tests.
[ERROR] src/test/java/com/puppycrawl/tools/checkstyle/DetailNodeTreeStringPrinterTest.java:[170,9] (extension) RequireFailForTryCatchInJunit: try/catch requires a fail at the end of the try clause for junit tests.
[ERROR] src/test/java/com/puppycrawl/tools/checkstyle/DetailNodeTreeStringPrinterTest.java:[186,9] (extension) RequireFailForTryCatchInJunit: try/catch requires a fail at the end of the try clause for junit tests.
[ERROR] src/test/java/com/puppycrawl/tools/checkstyle/DetailNodeTreeStringPrinterTest.java:[201,9] (extension) RequireFailForTryCatchInJunit: try/catch requires a fail at the end of the try clause for junit tests.
[ERROR] src/test/java/com/puppycrawl/tools/checkstyle/ThreadModeSettingsTest.java:[68,9] (extension) RequireFailForTryCatchInJunit: try/catch requires a fail at the end of the try clause for junit tests.
[ERROR] src/test/java/com/puppycrawl/tools/checkstyle/TreeWalkerTest.java:[200,9] (extension) RequireFailForTryCatchInJunit: try/catch requires a fail at the end of the try clause for junit tests.
[ERROR] src/test/java/com/puppycrawl/tools/checkstyle/TreeWalkerTest.java:[271,9] (extension) RequireFailForTryCatchInJunit: try/catch requires a fail at the end of the try clause for junit tests.
[ERROR] src/test/java/com/puppycrawl/tools/checkstyle/TreeWalkerTest.java:[293,9] (extension) RequireFailForTryCatchInJunit: try/catch requires a fail at the end of the try clause for junit tests.
[ERROR] src/test/java/com/puppycrawl/tools/checkstyle/TreeWalkerTest.java:[329,9] (extension) RequireFailForTryCatchInJunit: try/catch requires a fail at the end of the try clause for junit tests.
[ERROR] src/test/java/com/puppycrawl/tools/checkstyle/api/AutomaticBeanTest.java:[46,9] (extension) RequireFailForTryCatchInJunit: try/catch requires a fail at the end of the try clause for junit tests.
[ERROR] src/test/java/com/puppycrawl/tools/checkstyle/api/AutomaticBeanTest.java:[62,9] (extension) RequireFailForTryCatchInJunit: try/catch requires a fail at the end of the try clause for junit tests.
[ERROR] src/test/java/com/puppycrawl/tools/checkstyle/api/DetailASTTest.java:[156,17] (extension) RequireFailForTryCatchInJunit: try/catch requires a fail at the end of the try clause for junit tests.
[ERROR] src/test/java/com/puppycrawl/tools/checkstyle/checks/NewlineAtEndOfFileCheckTest.java:[251,9] (extension) RequireFailForTryCatchInJunit: try/catch requires a fail at the end of the try clause for junit tests.
[ERROR] src/test/java/com/puppycrawl/tools/checkstyle/checks/header/HeaderCheckTest.java:[74,9] (extension) RequireFailForTryCatchInJunit: try/catch requires a fail at the end of the try clause for junit tests.
[ERROR] src/test/java/com/puppycrawl/tools/checkstyle/checks/header/HeaderCheckTest.java:[89,9] (extension) RequireFailForTryCatchInJunit: try/catch requires a fail at the end of the try clause for junit tests.
[ERROR] src/test/java/com/puppycrawl/tools/checkstyle/checks/imports/CustomImportOrderCheckTest.java:[371,9] (extension) RequireFailForTryCatchInJunit: try/catch requires a fail at the end of the try clause for junit tests.
[ERROR] src/test/java/com/puppycrawl/tools/checkstyle/checks/javadoc/AbstractTypeAwareCheckTest.java:[79,9] (extension) RequireFailForTryCatchInJunit: try/catch requires a fail at the end of the try clause for junit tests.
[ERROR] src/test/java/com/puppycrawl/tools/checkstyle/checks/javadoc/AbstractTypeAwareCheckTest.java:[160,9] (extension) RequireFailForTryCatchInJunit: try/catch requires a fail at the end of the try clause for junit tests.
[ERROR] src/test/java/com/puppycrawl/tools/checkstyle/checks/javadoc/ClassResolverTest.java:[123,9] (extension) RequireFailForTryCatchInJunit: try/catch requires a fail at the end of the try clause for junit tests.
[ERROR] src/test/java/com/puppycrawl/tools/checkstyle/checks/javadoc/ClassResolverTest.java:[175,9] (extension) RequireFailForTryCatchInJunit: try/catch requires a fail at the end of the try clause for junit tests.
[ERROR] src/test/java/com/puppycrawl/tools/checkstyle/filters/SuppressWithNearbyCommentFilterTest.java:[246,9] (extension) RequireFailForTryCatchInJunit: try/catch requires a fail at the end of the try clause for junit tests.
[ERROR] src/test/java/com/puppycrawl/tools/checkstyle/filters/SuppressWithNearbyCommentFilterTest.java:[282,9] (extension) RequireFailForTryCatchInJunit: try/catch requires a fail at the end of the try clause for junit tests.
[ERROR] src/test/java/com/puppycrawl/tools/checkstyle/filters/SuppressionCommentFilterTest.java:[264,9] (extension) RequireFailForTryCatchInJunit: try/catch requires a fail at the end of the try clause for junit tests.
[ERROR] src/test/java/com/puppycrawl/tools/checkstyle/filters/SuppressionCommentFilterTest.java:[281,9] (extension) RequireFailForTryCatchInJunit: try/catch requires a fail at the end of the try clause for junit tests.
[ERROR] src/test/java/com/puppycrawl/tools/checkstyle/filters/SuppressionsLoaderTest.java:[142,9] (extension) RequireFailForTryCatchInJunit: try/catch requires a fail at the end of the try clause for junit tests.
[ERROR] src/test/java/com/puppycrawl/tools/checkstyle/filters/SuppressionsLoaderTest.java:[159,9] (extension) RequireFailForTryCatchInJunit: try/catch requires a fail at the end of the try clause for junit tests.
[ERROR] src/test/java/com/puppycrawl/tools/checkstyle/filters/SuppressionsLoaderTest.java:[176,9] (extension) RequireFailForTryCatchInJunit: try/catch requires a fail at the end of the try clause for junit tests.
[ERROR] src/test/java/com/puppycrawl/tools/checkstyle/filters/SuppressionsLoaderTest.java:[262,9] (extension) RequireFailForTryCatchInJunit: try/catch requires a fail at the end of the try clause for junit tests.
[ERROR] src/test/java/com/puppycrawl/tools/checkstyle/filters/SuppressionsLoaderTest.java:[283,9] (extension) RequireFailForTryCatchInJunit: try/catch requires a fail at the end of the try clause for junit tests.
[ERROR] src/test/java/com/puppycrawl/tools/checkstyle/filters/XpathFilterTest.java:[240,9] (extension) RequireFailForTryCatchInJunit: try/catch requires a fail at the end of the try clause for junit tests.
[ERROR] src/test/java/com/puppycrawl/tools/checkstyle/utils/AnnotationUtilityTest.java:[38,9] (extension) RequireFailForTryCatchInJunit: try/catch requires a fail at the end of the try clause for junit tests.
Member

rnveach commented Oct 26, 2017

These are the 32 violations produced by the custom check at sevntu-checkstyle/sevntu.checkstyle#619 :

[ERROR] src/test/java/com/puppycrawl/tools/checkstyle/DetailNodeTreeStringPrinterTest.java:[140,9] (extension) RequireFailForTryCatchInJunit: try/catch requires a fail at the end of the try clause for junit tests.
[ERROR] src/test/java/com/puppycrawl/tools/checkstyle/DetailNodeTreeStringPrinterTest.java:[155,9] (extension) RequireFailForTryCatchInJunit: try/catch requires a fail at the end of the try clause for junit tests.
[ERROR] src/test/java/com/puppycrawl/tools/checkstyle/DetailNodeTreeStringPrinterTest.java:[170,9] (extension) RequireFailForTryCatchInJunit: try/catch requires a fail at the end of the try clause for junit tests.
[ERROR] src/test/java/com/puppycrawl/tools/checkstyle/DetailNodeTreeStringPrinterTest.java:[186,9] (extension) RequireFailForTryCatchInJunit: try/catch requires a fail at the end of the try clause for junit tests.
[ERROR] src/test/java/com/puppycrawl/tools/checkstyle/DetailNodeTreeStringPrinterTest.java:[201,9] (extension) RequireFailForTryCatchInJunit: try/catch requires a fail at the end of the try clause for junit tests.
[ERROR] src/test/java/com/puppycrawl/tools/checkstyle/ThreadModeSettingsTest.java:[68,9] (extension) RequireFailForTryCatchInJunit: try/catch requires a fail at the end of the try clause for junit tests.
[ERROR] src/test/java/com/puppycrawl/tools/checkstyle/TreeWalkerTest.java:[200,9] (extension) RequireFailForTryCatchInJunit: try/catch requires a fail at the end of the try clause for junit tests.
[ERROR] src/test/java/com/puppycrawl/tools/checkstyle/TreeWalkerTest.java:[271,9] (extension) RequireFailForTryCatchInJunit: try/catch requires a fail at the end of the try clause for junit tests.
[ERROR] src/test/java/com/puppycrawl/tools/checkstyle/TreeWalkerTest.java:[293,9] (extension) RequireFailForTryCatchInJunit: try/catch requires a fail at the end of the try clause for junit tests.
[ERROR] src/test/java/com/puppycrawl/tools/checkstyle/TreeWalkerTest.java:[329,9] (extension) RequireFailForTryCatchInJunit: try/catch requires a fail at the end of the try clause for junit tests.
[ERROR] src/test/java/com/puppycrawl/tools/checkstyle/api/AutomaticBeanTest.java:[46,9] (extension) RequireFailForTryCatchInJunit: try/catch requires a fail at the end of the try clause for junit tests.
[ERROR] src/test/java/com/puppycrawl/tools/checkstyle/api/AutomaticBeanTest.java:[62,9] (extension) RequireFailForTryCatchInJunit: try/catch requires a fail at the end of the try clause for junit tests.
[ERROR] src/test/java/com/puppycrawl/tools/checkstyle/api/DetailASTTest.java:[156,17] (extension) RequireFailForTryCatchInJunit: try/catch requires a fail at the end of the try clause for junit tests.
[ERROR] src/test/java/com/puppycrawl/tools/checkstyle/checks/NewlineAtEndOfFileCheckTest.java:[251,9] (extension) RequireFailForTryCatchInJunit: try/catch requires a fail at the end of the try clause for junit tests.
[ERROR] src/test/java/com/puppycrawl/tools/checkstyle/checks/header/HeaderCheckTest.java:[74,9] (extension) RequireFailForTryCatchInJunit: try/catch requires a fail at the end of the try clause for junit tests.
[ERROR] src/test/java/com/puppycrawl/tools/checkstyle/checks/header/HeaderCheckTest.java:[89,9] (extension) RequireFailForTryCatchInJunit: try/catch requires a fail at the end of the try clause for junit tests.
[ERROR] src/test/java/com/puppycrawl/tools/checkstyle/checks/imports/CustomImportOrderCheckTest.java:[371,9] (extension) RequireFailForTryCatchInJunit: try/catch requires a fail at the end of the try clause for junit tests.
[ERROR] src/test/java/com/puppycrawl/tools/checkstyle/checks/javadoc/AbstractTypeAwareCheckTest.java:[79,9] (extension) RequireFailForTryCatchInJunit: try/catch requires a fail at the end of the try clause for junit tests.
[ERROR] src/test/java/com/puppycrawl/tools/checkstyle/checks/javadoc/AbstractTypeAwareCheckTest.java:[160,9] (extension) RequireFailForTryCatchInJunit: try/catch requires a fail at the end of the try clause for junit tests.
[ERROR] src/test/java/com/puppycrawl/tools/checkstyle/checks/javadoc/ClassResolverTest.java:[123,9] (extension) RequireFailForTryCatchInJunit: try/catch requires a fail at the end of the try clause for junit tests.
[ERROR] src/test/java/com/puppycrawl/tools/checkstyle/checks/javadoc/ClassResolverTest.java:[175,9] (extension) RequireFailForTryCatchInJunit: try/catch requires a fail at the end of the try clause for junit tests.
[ERROR] src/test/java/com/puppycrawl/tools/checkstyle/filters/SuppressWithNearbyCommentFilterTest.java:[246,9] (extension) RequireFailForTryCatchInJunit: try/catch requires a fail at the end of the try clause for junit tests.
[ERROR] src/test/java/com/puppycrawl/tools/checkstyle/filters/SuppressWithNearbyCommentFilterTest.java:[282,9] (extension) RequireFailForTryCatchInJunit: try/catch requires a fail at the end of the try clause for junit tests.
[ERROR] src/test/java/com/puppycrawl/tools/checkstyle/filters/SuppressionCommentFilterTest.java:[264,9] (extension) RequireFailForTryCatchInJunit: try/catch requires a fail at the end of the try clause for junit tests.
[ERROR] src/test/java/com/puppycrawl/tools/checkstyle/filters/SuppressionCommentFilterTest.java:[281,9] (extension) RequireFailForTryCatchInJunit: try/catch requires a fail at the end of the try clause for junit tests.
[ERROR] src/test/java/com/puppycrawl/tools/checkstyle/filters/SuppressionsLoaderTest.java:[142,9] (extension) RequireFailForTryCatchInJunit: try/catch requires a fail at the end of the try clause for junit tests.
[ERROR] src/test/java/com/puppycrawl/tools/checkstyle/filters/SuppressionsLoaderTest.java:[159,9] (extension) RequireFailForTryCatchInJunit: try/catch requires a fail at the end of the try clause for junit tests.
[ERROR] src/test/java/com/puppycrawl/tools/checkstyle/filters/SuppressionsLoaderTest.java:[176,9] (extension) RequireFailForTryCatchInJunit: try/catch requires a fail at the end of the try clause for junit tests.
[ERROR] src/test/java/com/puppycrawl/tools/checkstyle/filters/SuppressionsLoaderTest.java:[262,9] (extension) RequireFailForTryCatchInJunit: try/catch requires a fail at the end of the try clause for junit tests.
[ERROR] src/test/java/com/puppycrawl/tools/checkstyle/filters/SuppressionsLoaderTest.java:[283,9] (extension) RequireFailForTryCatchInJunit: try/catch requires a fail at the end of the try clause for junit tests.
[ERROR] src/test/java/com/puppycrawl/tools/checkstyle/filters/XpathFilterTest.java:[240,9] (extension) RequireFailForTryCatchInJunit: try/catch requires a fail at the end of the try clause for junit tests.
[ERROR] src/test/java/com/puppycrawl/tools/checkstyle/utils/AnnotationUtilityTest.java:[38,9] (extension) RequireFailForTryCatchInJunit: try/catch requires a fail at the end of the try clause for junit tests.
@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Oct 26, 2017

Member

[ERROR] src/test/java/com/puppycrawl/tools/checkstyle/api/DetailASTTest.java:[156,17] (extension) RequireFailForTryCatchInJunit: try/catch requires a fail at the end of the try clause for junit tests.

ast -> {
try {
Whitebox.invokeMethod(child, "setParent", ast);
}
// -@cs[IllegalCatch] Cannot avoid catching it.
catch (Exception exception) {
throw new IllegalStateException(exception);
}
}

This is a false positive because it is inside a lambda and the method throws a checked exception.
If you don't have the catch, it won't compile with the error Unhandled exception type Exception because lambdas don't declare any exceptions thrown. You must catch and throw it as an unchecked exception in this case.
I will update the check to ignore this situation.

Member

rnveach commented Oct 26, 2017

[ERROR] src/test/java/com/puppycrawl/tools/checkstyle/api/DetailASTTest.java:[156,17] (extension) RequireFailForTryCatchInJunit: try/catch requires a fail at the end of the try clause for junit tests.

ast -> {
try {
Whitebox.invokeMethod(child, "setParent", ast);
}
// -@cs[IllegalCatch] Cannot avoid catching it.
catch (Exception exception) {
throw new IllegalStateException(exception);
}
}

This is a false positive because it is inside a lambda and the method throws a checked exception.
If you don't have the catch, it won't compile with the error Unhandled exception type Exception because lambdas don't declare any exceptions thrown. You must catch and throw it as an unchecked exception in this case.
I will update the check to ignore this situation.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Oct 27, 2017

Member

fix is merged.

Member

romani commented Oct 27, 2017

fix is merged.

@romani romani closed this Oct 27, 2017

@rnveach rnveach added this to the 8.4 milestone Oct 27, 2017

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