feat: Add support for Checkstyle Configuration file properties

[SaptarshiSarkar12]

What's changed?

• Fixes feat: Add support for variables in checkstyle config file #699

Anyone you would like to review specifically?

@timtebeek

Checklist

• I've added unit tests to cover both positive and negative cases
• I've read and applied the recipe conventions and best practices
• I've used the IntelliJ IDEA auto-formatter on affected files

[SaptarshiSarkar12]

@timtebeek I have created a blank test class. Can you please tell how I should test for the working of the checkstyle config file variables?

[timtebeek]

Hi @SaptarshiSarkar12 ! You'll want to create matching files in https://github.com/openrewrite/rewrite-maven-plugin/tree/main/src/test/resources-its/org/openrewrite/maven as you can see there for other unit tests too. All those tests use https://khmarbaise.github.io/maven-it-extension/itf-documentation/usersguide/usersguide.html, which might help to read up on as well. Thanks for helping out here!

[SaptarshiSarkar12]

Hi @timtebeek 👋!
Thank you for pointing to the proper resources. I was thinking that this project was using basic Junit Test structure. So, I couldn't understand what IT meant and how did the test classes get executed. But, now I know about the Junit extension that this project is using. Thank you for that. If I face any issue, I'll post those problems here 😄.

[SaptarshiSarkar12]

@timtebeek As a side question, is there a general rule of this project to assign the PRs to the contributors? I have seen projects assign the issues to the contributors. If this is a silly question, please do not mind 🙃. I asked this out of curiosity 😄.

[timtebeek]

As a side question, is there a general rule of this project to assign the PRs to the contributors? I have seen projects assign the issues to the contributors. If this is a silly question, please do not mind 🙃. I asked this out of curiosity 😄.

We typically assign the issue or pull request to whoever is working on it, also if they are community contributors. That way it's easy to see at a glance who (if anyone) is working on something from our project board. Sometimes when folks then later indicate they do not have the time or expertise to finish something, we can unassign them to indicate someone else needs to step up. Not a silly question at all; hope my answer helps you understand!

[SaptarshiSarkar12]

Yeah, I understood. Thank you for the explanation @timtebeek 😁!

[SaptarshiSarkar12]

@timtebeek Which recipe should I use in the plugin configuration for tests - the custom configuration that I made in strimzi/strimzi-kafka-operator#9422 or the one described in the docs of open-rewrite?

[timtebeek]

@timtebeek Which recipe should I use in the plugin configuration for tests - the custom configuration that I made in strimzi/strimzi-kafka-operator#9422 or the one described in the docs of open-rewrite?

org.openrewrite.staticanalysis.CodeCleanup should work well enough I suppose. :)

[SaptarshiSarkar12]

Yeah, it should 😄.

[SaptarshiSarkar12]

@timtebeek I have added a small test project in the resource-its folder as well as I have added a method in the test directory. Can you please check if those changes are right? Can you please tell me what are the next steps to do?

[SaptarshiSarkar12]

@timtebeek I have added a small test project in the resource-its folder as well as I have added a method in the test directory. Can you please check if those changes are right? Can you please tell me what are the next steps to do?

@timtebeek Have you checked?

[SaptarshiSarkar12]

@timtebeek Please tell me what changes are required.

[timtebeek]

Hi @SaptarshiSarkar12 ; Appreciate your enthusiasm to get this going! I've not had time yet to dive into what would be needed next; I did push two small changes to hopefully get the test going, after seeing the tests previously failed with

[ERROR] org.openrewrite.maven.CheckstylePropertiesTest.checkstyleProperties(MavenExecutionResult)  Time elapsed: 0.035 s  <<< ERROR!
com.soebes.itf.jupiter.extension.exceptions.PathUtilException: java.nio.file.NoSuchFileException: /home/runner/work/rewrite-maven-plugin/rewrite-maven-plugin/target/test-classes/org/openrewrite/maven/CheckstylePropertiesTest/checkstyleProperties
	at com.soebes.itf.jupiter.extension.PathUtils.copy(PathUtils.java:51)
	at com.soebes.itf.jupiter.extension.PathUtils.copyDirectoryRecursively(PathUtils.java:109)
	at com.soebes.itf.jupiter.extension.MavenITExtension.beforeEach(MavenITExtension.java:133)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
