Remove ArchUnit FreezingArchRule as it unstable to skip line numbers and any other extra changes

[romani]

Based on discussion at #14009
It becomes clear that validation of cycles by ArchUnit FreezingArchRule has problems due to fact that base file that we store in our git repo contains line numbers.
Example:

checkstyle/config/archunit-store/slices-should-be-free-of-cycles-suppressions

Line 10 in 76a65e1

- Method <com.puppycrawl.tools.checkstyle.api.FileContents.lineIsBlank(int)> calls method <com.puppycrawl.tools.checkstyle.utils.CommonUtil.isBlank(java.lang.String)> in (FileContents.java:234)\

and almost each change in files that referenced in this suppression, results in no problems in ArchUnit validation as it skipped by design.
According to the docs, it should be:

On the first run all violations of that rule will be stored as the current state. On consecutive runs only new violations will be reported. By default FreezingArchRule will ignore line numbers, i.e. if a violation is just shifted to a different line, it will still count as previously recorded and will not be reported.

BUT, as we store this file in git, this file is changing, and our CI detects not committed changes and failing.

---

We need to update execution of test method that triggers validation and update of this file to have post validation update on file to remove or substitute to dummy 0 value for line number.

If approach above is not going to help, we need to disable this validation as we do not know how to freeze old problems and only update this file if we agree to extend our cycles. We should not update this file at any other case.

Referenced PR is example how to activate back validation and play with extra updates to such file.

[romani]

https://github.com/checkstyle/checkstyle/pull/14009/files#diff-a976ae03b947162410f0c24b8a0892a6b4410b110c297dfee26878446137c367R6

This diff is good example that file is just snapshot of our current dependencies of packages. So not only changes in line numbers, in signatures of methods. BUT also new line might appear.

So I propose to:
In test method, before arch execution, copy suppression file to .ci-temp folder
Let ArchUnit use file from temp folder.
It will update file in that folder, it test is not failing, nobody will care that such file is changed.
If test is failing, and we agree to extend our suppression with extra cycle, we will instruct contributor to copy file to config folder and make change to be part of commit.

[nrmancuso]

@romani doesn't only one test in https://github.com/checkstyle/checkstyle/pull/14009/files#diff-a976ae03b947162410f0c24b8a0892a6b4410b110c297dfee26878446137c367R6 depend on a file being checked in? Can we enable the other tests?

So I propose to...

This really doesn't address my main concern from #14009 (comment); I feel that having a unit test modify some file that is checked in is confusing for contributors; manual copying of files only exacerbates this issue.

Is it possible for us to simply have some number of violations that lives in the test itself (not an external file), and we just make sure that this number never increases?

[romani]

Can we enable the other tests?

yes, I thought that all of them use it, my bad.

I feel that having a unit test modify some file that is checked in is confusing for contributors; manual copying of files only exacerbates this issue.

Confusion of contributors happening by any violation or CI failure. As we move file out or git monitoring, it will not be a problem as we suppose to fail only if new cycle is introduced. But yes, if new cycle appeared, it will not easy to resolve for contributors and they will need help of maintainers. If cycle is not required, we will show how to fix code to avoid it. If cycle is not avoidable, maintainers will guide how to update this file.

manual copying of files only exacerbates this issue.

goal it archive a state when it is required in very rare cases when new cycle is approved, unfortunate but approved. In all other cases dependency files will stay as is. It will be stale, very outdated but arch unit will update it and I hope will fail only if new cycle is created.

If this is not going to work. We need to disable/remove it completely as it is not usable/practical validation.

[nrmancuso]

@romani what about proposal from above:

Is it possible for us to simply have some number of violations that lives in the test itself (not an external file), and we just make sure that this number never increases?

We can just check how long the generated file is (or maybe not even generate a file) and keep this number of violations in the test only. This way we make sure that we don’t add to violations until we are ready to start breaking cycles.

[romani]

I have problem to reproduce any violation by this test :)

