Issue #5337: Rename AutomaticBean

[stoyanK7]

Resolves #5337

First step of deprecating AutomaticBean and moving to AbstractAutomaticBean.

[rnveach]

CI Failure: https://github.com/checkstyle/checkstyle/actions/runs/3092656103/jobs/5004176175

Caused by: java.lang.ClassNotFoundException: com.puppycrawl.tools.checkstyle.api.AutomaticBean$OutputStreamOptions
    at org.codehaus.plexus.classworlds.strategy.SelfFirstStrategy.loadClass (SelfFirstStrategy.java:50)
    at org.codehaus.plexus.classworlds.realm.ClassRealm.unsynchronizedLoadClass (ClassRealm.java:271)
    at org.codehaus.plexus.classworlds.realm.ClassRealm.loadClass (ClassRealm.java:247)
    at org.codehaus.plexus.classworlds.realm.ClassRealm.loadClass (ClassRealm.java:239)
    at org.apache.maven.plugins.checkstyle.AbstractCheckstyleReport.getConsoleListener (AbstractCheckstyleReport.java:685)

https://github.com/apache/maven-checkstyle-plugin/blob/627fa4f684866a579f2c105fcc1dbf3ed776daa8/src/main/java/org/apache/maven/plugins/checkstyle/AbstractCheckstyleReport.java#L63 https://github.com/apache/maven-checkstyle-plugin/blob/627fa4f684866a579f2c105fcc1dbf3ed776daa8/src/main/java/org/apache/maven/plugins/checkstyle/AbstractCheckstyleReport.java#L686

[rnveach]

It looks like this usage is preventing us from fully separating the classes like planned as we are completely reliant on this maven plugin and have not severed times with them. https://github.com/checkstyle/checkstyle/issues/11602

Unless there is any other ideas, ~I think we will have to have a copy of OutputStreamOptions in both classes, with everything being marked deprecated in the old class.~ Edit: Since its an enum, duplicating the class means we have 2 different classes and would have to quadruple method signatures. We may be forced to keep the enum in the deprecated class until it can be completely removed.

[stoyanK7]

Not sure why No Error Test (openjdk11, fast pool) - CMD=.ci/validation.sh no-error-methods-distance complains. https://github.com/sevntu-checkstyle/methods-distance/blob/412c3bff7258daf7683081f72dd1bf9d8aaf622a/methods-distance/src/test/java/com/github/sevntu/checkstyle/domain/BaseCheckTestSupport.java#L132-L136

I think it automatically tries to use AbstractAutomaticBean.OutputStreamOptions instead of AutomaticBean.OutputStreamOptions? Is an explicit import valid solution here?

---

Should I also deal with Checker-Framework errors? I notice it is still being introduced to the repo.

---

Pitest / pitest (pitest-api) (pull_request)

Error:  Failed to execute goal org.pitest:pitest-maven:1.9.5:mutationCoverage (default-cli) on project checkstyle: 
Line coverage of 99(661/665) is below threshold of 100 -> [Help 1]

Ran pitest locally and this is the result 650/650, not 661/665 though it fails with same error message.

[rnveach]

Should I also deal with Checker-Framework errors?

I am not really a fan of checker, so I would concentrate on it's failures last. It looks like pitest is failing, so I would look at that first.

Not sure why [No Error Test (openjdk11, fast pool) - CMD=.ci/validation.sh no-error-methods-distance] I think it automatically tries to use AbstractAutomaticBean.OutputStreamOptions instead of AutomaticBean.OutputStreamOptions? Is an explicit import valid solution here?

I am surprised some import statement is not needed for OutputStreamOptions in this situation. Forcefully adding one gives an error in Eclipse saying the import is never used.

I do think one issue is there are 2 OutputStreamOptions in this PR. I thought we would keep the original enum in the deprecated class and leave it there and not duplicate it until we can full break compatibility as said in https://github.com/checkstyle/checkstyle/pull/12213#issuecomment-1253911824 .

Pitest / pitest (pitest-api) (pull_request) 99(661/665) is below threshold of 100