Caused by: java.nio.file.NoSuchFileException: /home/runner/work/rewrite-maven-plugin/rewrite-maven-plugin/target/test-classes/org/openrewrite/maven/CheckstylePropertiesTest/checkstyleProperties
	at java.base/sun.nio.fs.UnixException.translateToIOException(UnixException.java:92)
	at java.base/sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:106)
	at java.base/sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:111)
	at java.base/sun.nio.fs.UnixCopyFile.copy(UnixCopyFile.java:548)
	at java.base/sun.nio.fs.UnixFileSystemProvider.copy(UnixFileSystemProvider.java:257)
	at java.base/java.nio.file.Files.copy(Files.java:1305)
	at com.soebes.itf.jupiter.extension.PathUtils.copy(PathUtils.java:49)
	... 4 more

Sadly now there's a different puzzling error.

[timtebeek]

Now finally we fail with

,  [STDOUT] [ERROR] Failed to execute goal org.apache.maven.plugins:maven-checkstyle-plugin:3.3.1:check (validate) on project checkstyle_properties: Failed during checkstyle execution: Failed during checkstyle configuration: unable to parse configuration stream: com.puppycrawl.tools.checkstyle.api.CheckstyleException: Property ${checkstyle.suppressions.file} has not been set -> [Help 1]

Time to work in this earlier suggestion #699 (comment) ; Would you want to try your hand at that @SaptarshiSarkar12 ?

[SaptarshiSarkar12]

Hi @SaptarshiSarkar12 ; Appreciate your enthusiasm to get this going! I've not had time yet to dive into what would be needed next; I did push two small changes to hopefully get the test going, after seeing the tests previously failed with

[ERROR] org.openrewrite.maven.CheckstylePropertiesTest.checkstyleProperties(MavenExecutionResult)  Time elapsed: 0.035 s  <<< ERROR!
com.soebes.itf.jupiter.extension.exceptions.PathUtilException: java.nio.file.NoSuchFileException: /home/runner/work/rewrite-maven-plugin/rewrite-maven-plugin/target/test-classes/org/openrewrite/maven/CheckstylePropertiesTest/checkstyleProperties
	at com.soebes.itf.jupiter.extension.PathUtils.copy(PathUtils.java:51)
	at com.soebes.itf.jupiter.extension.PathUtils.copyDirectoryRecursively(PathUtils.java:109)
	at com.soebes.itf.jupiter.extension.MavenITExtension.beforeEach(MavenITExtension.java:133)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
Caused by: java.nio.file.NoSuchFileException: /home/runner/work/rewrite-maven-plugin/rewrite-maven-plugin/target/test-classes/org/openrewrite/maven/CheckstylePropertiesTest/checkstyleProperties
	at java.base/sun.nio.fs.UnixException.translateToIOException(UnixException.java:92)
	at java.base/sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:106)
	at java.base/sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:111)
	at java.base/sun.nio.fs.UnixCopyFile.copy(UnixCopyFile.java:548)
	at java.base/sun.nio.fs.UnixFileSystemProvider.copy(UnixFileSystemProvider.java:257)
	at java.base/java.nio.file.Files.copy(Files.java:1305)
	at com.soebes.itf.jupiter.extension.PathUtils.copy(PathUtils.java:49)
	... 4 more

Sadly now there's a different puzzling error.

@timtebeek Thank you for spending your valuable time! Sorry, I was very busy the last three days. So, I couldn't reply.

[SaptarshiSarkar12]

@timtebeek I wanted to know how I can print the content of the checkstyle config that I modified in this code block. I tried this one, but it didn't work. Also, the output of System.out.println() was not visible in the terminal while running the test. Can you please help me?

// Print out each line of the resulting string
getLog().info("Checkstyle config file contents:\n" + checkstyleConfig);

[timtebeek]

Hi @SaptarshiSarkar12 I suppose you want to print as a form of debugging? Might be better to attach an actual debugger if at all possible. We don't use logging much, but you can assert a particular message was logged with the pattern seen here.

rewrite-maven-plugin/src/test/java/org/openrewrite/maven/BasicIT.java

Lines 43 to 47 in e8b5855

	assertThat(result)
	.isSuccessful()
	.out()
	.info()
	.anySatisfy(line -> assertThat(line).contains("Applying recipes would make no changes. No patch file generated."));

[SaptarshiSarkar12]

Hi @SaptarshiSarkar12 I suppose you want to print as a form of debugging?

Yeah, I wanted to test if the checkstyle config string is modified or not.

Might be better to attach an actual debugger if at all possible. We don't use logging much, but you can assert a particular message was logged with the pattern seen here.

@timtebeek But I wanted to see what the content of the checkstyle config string is. How do I do that?