$ git diff
diff --git a/src/main/java/com/puppycrawl/tools/checkstyle/ant/CheckstyleAntTask.java b/src/main/java/com/puppycrawl/tools/checkstyle/ant/CheckstyleAntTask.java
index 33d75e3..b70121b 100644
--- a/src/main/java/com/puppycrawl/tools/checkstyle/ant/CheckstyleAntTask.java
+++ b/src/main/java/com/puppycrawl/tools/checkstyle/ant/CheckstyleAntTask.java
@@ -58,6 +58,7 @@ import com.puppycrawl.tools.checkstyle.api.Configuration;
 import com.puppycrawl.tools.checkstyle.api.RootModule;
 import com.puppycrawl.tools.checkstyle.api.SeverityLevel;
 import com.puppycrawl.tools.checkstyle.api.SeverityLevelCounter;
+import com.puppycrawl.tools.checkstyle.checks.naming.AbbreviationAsWordInNameCheck;
 
 /**
  * An implementation of an ANT task for calling checkstyle. See the documentation
@@ -67,6 +68,7 @@ public class CheckstyleAntTask extends Task {
 
     /** Poor man's enum for an xml formatter. */
     private static final String E_XML = "xml";
+    private AbbreviationAsWordInNameCheck check;
     /** Poor man's enum for a plain formatter. */
     private static final String E_PLAIN = "plain";
     /** Poor man's enum for a sarif formatter. */
diff --git a/src/main/java/com/puppycrawl/tools/checkstyle/api/AbstractCheck.java b/src/main/java/com/puppycrawl/tools/checkstyle/api/AbstractCheck.java
index fc98863..ab04681 100644
--- a/src/main/java/com/puppycrawl/tools/checkstyle/api/AbstractCheck.java
+++ b/src/main/java/com/puppycrawl/tools/checkstyle/api/AbstractCheck.java
@@ -25,6 +25,7 @@ import java.util.Set;
 import java.util.SortedSet;
 import java.util.TreeSet;
 
+import com.puppycrawl.tools.checkstyle.checks.naming.AbbreviationAsWordInNameCheck;
 import com.puppycrawl.tools.checkstyle.utils.CommonUtil;
 
 /**
@@ -51,6 +52,9 @@ public abstract class AbstractCheck extends AbstractViolationReporter {
     /** The tokens the check is interested in. */
     private final Set<String> tokens = new HashSet<>();
 
+
+
+
     /**
      * The tab width for column reporting. Default is uninitialized as the value is inherited from
      * the parent module.
diff --git a/src/main/java/com/puppycrawl/tools/checkstyle/api/AbstractFileSetCheck.java b/src/main/java/com/puppycrawl/tools/checkstyle/api/AbstractFileSetCheck.java
index 4a36b03..a534e0f 100644
--- a/src/main/java/com/puppycrawl/tools/checkstyle/api/AbstractFileSetCheck.java
+++ b/src/main/java/com/puppycrawl/tools/checkstyle/api/AbstractFileSetCheck.java
@@ -24,6 +24,7 @@ import java.util.Arrays;
 import java.util.SortedSet;
 import java.util.TreeSet;
 
+import com.puppycrawl.tools.checkstyle.checks.naming.AbbreviationAsWordInNameCheck;
 import com.puppycrawl.tools.checkstyle.utils.CommonUtil;
 
 /**
@@ -65,6 +66,8 @@ public abstract class AbstractFileSetCheck
      */
     private int tabWidth;
 
