Issue #9142: finalize #11151

rnveach · 2022-01-03T05:53:04Z

Doing a search on org.junit.jupiter.api.Assertions only provides the following results:

M:\checkstyleWorkspace\checkstyle\src\test\java\com\puppycrawl\tools\checkstyle\ant\CheckstyleAntTaskTest.java (1 hit)
	Line 23: import static org.junit.jupiter.api.Assertions.assertThrows;
  M:\checkstyleWorkspace\checkstyle\src\test\java\com\puppycrawl\tools\checkstyle\checks\header\HeaderCheckTest.java (1 hit)
	Line 25: import static org.junit.jupiter.api.Assertions.assertThrows;
  M:\checkstyleWorkspace\checkstyle\src\test\java\com\puppycrawl\tools\checkstyle\checks\imports\ImportControlLoaderTest.java (1 hit)
	Line 23: import static org.junit.jupiter.api.Assertions.assertThrows;
  M:\checkstyleWorkspace\checkstyle\src\test\java\com\puppycrawl\tools\checkstyle\checks\javadoc\AbstractJavadocCheckTest.java (1 hit)
	Line 30: import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
  M:\checkstyleWorkspace\checkstyle\src\test\java\com\puppycrawl\tools\checkstyle\checks\regexp\RegexpOnFilenameCheckTest.java (1 hit)
	Line 25: import static org.junit.jupiter.api.Assertions.assertThrows;
  M:\checkstyleWorkspace\checkstyle\src\test\java\com\puppycrawl\tools\checkstyle\ConfigurationLoaderTest.java (1 hit)
	Line 23: import static org.junit.jupiter.api.Assertions.assertThrows;
  M:\checkstyleWorkspace\checkstyle\src\test\java\com\puppycrawl\tools\checkstyle\DefaultLoggerTest.java (1 hit)
	Line 23: import static org.junit.jupiter.api.Assertions.assertThrows;
  M:\checkstyleWorkspace\checkstyle\src\test\java\com\puppycrawl\tools\checkstyle\PropertyCacheFileTest.java (1 hit)
	Line 23: import static org.junit.jupiter.api.Assertions.assertThrows;
  M:\checkstyleWorkspace\checkstyle\src\test\java\com\puppycrawl\tools\checkstyle\utils\ChainedPropertyUtilTest.java (1 hit)
	Line 24: import static org.junit.jupiter.api.Assertions.assertThrows;
  M:\checkstyleWorkspace\checkstyle\src\test\java\com\puppycrawl\tools\checkstyle\utils\CommonUtilTest.java (1 hit)
	Line 25: import static org.junit.jupiter.api.Assertions.assertThrows;
  M:\checkstyleWorkspace\checkstyle\src\test\java\com\puppycrawl\tools\checkstyle\xpath\AttributeNodeTest.java (1 hit)
	Line 23: import static org.junit.jupiter.api.Assertions.assertThrows;

pbludov · 2022-01-03T06:33:07Z

src/test/java/com/puppycrawl/tools/checkstyle/filters/SuppressionsLoaderTest.java

@@ -75,7 +73,9 @@ public void testLoadFromUrl() throws Exception {
            }
        }

-        assumeTrue(actualFilterSet != null, "No Internet connection.");


assumeXXX should not be replaced with an assert.
This test requires an internet connection, which may not be available.
This can be replaced with if, but an assume method is better, since it will output the reason to the tests log.

I am not understanding why it can't be replaced. Can you provide clarification on why we can't use assert here?

Why aren't the 2 equal?

assumeTrue(variable, "Text");
assertWithMessage("Text").that(variable).isTrue();

false conditions in assume methods will not break the build. It simply reports that the test is ignored due to "Text".
false condition in assert methods will break the build.

pbludov · 2022-01-03T06:33:26Z

src/test/java/com/puppycrawl/tools/checkstyle/DefaultLoggerTest.java