[timtebeek]

@timtebeek But I wanted to see what the content of the checkstyle config string is. How do I do that?

You can either create an assertion that fails (does the result .out() contain a bogus String, or set a breakpoint in your test and inspect the result directly.

[SaptarshiSarkar12]

You can either create an assertion that fails (does the result .out() contain a bogus String, or set a breakpoint in your test and inspect the result directly.

There's no method named out() under result.

@timtebeek I tried to see if I can get anything I logged previously in the result object but, there's none. Is there any method to debug code? Like how do you debug in this maven plugin project? I am new to debugging in projects concerned with plugin. Breakpoints seem to work only in the Test method and nowhere else, not even in the internal files (ConfigurableRewriteMojo.java).

I tried setting breakpoints but they didn't get triggered during the execution of the test via IntelliJ. No notification for ignoring a breakpoint (the usual behaviour of IntelliJ) was observed.

Do breakpoints even work here?

[SaptarshiSarkar12]

@timtebeek Please reply 😄.

[timtebeek]

Hi @SaptarshiSarkar12 ! Apologies; it's hard to get to everything. As far as I can tell breakpoints indeed do not work with the tst framework we use for the plugin, so that will complicate the work that you're doing.

You can see how assertions on the logged output are handled in this recent example.

rewrite-maven-plugin/src/test/java/org/openrewrite/maven/RewriteRunIT.java

Lines 48 to 57 in d86d22c

	void multi_source_sets_project(MavenExecutionResult result) {
	assertThat(result)
	.isSuccessful()
	.out()
	.warn()
	.contains(
	"Changes have been made to target/maven-it/org/openrewrite/maven/RewriteRunIT/multi_source_sets_project/project/src/integration-test/java/sample/IntegrationTest.java by:",
	"Changes have been made to target/maven-it/org/openrewrite/maven/RewriteRunIT/multi_source_sets_project/project/src/test/java/sample/RegularTest.java by:"
	);
	}

A similar approach could help you in working through your plugin changes there.

[SaptarshiSarkar12]

Hi Tim 👋!
Okay, I would try it that way, once more.
Thank you for the help.

[github-actions]

This PR is stale because it has been open for 90 days with no activity. Remove stale label or comment or this will be closed in 7 days.

[SaptarshiSarkar12]

Hi 👋!
I was very busy in my academics work for the last 6-7 months and I couldn't get enough time to focus on this PR. I will look into it soon.

[SaptarshiSarkar12]

@timtebeek I tried to go through the codebase and figured out the following findings. Please tell me if my approach requires any change.

• The checkstyle config file content is loaded in

  rewrite-maven-plugin/src/main/java/org/openrewrite/maven/ConfigurableRewriteMojo.java

  Line 245 in 6317586

  styles.add(loadCheckstyleConfig(Paths.get(checkstyleConfigFile), emptyMap()));

• I will get the config file location via checkstyleConfigFile and the parameters to be replaced via some XML parameters.
• Then, I will replace the checkstyle parameters and might either save it as a temp file or pass the new file content as a string (I need your views on it).

[timtebeek]

It looks like your earlier change in d25aed2 cleared out any changes you had proposed adding; would it make sense to try to restore those on your end?

[SaptarshiSarkar12]

It looks like your earlier change in d25aed2 cleared out any changes you had proposed adding; would it make sense to try to restore those on your end?

Hi @timtebeek 👋!
I apologise for the late reply. As the codebase has changed a lot within the few months, I thought to start from the latest changes to avoid confusions in the code. I shared my approach that I found out suitable after looking through the codebase. So, should I restore the changes? Or should I go with the new approach if it stands good in your opinion?

[timtebeek]

Welcome to force push your new approach here!

[github-actions]

This PR is stale because it has been open for 90 days with no activity. Remove stale label or comment or this will be closed in two weeks. PRs may be reopened when there is renewed interest.

[SaptarshiSarkar12]

@timtebeek I made some changes in ConfigurableRewriteMojo.java file. Can you please verify if I am going in the right direction?
How do I test whether they work as expected? Do I need to write any IT test for that?

[timtebeek]

Thanks @SaptarshiSarkar12 ! Yes it would be great to have a unit test for your case, just to ensure we don't unintentionally break things in the future. You can look at the existing tests we have as a combination of resources-its and src/test/java:
https://github.com/openrewrite/rewrite-maven-plugin/tree/main/src/test

I've myself seen some challenges running those tests from IntelliJ unless you correctly set your PATH variable; might help to test using mvn verify.