I don't know why the main report is not reporting anything, but do a scan on uncovered and that will narrow down where you need to look. I was only seeing lines in AbstractAutomaticBean connected to the enum. So I assume that is what it is reporting. Since this connects to my previous statement, lets look into that first and then we can come back to pitest.

[stoyanK7]

@rnveach Removed OutputStreamOptions completely from AbstractAutomaticBean. pitest-api seems to be ok.

However, the No Error Test (openjdk11, fast pool) - CMD=.ci/validation.sh no-error-methods-distance problem still persists:

[ERROR] /home/semaphore/checkstyle/.ci-temp/methods-distance/methods-distance/src/test/java/com/github/sevntu/checkstyle/domain/BaseCheckTestSupport.java:[135,56] cannot find symbol
[ERROR]   symbol:   variable OutputStreamOptions

[rnveach]

@stoyanK7 The compilation error changed, it isn't the same issue, so you'll need to look at what we need to do to make it pass. All these compilation errors seem to mean a classpath is changing and the other projects we are compiling can't be changed until this PR is released, so you have to keep the classpaths the same.

[stoyanK7]

@rnveach

I am looking at ClassNotFound failures a few commits ago(before OutputStreamOptions was moved to AutomaticBean).

no-exception-test.sh guava-with-google-checks

java.lang.NoClassDefFoundError: com/puppycrawl/tools/checkstyle/api/AutomaticBean$OutputStreamOptions 01:16
    at org.apache.maven.plugins.checkstyle.AbstractCheckstyleReport.getConsoleListener (AbstractCheckstyleReport.java:685)

We have explicit import at AbstractCheckstyleReport.java#63

no-error-orekit

Caused by: java.lang.NoClassDefFoundError: com/puppycrawl/tools/checkstyle/api/AutomaticBean$OutputStreamOptions 02:10
    at org.apache.maven.plugins.checkstyle.CheckstyleViolationCheckMojo.getConsoleListener (CheckstyleViolationCheckMojo.java:802)

We have explicit import at CheckstyleViolationCheckMojo.java#65

These two and multiple other jobs that have the same errors are now working except no-error-methods-distance because it does not have that explicit import. I do not see another way here.

Besides, there are already multiple explicit imports from com.puppycrawl.tools.checkstyle in BaseCheckTestSupport

[rnveach]

We have explicit import at CheckstyleViolationCheckMojo.java#65 no-error-methods-distance

The main difference I am seeing between these is that methods-distance extends DefaultLogger while the others just instantiate DefaultLogger. When I modify BaseCheckTestSupport and remove the extension of DefaultLogger it then starts complaining about it doesn't know where OutputStreamOptions comes from.

Like I said, I don't understand how this is all working behind the scenes but this is appears to be how it is working. Feel free to test things out on your local to see how things are behaving and if a resolution can be made.

As a fall back, you can try to add an import to the class and see if it will compile with this PR. In my Eclipse, it tells me its an unnecessary import and we have other static checks that also may flag it as unnecessary and prevent merging that fix.

there are already multiple explicit imports from com.puppycrawl.tools.checkstyle in BaseCheckTestSupport

Because the package name is different, so the imports have to be made explicit. All those classes are being instantiated and not extended which is making OutputStreamOptions behave differently for imports.

=====

The reason why it is "not working" anymore is because DefaultLogger is not extending the class that has the enum, so it is not seeing the enum in its path anymore. The enum has to stay in the same location for other CI items...

I also noticed that the enum is also now being marked as deprecated since the class it comes from, AutomaticBean is deprecated. This is not looking good since this enum is not being deprecated but it appears that way. I am rethinking my earlier decision of not duplicating the enum in the AbstractAutomaticBean. We need somewhere for them to swap to if the enum will have any chance of being deprecated. If we do this, we will need to appease code coverage but all instances should be using the non-deprecated enum. I recommend making something to convert one enum to the other. This will also resolve the methods-distance issue as well.

Let me know if you have any other ideas.

[stoyanK7]

@rnveach