+    private AbbreviationAsWordInNameCheck check;
+
     /**
      * Called to process a file that matches the specified file extensions.
      *
diff --git a/src/main/java/com/puppycrawl/tools/checkstyle/checks/header/AbstractHeaderCheck.java b/src/main/java/com/puppycrawl/tools/checkstyle/checks/header/AbstractHeaderCheck.java
index ddf463d..17a12e5 100644
--- a/src/main/java/com/puppycrawl/tools/checkstyle/checks/header/AbstractHeaderCheck.java
+++ b/src/main/java/com/puppycrawl/tools/checkstyle/checks/header/AbstractHeaderCheck.java
@@ -39,6 +39,7 @@ import com.puppycrawl.tools.checkstyle.XdocsPropertyType;
 import com.puppycrawl.tools.checkstyle.api.AbstractFileSetCheck;
 import com.puppycrawl.tools.checkstyle.api.CheckstyleException;
 import com.puppycrawl.tools.checkstyle.api.ExternalResourceHolder;
+import com.puppycrawl.tools.checkstyle.checks.naming.AbbreviationAsWordInNameCheck;
 import com.puppycrawl.tools.checkstyle.utils.CommonUtil;
 
 /**
@@ -60,6 +61,7 @@ public abstract class AbstractHeaderCheck extends AbstractFileSetCheck
     /** Specify the character encoding to use when reading the headerFile. */
     @XdocsPropertyType(PropertyType.STRING)
     private Charset charset;
+    private AbbreviationAsWordInNameCheck check;
 
     /**
      * Hook method for post-processing header lines.
diff --git a/src/main/java/com/puppycrawl/tools/checkstyle/checks/naming/AbbreviationAsWordInNameCheck.java b/src/main/java/com/puppycrawl/tools/checkstyle/checks/naming/AbbreviationAsWordInNameCheck.java
index b1d30b3..c1799c4 100644
--- a/src/main/java/com/puppycrawl/tools/checkstyle/checks/naming/AbbreviationAsWordInNameCheck.java
+++ b/src/main/java/com/puppycrawl/tools/checkstyle/checks/naming/AbbreviationAsWordInNameCheck.java
@@ -27,6 +27,7 @@ import java.util.Set;
 import java.util.stream.Collectors;
 
 import com.puppycrawl.tools.checkstyle.StatelessCheck;
+import com.puppycrawl.tools.checkstyle.ant.CheckstyleAntTask;
 import com.puppycrawl.tools.checkstyle.api.AbstractCheck;
 import com.puppycrawl.tools.checkstyle.api.DetailAST;
 import com.puppycrawl.tools.checkstyle.api.TokenTypes;
@@ -144,6 +145,7 @@ public class AbbreviationAsWordInNameCheck extends AbstractCheck {
      * Warning message key.
      */
     public static final String MSG_KEY = "abbreviation.as.word";
+    private CheckstyleAntTask task;
 
     /**
      * The default value of "allowedAbbreviationLength" option.

by command:

✔ ~/java/github/romani/checkstyle [i14053-cycle L|✚ 5] 
$ mvn clean test -Dtest=ArchUnitCyclesCheckTest
.....
[INFO] Running com.puppycrawl.tools.checkstyle.internal.ArchUnitCyclesCheckTest
SLF4J: Failed to load class "org.slf4j.impl.StaticLoggerBinder".
SLF4J: Defaulting to no-operation (NOP) logger implementation
SLF4J: See http://www.slf4j.org/codes.html#StaticLoggerBinder for further details.
[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 5.875 s -- in com.puppycrawl.tools.checkstyle.internal.ArchUnitCyclesCheckTest

[romani]

and keep this number of violations

I see no violations for now at all.

[romani]

@Vyom-Yadav , do you remember why our validation doesn't detect cycles? See log above

[rnveach]

Test is still disabled in master.

checkstyle/src/test/java/com/puppycrawl/tools/checkstyle/internal/ArchUnitCyclesCheckTest.java

Line 33 in 11e2b4b

@Disabled("until https://github.com/checkstyle/checkstyle/issues/14053")

[romani]

I put a lot of details to #14060, I do not think we can easily use this Rule now.

[nrmancuso]

@romani are we done here?

[romani]

I planned to open new PR, to do it in "correct way " at least as they wanted it to be, to open issue on them.
after reading their issue tracker, especially TNG/ArchUnit#1068 , I think we need to abandon it, too much efforts, and still no certain approach.
There are also people complaining that different jdk generates different store files.