Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add support for Checkstyle Configuration file properties #700

Draft
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

SaptarshiSarkar12
Copy link

@SaptarshiSarkar12 SaptarshiSarkar12 commented Dec 29, 2023

What's changed?

Fixes #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
Copy link
Author

@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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Author

@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
Copy link
Contributor

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
Copy link
Author

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

@SaptarshiSarkar12
Copy link
Author

SaptarshiSarkar12 commented Jan 3, 2024

@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
Copy link
Contributor

@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
Copy link
Author

Yeah, it should 😄.

@SaptarshiSarkar12
Copy link
Author

@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
Copy link
Author

@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
Copy link
Author

@timtebeek Please tell me what changes are required.

@timtebeek
Copy link
Contributor

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 timtebeek added the bug Something isn't working label Jan 16, 2024
@timtebeek
Copy link
Contributor

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
Copy link
Author

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
Copy link
Author

SaptarshiSarkar12 commented Feb 10, 2024

@timtebeek Please tell me if any further changes in the missing files are required 😃.
If not, then can you please guide me how to change the way the checkstyle properties are handled by open rewrite Maven plugin?

@SaptarshiSarkar12
Copy link
Author

@timtebeek Please reply 😃.

@timtebeek
Copy link
Contributor

Hi yes sorry I'm swamped with work and other reviews at the moment. This PR seems like it would take more effort on my side to help guide you through, which I'm having trouble fitting in on the short term.

There's were some earlier hints here and here, but I don't know if that's enough for you to try your hand at changing the actual code too in addition to the test you've already added.

@SaptarshiSarkar12
Copy link
Author

@timtebeek I was busy in some other works the last month. I went through some parts of the ConfigurableRewriteMojo java class and learnt how we can retrieve the required data from the pom file. I added a <CheckstyleProperties/> tag in pom file and a method to retrieve the same. But, got some error as shown below 👇

image

Do you know how to fix that?
Secondly, can you please check if the way to replace the checkstyle properties with the actual value is correct?

Plugin checkstylePlugin = project.getPlugin("org.apache.maven.plugins:maven-checkstyle-plugin");
if (checkstyleConfigFile != null && !checkstyleConfigFile.isEmpty()) {
// Convert the checkstyle config file contents to a String
String checkstyleConfig = new String(Files.readAllBytes(Paths.get(checkstyleConfigFile)));
Set<String> checkstyleProperties = getCheckstyleProperties();
if (!checkstyleProperties.isEmpty()) {
checkstyleConfig = checkstyleProperties.stream()
.map(s -> "-P" + s)
.collect(Collectors.joining("\n", "", "\n")) + checkstyleConfig;
}
styles.add(CheckstyleConfigLoader.loadCheckstyleConfig(checkstyleConfig, emptyMap()));
// styles.add(CheckstyleConfigLoader.loadCheckstyleConfig(Paths.get(checkstyleConfigFile), emptyMap()));

@SaptarshiSarkar12
Copy link
Author

@timtebeek Can you please help me to fix the above problem?

@knutwannheden
Copy link
Contributor

Based on the screenshot it looks like you need to rename your folder from CheckstylePropertiesTest to CheckstylePropertiesIT.

@SaptarshiSarkar12
Copy link
Author

@knutwannheden Thank you for the help 😄!
It worked. But, there's another error coming up. It says about any itf-repo folder which I don't think exist in the project. Do you know how to fix this error?
image

@timtebeek
Copy link
Contributor

I've renamed the test resources to line up with the test class name, as required by the integration test framework.

Unfortunately it seems there's now some conflicts with the main branch, after merging this recent support for inline config:

@SaptarshiSarkar12
Copy link
Author

SaptarshiSarkar12 commented Apr 25, 2024

I've renamed the test resources to line up with the test class name, as required by the integration test framework.

Thank you 😄. But, there's another error coming up as posted in this comment. Can you help me fix that error?

Unfortunately it seems there's now some conflicts with the main branch, after merging this recent support for inline config:

I have resolved the conflicts and merged the changes successfully.
Thank you for informing @timtebeek.

@SaptarshiSarkar12
Copy link
Author

@timtebeek Can you please tell me how do I fix that error?

@timtebeek
Copy link
Contributor

I've pushed up a change to fix the test for now @SaptarshiSarkar12 ; hope that helps you on your way!

@SaptarshiSarkar12
Copy link
Author

Thank you @timtebeek 😄. I'll try if it works, and no errors are found. Will certainly let you know 🙃.

@SaptarshiSarkar12
Copy link
Author

I've pushed up a change to fix the test for now @SaptarshiSarkar12 ; hope that helps you on your way!

@timtebeek Sorry for the delay. I was quite busy in these few months. So, I couldn't get time. I tried running the tests today and it worked successfully 🎉. Here's a screenshot for your reference 😄. Thank you for the fix.
image

@SaptarshiSarkar12
Copy link
Author

@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);

image

@timtebeek
Copy link
Contributor

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.

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

@SaptarshiSarkar12
Copy link
Author

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
Copy link
Contributor

@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
Copy link
Author

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.
image

@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).
image

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.
image

Do breakpoints even work here?
image

@SaptarshiSarkar12
Copy link
Author

@timtebeek Please reply 😄.

@timtebeek
Copy link
Contributor

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.

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
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

feat: Add support for variables in checkstyle config file
3 participants