Issue #12586: Optimize testing methods to run check one time over a file #14588

mahfouz72 · 2024-03-02T15:48:29Z

First step for #12586:

this is not the final fix I just want to make sure that I am on the right direction also I faced some errors want to discuss them. IMO once this optimization is completed and reaches its final form, it will provide significant benefits. specifically aims to improve the process of mvn test and mvn clean verify.

mahfouz72 · 2024-03-02T15:57:57Z

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

-        verifyViolations(parsedConfig, filePath, testInputConfiguration.getViolations());
-        verify(parsedConfig, filePath, expected);
+        final List<String> actualViolations = getActualViolationsForFile(parsedConfig, filePath);
+        verifyViolations(filePath, testInputConfiguration.getViolations(), actualViolations);
+
+        assertWithMessage("Violations for %s differ.", filePath)
+            .that(actualViolations)
+            .containsExactlyElementsIn(expected);


Instead of running a check twice. once to verify the violation in verifyViolations() against testInputConfigurationViolation that got parsed from the input file. and another one with verify() to verify violations against the expected array.

I extracted running the check out to getActualViolationsForFile() once and then used these actualViolations to verify them against the testInputConfigurationViolation with verifyViolations() and then verify them with the expected array with assertion

mahfouz72 · 2024-03-02T16:01:11Z

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

+    private void verifyViolations(String file,
+                                  List<TestInputViolation> testInputViolations,
+                                  List<String> actualViolations) {
+        final List<Integer> actualViolationLines = actualViolations.stream()
+                .map(violation -> violation.substring(0, violation.indexOf(':')))
+                .map(Integer::valueOf)
+                .collect(Collectors.toUnmodifiableList());
+        final List<Integer> expectedViolationLines = testInputViolations.stream()
+                .map(TestInputViolation::getLineNo)
+                .collect(Collectors.toUnmodifiableList());
+        assertWithMessage("Violation lines for %s differ.", file)
+                .that(actualViolationLines)
+                .isEqualTo(expectedViolationLines);
+        for (int index = 0; index < actualViolations.size(); index++) {
+            assertWithMessage("Actual and expected violations differ.")
+                    .that(actualViolations.get(index))
+                    .matches(testInputViolations.get(index).toRegex());
+        }
+    }


this method is not supposed to be here at the final fix . I just overloaded the normal verifyViolation method to test my new changes. but the other remaining test methods still use the old verifyViolation So I just did that to make the old tests method work but at the end of the fix it should be only one verifyViolation method

mahfouz72 · 2024-03-02T16:29:53Z

src/test/java/com/puppycrawl/tools/checkstyle/checks/javadoc/WriteTagCheckTest.java

@@ -188,6 +188,7 @@ public void testWriteTagRecordsAndCompactCtors() throws Exception {
            getNonCompilablePath("InputWriteTagRecordsAndCompactCtors.java"), expected);
    }

+
    @Override
    protected void verify(Checker checker,
                          File[] processedFiles,


some of these tests here are not passing because it was using an overridden verify method not the normal one in the abstractTestSupport. any guidance on how to solve this will be helpful

getActualViolationForFile that I used above is returning empty array and not detecting the violation in input files of WriteTagCheckTest

I am not an expert on this part of the code, but you may find it useful to look back through git blame to see why these tests are different than others, and investigate the possibility of migrating the tests that deviate to the normal verify methods. In fact, this may even be some prework that you could do before solving this issue.

@nrmancuso I looked in git blame didn't help much. but I think that the reason this test is different is that the default severity for this check is info so the getViolationForFile() is returning an empty array ( I am not sure please confirm that) . so this verify uses the output stream to test with it not the actual violation returned from the file

is migrating back to use verify instead of verifyWithInlineConfigParser for this test only will be a valid solution

mahfouz72 · 2024-03-04T23:31:31Z

tried to override verifyWithInlineConfigParser() to use the old way of testing but got error as usage of verify is forbbeden

forbidden method invocation: com.puppycrawl.tools.checkstyle.AbstractModuleTestSupport#verify(com.puppycrawl.tools.checkstyle.api.Configuration, java.lang.String, java.lang.String[]) [Use inline config parser instead.]
[ERROR]   in com.puppycrawl.tools.checkstyle.checks.javadoc.WriteTagCheckTest

romani · 2024-03-05T02:53:15Z

We just need to reuse existing method in final fix. During transition to final state we can suppress such violation

nrmancuso · 2024-03-15T17:32:58Z

@mahfouz72 where are we at here? Can we assist?

mahfouz72 · 2024-03-16T03:26:54Z

@nrmancuso I just need time to get back to this PR to figure out why there are surviving mutations and how to kill them. it would be helpful If you could review my changes and tell me if there is something that is not correct and is the reason for all those survivals

romani · 2024-03-16T04:41:54Z

This means that we actually used double execution to kill mutation :).
We can execute some tests twice to kill them again :).

mahfouz72 · 2024-03-16T13:12:09Z

@romani I optimized one test only and still got survivals I don't why we still have tests that execute check twice

mahfouz72 · 2024-03-17T17:43:54Z

@romani do you have idea why this happened? .I debugged and we didn't change the logic of testing at all the logic still the same we now just run the checker one time why we got this survivals?

romani · 2024-05-08T04:22:24Z

Let's be not fanatic about to convert all at ones, keep treewalker on double run method for now. In next step/PR we will reduce double run to few tests methods, and may be we will keep such few test methods on double run.

Create verify method that kept old behavior of double run, and use it to satisfy pitest.

mahfouz72 · 2024-05-08T22:52:51Z