As a fallback, you can try to add an import to the class and see if it will compile with this PR. In my Eclipse, it tells me its an unnecessary import and we have other static checks that also may flag it as unnecessary and prevent merging that fix.

IntelliJ is okay with this import for me when methods-distance project is under .ci-temp/ folder. Though running mvn package results in build failure:

Tests run: 31, Failures: 30, Errors: 0, Skipped: 1

[INFO] ------------------------------------------------------------------------
[INFO] Reactor Summary for methods-distance-parent 1.0-SNAPSHOT:
[INFO] 
[INFO] methods-distance-parent ............................ SUCCESS [  2.529 s]
[INFO] methods-distance ................................... FAILURE [  1.466 s]
[INFO] methods-distance-web ............................... SKIPPED
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE

Cloning the repo and running it alone fails the same way. Am I missing something? README.md only mentions to clone source and run mvn package.

[rnveach]

Have you tried the other items before the "fallback" one? If the others are not working, please explain why.

[stoyanK7]

Currently testing them, the above one is an experiment as well. Will let you know when done.

[rnveach]

Though running mvn package results in build failure:

This is the command we run in CI. If you still have issues I would need to know what the failure is. https://github.com/checkstyle/checkstyle/blob/master/.ci/validation.sh#L709-L710

[rnveach]

@stoyanK7 ping

[stoyanK7]

On it. Apologies.

[stoyanK7]

Ran pitest-api mode locally. Doesn't show where the uncovered lines are:

[ERROR] Failed to execute goal org.pitest:pitest-maven:1.11.0:mutationCoverage (default-cli) on project checkstyle: Line coverage of 99(538/544) is below threshold of 100 -> [Help 1]
org.apache.maven.lifecycle.LifecycleExecutionException: Failed to execute goal org.pitest:pitest-maven:1.11.0:mutationCoverage (default-cli) on project checkstyle: Line coverage of 99(538/544) is below threshold of 100

I assume it is the enum like before?

[rnveach]

@Vyom-Yadav @nrmancuso https://github.com/checkstyle/checkstyle/actions/runs/4213455949/jobs/7313174365#step:6:365 This PR has a coverage drop but pitest says No new surviving mutation(s) found..

Edit: I will update this with more info.

Line coverage of 99(538/544) is below threshold of 100

We have a line coverage drop, but not a mutation drop (least not failed with). So this is why we have "no new surviving mutations".

For me, master says 640/640.

The report downloaded from the CI does not have 538/544 for line coverage. It says 532/532 and 100%.

The CI report has lost AutomaticBean.java completely. All other classes report the same numbers as master. There is no AbstractAutomaticBean.java listed but it moved outside api package. AutomaticBean is still listed in api but only contains an enum class.

I can confirm CI behavior in my local.

[rnveach]

@stoyanK7 This looks like an issue with pitest. It doesn't report data on AbstractAutomaticBean so it doesn't show up in the report. As such, I think missing code coverage is in that class.

Please report issue to https://github.com/hcoles/pitest/issues/ so we can get their take and look into the class you changed and it's code coverage to show us it is still at 100%.

Also please resolve other CI errors and anything left for this PR.

Edit: AbstractAutomaticBean is no longer in API package. So this is even more confusing, issue needs to be reported to pitest as it isn't making sense to me.

Edit 2: Report doesn't list the remaining AutomaticBean which just contains an Enum inside it. So yea, it seems likely it is what is having issue with line coverage. This still seems like an issue in pitest.

[stoyanK7]

@rnveach Most of the checker errors are just old ones that are being moved to AbstractAutomaticBean. Should I bother with fixing those?

[romani]

Just reminder that we need compatibility. We need to be sure that sevntu works fine.

[stoyanK7]

Sevntu fails because https://github.com/checkstyle/checkstyle/pull/12755 is waiting to get merged.

[romani]

I marked such violations as wontfix

[romani]

@stoyanK7 , please resolve conflict.

[stoyanK7]

@romani Can I resolve this conflict when this PR is approved? This conflict happens almost every day.

[romani]

Can I resolve this conflict when this PR is approved?

ok.