Issue #14019: Kill mutation in SuppressWithNearbyTextFilter by akankshat05 · Pull Request #15941 · checkstyle/checkstyle

akankshat05 · 2024-11-18T17:58:58Z

For #14019
Removed suppressions:

  <mutation unstable="false">
    <sourceFile>SuppressWithNearbyTextFilter.java</sourceFile>
    <mutatedClass>com.puppycrawl.tools.checkstyle.filters.SuppressWithNearbyTextFilter</mutatedClass>
    <mutatedMethod>accept</mutatedMethod>
    <mutator>org.pitest.mutationtest.engine.gregor.mutators.NonVoidMethodCallMutator</mutator>
    <description>removed call to java/lang/String::equals</description>
    <lineContent>if (!cachedFileAbsolutePath.equals(eventFileTextAbsolutePath)) {</lineContent>
  </mutation>

  <mutation unstable="false">
    <sourceFile>SuppressWithNearbyTextFilter.java</sourceFile>
    <mutatedClass>com.puppycrawl.tools.checkstyle.filters.SuppressWithNearbyTextFilter</mutatedClass>
    <mutatedMethod>accept</mutatedMethod>
    <mutator>org.pitest.mutationtest.engine.gregor.mutators.RemoveConditionalMutator_EQUAL_IF</mutator>
    <description>removed conditional - replaced equality check with true</description>
    <lineContent>if (!cachedFileAbsolutePath.equals(eventFileTextAbsolutePath)) {</lineContent>
  </mutation>

Steps followed to kill mutation

Removed suppression from pitest-filters-suppressions.xml
Listed pitest profiles with groovy .ci/pitest-survival-check-xml.groovy --list using java 11 and groovy 2.4.5
Picked up pitest-filters as active profile
Generated report locally using mvn -e --no-transfer-progress -P"$PITEST_PROFILE" clean test-compile org.pitest:pitest-maven:mutationCoverage
Screenshots:

Analyzed report locally with groovy .ci/pitest-survival-check-xml.groovy "$PITEST_PROFILE".
Response:

New surviving mutation(s) found:

Source File: "SuppressWithNearbyTextFilter.java"
Class: "com.puppycrawl.tools.checkstyle.filters.SuppressWithNearbyTextFilter"
Method: "accept"
Line Contents: "if (!cachedFileAbsolutePath.equals(eventFileTextAbsolutePath)) {"
Mutator: "org.pitest.mutationtest.engine.gregor.mutators.NonVoidMethodCallMutator"
Description: "removed call to java/lang/String::equals"
Line Number: 198

Source File: "SuppressWithNearbyTextFilter.java"
Class: "com.puppycrawl.tools.checkstyle.filters.SuppressWithNearbyTextFilter"
Method: "accept"
Line Contents: "if (!cachedFileAbsolutePath.equals(eventFileTextAbsolutePath)) {"
Mutator: "org.pitest.mutationtest.engine.gregor.mutators.RemoveConditionalMutator_EQUAL_IF"
Description: "removed conditional - replaced equality check with true"
Line Number: 198

Added new test case
Generated report again
Screenshots:

Report analysis No new surviving mutation(s) found.

romani

Items

romani · 2024-11-19T16:43:44Z

+    /**
+     * Calls the filter on two real input files and verifies that the
+     * filter caches the file path only once. Ensures 'cachedFileAbsolutePath' remains
+     * unchanged and validates a single call to retrieve file text.


I understand that in web there are huge amount of examples that shows such type of testing where numerous mocks are used to hit specific code, testing just for testing, with little business values and little chance to be understood by next engineer.

Our project used this model in past, that is why you see here and there still bad examples. After many years we saw that such tests are just problems.

We are switched to BDD style testing, that is more expensive in execution time but has significantly more value and ease of maintainability.

So we need to redo this test method to use config in Input file.

Your case is not trivial, so let me guide you.
We need to create testing Filter module that is defined in same package as this Test class. This filter module will remove a file that is referenced in violation.
So some Check will do 2 violations in Input file. In config you will place two filter modules (target filter and filter that removes file). Input should be located in regular folder as all others. Before execution you will copy Input of repository to some temp file. Filters executed in order of declaration, so removal filter should be last. On second violation Input file will be removed, so loading of it will do exception if cache doesn't not happening.

Okay, I will update the test.

Look at examples of internal testing modules https://github.com/checkstyle/checkstyle/tree/master/src/test/java/com/puppycrawl/tools/checkstyle/internal/testmodules

If looks at commits involved in introduction of such modules, you can see how we migrated from unit testing to Input based https://github.com/checkstyle/checkstyle/commits/master/src/test/java/com/puppycrawl/tools/checkstyle/internal/testmodules

Latest example 2d5bfc1

Just FYI, is case you think we are crazy to not use mocks https://testing.googleblog.com/2024/02/increase-test-fidelity-by-avoiding-mocks.html

romani

Items

romani · 2024-11-26T19:52:27Z

                if (!path.contains(File.separatorChar + "grammar" + File.separatorChar)
-                        && !path.contains(File.separatorChar + "internal" + File.separatorChar)) {
+                        && !path.contains(File.separatorChar + "internal" + File.separatorChar)
+                        && !path.contains(File.separatorChar + "filters" + File.separatorChar)) {


Please revert this.
We can put internal test input to existing folder "internal".

romani · 2024-11-26T19:53:01Z

+///////////////////////////////////////////////////////////////////////////////////////////////
+
+package com.puppycrawl.tools.checkstyle.filters.utils;
+


Please put it under "internal"

romani · 2024-11-26T19:53:43Z

+import com.puppycrawl.tools.checkstyle.api.AuditEvent;
+import com.puppycrawl.tools.checkstyle.api.Filter;
+
+public final class FileRemovalFilter implements Filter {


FileRemovalAfterFirstUsageFilter

romani · 2024-11-26T19:54:20Z

+
+public final class FileRemovalFilter implements Filter {
+
+    private final AtomicInteger violationCount = new AtomicInteger(0);


boolean isUsed

romani · 2024-11-26T19:55:55Z

+        final int currentCount = violationCount.incrementAndGet();
+
+        boolean result = true;
+        if (currentCount == 2) {


If not used them delete.

romani · 2024-11-26T19:56:35Z

+                targetFilter, tempInputFile.getPath());
+
+        final AuditEvent dummyEvent1 = buildDummyAuditEvent(tempInputFile.getPath());
+        final AuditEvent dummyEvent2 = buildDummyAuditEvent(tempInputFile.getPath());


Nothing dummy.

Please use Input file.

romani

please always reply to each review item as "done" or any disagreement or comments.
it help a lot.

romani · 2024-11-29T14:50:20Z

+        final AuditEvent auditEvent1 = new AuditEvent(
+                tempInputFile.getPath(), tempInputFile.getPath(),
+                new Violation(1, null, null, null, null, Object.class, null));
+        targetFilter.accept(auditEvent1);


one more time: test method should use Input files with config inside of it.
2d5bfc1#diff-0aa5d4351017b80ba94f82da4e00e2396e2db8a811918444878635c740400263R1401

test should use verifyWithInlineConfigParser method only.

location to register test module - 2d5bfc1#diff-e7b107f6c9570fd3f500483f55b8bfb863cae9c96384f286d9a50be3ed4b725fR61

example of Input file 2d5bfc1#diff-fa96fe0c65d2dd977a14acfbd8d38cebb470c9b1fff52d19e22801b2d833120dR4

romani · 2024-12-16T16:27:25Z

@akankshat05, ping.

romani · 2025-01-03T20:25:34Z

we lost connection to contributor

zyadahmed · 2025-02-20T18:17:31Z

@romani i have question about your statment

Filters executed in order of declaration,

is that actually true in FilterSet.java because the storage of filters in private final Set<Filter> filters = new HashSet<>();
and HashSet does not guarantee order, so wouldn’t we need a List to ensure the execution order you mentioned? Is my understanding correct, or am I missing something?

romani · 2025-02-20T21:46:40Z

Looks like not:), thanks for pointing on this.
Let's keep Set to avoid easy relying on order.

zyadahmed · 2025-02-20T22:11:25Z

Looks like not:), thanks for pointing on this. Let's keep Set to avoid easy relying on order.

with set that solution will not work what about using LinkedHashSet it will not change alot but we can ensure the order

romani · 2025-02-20T22:19:31Z

Please create new PR, let's discuss it from scratch

zyadahmed · 2025-02-20T22:24:54Z

Please create new PR, let's discuss it from scratch

i am trying to solve problem with LinkedhashSet and verifyWithInlineXmlConfig to make working pr to discuss the issue but i have several problem

romani · 2025-02-20T22:29:56Z

Please create PR with what you have, and we will discuss and more clearly understand problems

akankshat05 marked this pull request as draft November 18, 2024 19:58

akankshat05 marked this pull request as ready for review November 18, 2024 20:06

romani requested changes Nov 19, 2024

View reviewed changes

akankshat05 force-pushed the suppress-with-nearby-text branch from 0986a35 to b3d5fad Compare November 26, 2024 17:45

akankshat05 requested a review from romani November 26, 2024 18:37

romani requested changes Nov 26, 2024

View reviewed changes

Issue checkstyle#14019: Kill mutation in SuppressWithNearbyTextFilter

26bfe8e

akankshat05 force-pushed the suppress-with-nearby-text branch from b3d5fad to 26bfe8e Compare November 27, 2024 14:18

akankshat05 requested a review from romani November 27, 2024 15:05

romani requested changes Nov 29, 2024

View reviewed changes

romani added the awaiting changes label Dec 17, 2024

romani mentioned this pull request Jan 3, 2025

Cover pitest survivals with tests #14019

Closed

54 tasks

romani closed this Jan 3, 2025

romani added the abandoned label Jan 3, 2025

		///////////////////////////////////////////////////////////////////////////////////////////////

		package com.puppycrawl.tools.checkstyle.filters.utils;


		public final class FileRemovalFilter implements Filter {

		private final AtomicInteger violationCount = new AtomicInteger(0);

Uh oh!

Conversation

akankshat05 commented Nov 18, 2024

Uh oh!

romani left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romani Nov 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romani left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romani left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romani commented Dec 16, 2024

Uh oh!

romani commented Jan 3, 2025

Uh oh!

zyadahmed commented Feb 20, 2025

Uh oh!

romani commented Feb 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zyadahmed commented Feb 20, 2025

Uh oh!

romani commented Feb 20, 2025

Uh oh!

zyadahmed commented Feb 20, 2025

Uh oh!

romani commented Feb 20, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

romani Nov 20, 2024 •

edited

Loading

romani commented Feb 20, 2025 •

edited

Loading