@romani Please review I kept tree walker tests on double run to satisfy pitest
I also kept writeTagCheckTest on double run due to #14588 (comment)
In the next PR, we can fix other verify methods

romani · 2024-05-09T01:58:32Z

Code looks good.

Please fix https://app.circleci.com/pipelines/github/checkstyle/checkstyle/25485/workflows/7c31ed43-d542-45a6-983b-307fb0c38674/jobs/588676/artifacts

mahfouz72 · 2024-05-09T09:40:08Z

@romani done can you please explain to me why we should make the method static and no warnings on other methods?
6034763

romani · 2024-05-09T12:52:22Z

static modifier is kind of synonym to "state less" method, so does not depends on class fields, so can be used anywhere and does no side effects and simply return you a value based on parameters.

romani

ok to merge.

in next PRs we will try to minimize usage of double execution in other test methods.

nrmancuso

Items:

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

…e over a file

mahfouz72 force-pushed the issue-12586-optimizetests branch from af701d2 to 854812e Compare March 2, 2024 15:54

mahfouz72 commented Mar 2, 2024

View reviewed changes

mahfouz72 changed the title ~~Issue #12586: Optimize testing methods to run check on time on a file~~ Issue #12586: Optimize testing methods to run check one time over a file Mar 2, 2024

mahfouz72 force-pushed the issue-12586-optimizetests branch from 854812e to 19586e9 Compare March 4, 2024 23:26

mahfouz72 force-pushed the issue-12586-optimizetests branch 4 times, most recently from 79f130e to cb3f403 Compare March 5, 2024 10:56

mahfouz72 force-pushed the issue-12586-optimizetests branch 2 times, most recently from 2859568 to 752d20b Compare March 16, 2024 12:36

rnveach added the high demand label May 8, 2024

mahfouz72 force-pushed the issue-12586-optimizetests branch 5 times, most recently from 41ee4a3 to d26b0d9 Compare May 8, 2024 18:38

mahfouz72 force-pushed the issue-12586-optimizetests branch from 6034763 to ca7f50f Compare May 9, 2024 09:10

romani approved these changes May 9, 2024

View reviewed changes

romani assigned nrmancuso May 9, 2024

nrmancuso requested changes May 10, 2024

View reviewed changes

src/test/java/com/puppycrawl/tools/checkstyle/AbstractModuleTestSupport.java Show resolved Hide resolved

src/test/java/com/puppycrawl/tools/checkstyle/AbstractModuleTestSupport.java Outdated Show resolved Hide resolved

Issue checkstyle#12586: Optimize testing methods to run check one tim…

a607848

…e over a file

mahfouz72 force-pushed the issue-12586-optimizetests branch from ca7f50f to a607848 Compare May 10, 2024 09:07

nrmancuso approved these changes May 10, 2024

View reviewed changes

nrmancuso merged commit c34d756 into checkstyle:master May 10, 2024
114 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #12586: Optimize testing methods to run check one time over a file #14588

Issue #12586: Optimize testing methods to run check one time over a file #14588

mahfouz72 commented Mar 2, 2024 •

edited by romani

mahfouz72 Mar 2, 2024

mahfouz72 Mar 2, 2024 •

edited

mahfouz72 Mar 2, 2024 •

edited

nrmancuso Mar 3, 2024

mahfouz72 Mar 4, 2024 •

edited

mahfouz72 Mar 4, 2024 •

edited

mahfouz72 commented Mar 4, 2024

romani commented Mar 5, 2024

nrmancuso commented Mar 15, 2024

mahfouz72 commented Mar 16, 2024 •

edited

romani commented Mar 16, 2024

mahfouz72 commented Mar 16, 2024

mahfouz72 commented Mar 17, 2024

romani commented May 8, 2024

mahfouz72 commented May 8, 2024

romani commented May 9, 2024 •

edited

mahfouz72 commented May 9, 2024

romani commented May 9, 2024

romani left a comment

nrmancuso left a comment

Issue #12586: Optimize testing methods to run check one time over a file #14588

Issue #12586: Optimize testing methods to run check one time over a file #14588

Conversation

mahfouz72 commented Mar 2, 2024 • edited by romani

mahfouz72 Mar 2, 2024

Choose a reason for hiding this comment

mahfouz72 Mar 2, 2024 • edited

Choose a reason for hiding this comment

mahfouz72 Mar 2, 2024 • edited

Choose a reason for hiding this comment

nrmancuso Mar 3, 2024

Choose a reason for hiding this comment

mahfouz72 Mar 4, 2024 • edited

Choose a reason for hiding this comment

mahfouz72 Mar 4, 2024 • edited

Choose a reason for hiding this comment

mahfouz72 commented Mar 4, 2024

romani commented Mar 5, 2024

nrmancuso commented Mar 15, 2024

mahfouz72 commented Mar 16, 2024 • edited

romani commented Mar 16, 2024

mahfouz72 commented Mar 16, 2024

mahfouz72 commented Mar 17, 2024

romani commented May 8, 2024

mahfouz72 commented May 8, 2024

romani commented May 9, 2024 • edited

mahfouz72 commented May 9, 2024

romani commented May 9, 2024

romani left a comment

Choose a reason for hiding this comment

nrmancuso left a comment

Choose a reason for hiding this comment

mahfouz72 commented Mar 2, 2024 •

edited by romani

mahfouz72 Mar 2, 2024 •

edited

mahfouz72 Mar 2, 2024 •

edited

mahfouz72 Mar 4, 2024 •

edited

mahfouz72 Mar 4, 2024 •

edited

mahfouz72 commented Mar 16, 2024 •

edited

romani commented May 9, 2024 •

edited