@@ -223,7 +222,9 @@ public void testFinishLocalSetup() {
    @Test
    public void testLanguageIsValid() {
        final String language = DEFAULT_LOCALE.getLanguage();
-        assumeFalse(language.isEmpty(), "Locale not set");
+        assertWithMessage("Locale not set")


pbludov · 2022-01-03T06:33:32Z

src/test/java/com/puppycrawl/tools/checkstyle/DefaultLoggerTest.java

@@ -235,7 +236,9 @@ public void testLanguageIsValid() {
    @Test
    public void testCountryIsValid() {
        final String country = DEFAULT_LOCALE.getCountry();
-        assumeFalse(country.isEmpty(), "Locale not set");
+        assertWithMessage("Locale not set")


pbludov · 2022-01-03T06:33:41Z

src/test/java/com/puppycrawl/tools/checkstyle/DefaultLoggerTest.java

@@ -248,8 +251,9 @@ public void testCountryIsValid() {
    @Test
    public void testLocaleIsSupported() throws Exception {
        final String language = DEFAULT_LOCALE.getLanguage();
-        assumeFalse(language.isEmpty() || Locale.ENGLISH.getLanguage().equals(language),
-                "Custom locale not set");
+        assertWithMessage("Custom locale not set")


config/import-control-test.xml

pbludov · 2022-01-04T07:15:34Z

src/it/java/org/checkstyle/base/AbstractItModuleTestSupport.java

@@ -251,22 +250,28 @@ protected final void verify(Checker checker,
            for (int i = 0; i < expected.length; i++) {
                final String expectedResult = messageFileName + ":" + expected[i];
                final String actual = lnr.readLine();
-                assertEquals(expectedResult, actual, "error message " + i);
+                assertWithMessage("error message " + i)


It is better to use formatted message here:

assertWithMessage("error message %s", i)

What is better about it? The readability or is there some performance?

Both assertWithMessage("error message %s", i) and `assertWithMessage("error message " + i) have the same readability and performance.

The difference is that "error message " + i is actually String.concat("error message "), i.toString()). And this .toString() method can (in theory) have side effects. For example, it may throw an NPE, which will cause the test to fail for no clear reason.

This is absolutely inapplicable to any primitive type such as int, but this is a style issue. If we all agree to use this style, it will avoid possible problems in the future.

What about #11151 (comment) ?

pbludov · 2022-01-04T07:16:21Z

src/it/java/org/checkstyle/base/AbstractItModuleTestSupport.java

-                                || theWarnings.remove((Integer) lineNumber),
-                        "input file is expected to have a warning comment on line number "
-                                        + lineNumber);
+                assertWithMessage("input file is expected to have a warning comment on line number "


pbludov

One minor change please

pbludov · 2022-01-04T07:16:32Z

src/it/java/org/checkstyle/base/AbstractItModuleTestSupport.java

-            assertEquals(expected.length,
-                    errs, "unexpected output: " + lnr.readLine());
-            assertEquals(0, theWarnings.size(), "unexpected warnings " + theWarnings);
+            assertWithMessage("unexpected output: " + lnr.readLine())


pbludov · 2022-01-04T07:17:04Z

src/it/java/org/checkstyle/base/AbstractItModuleTestSupport.java

+            assertWithMessage("unexpected output: " + lnr.readLine())
+                .that(errs)
+                .isEqualTo(expected.length);
+            assertWithMessage("unexpected warnings " + theWarnings)


pbludov · 2022-01-04T07:17:22Z

src/test/java/com/puppycrawl/tools/checkstyle/utils/CheckUtilTest.java

@@ -154,7 +153,8 @@ public void testGetAccessModifierFromModifiersTokenWrongTokenType() {

        try {
            CheckUtil.getAccessModifierFromModifiersToken(modifiers);
-            fail(IllegalArgumentException.class.getSimpleName() + " was expected.");
+            assertWithMessage(IllegalArgumentException.class.getSimpleName() + " was expected.")


pbludov · 2022-01-04T07:17:49Z

src/test/java/com/puppycrawl/tools/checkstyle/utils/CheckUtilTest.java

-        if (!node.isPresent()) {
-            fail("Cannot find node of specified type: " + type);
-        }
+        assertWithMessage("Cannot find node of specified type: " + type)


rnveach · 2022-01-09T00:12:11Z

@pbludov all done

rnveach force-pushed the issue_9142_25 branch from 2f9dd0d to cce32c4 Compare January 3, 2022 06:05

pbludov suggested changes Jan 3, 2022

View reviewed changes

rnveach force-pushed the issue_9142_25 branch from cce32c4 to dc40b25 Compare January 3, 2022 06:44

romani reviewed Jan 3, 2022

View reviewed changes

config/import-control-test.xml Show resolved Hide resolved

rnveach force-pushed the issue_9142_25 branch from dc40b25 to 3aaad1f Compare January 3, 2022 17:16

romani approved these changes Jan 4, 2022

View reviewed changes

romani assigned pbludov Jan 4, 2022

pbludov reviewed Jan 4, 2022

View reviewed changes

pbludov suggested changes Jan 4, 2022

View reviewed changes

Issue checkstyle#9142: finalize

e6b3cb0

rnveach force-pushed the issue_9142_25 branch from 3aaad1f to e6b3cb0 Compare January 9, 2022 00:11

pbludov approved these changes Jan 9, 2022

View reviewed changes

pbludov merged commit a1c3c35 into checkstyle:master Jan 9, 2022

rnveach deleted the issue_9142_25 branch January 9, 2022 02:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #9142: finalize #11151

Issue #9142: finalize #11151

rnveach commented Jan 3, 2022 •

edited

pbludov Jan 3, 2022

rnveach Jan 3, 2022 •

edited

pbludov Jan 6, 2022

pbludov Jan 3, 2022

pbludov Jan 3, 2022

pbludov Jan 3, 2022

pbludov Jan 4, 2022

rnveach Jan 4, 2022

pbludov Jan 6, 2022 •

edited

rnveach Jan 6, 2022

pbludov Jan 4, 2022

pbludov left a comment

pbludov Jan 4, 2022

pbludov Jan 4, 2022

pbludov Jan 4, 2022

pbludov Jan 4, 2022

rnveach commented Jan 9, 2022

Issue #9142: finalize #11151

Issue #9142: finalize #11151

Conversation

rnveach commented Jan 3, 2022 • edited

Choose a reason for hiding this comment

rnveach Jan 3, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pbludov Jan 6, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pbludov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnveach commented Jan 9, 2022

rnveach commented Jan 3, 2022 •

edited

rnveach Jan 3, 2022 •

edited

pbludov Jan 6, 2022 •

edited