TestsShouldNotBePublic inserts modifications with tab indentation into an otherwise space indented file

Philzen commented 1 year ago

What version of OpenRewrite are you using?

I can only assume this used

OpenRewrite 8.7.2
rewrite-spring v5.0.11
rewrite-testing-frameworks 2.0.12

as i used the CLI command apparently has all current dependencies up-to-date and locally available. Attaching the full output here:

Full execution output

$ mvn -U org.openrewrite.maven:rewrite-maven-plugin:run \
  -Drewrite.recipeArtifactCoordinates=org.openrewrite.recipe:rewrite-spring:RELEASE \
  -Drewrite.activeRecipes=org.openrewrite.java.spring.boot2.UpgradeSpringBoot_2_7
[INFO] Scanning for projects...
Downloading from central: https://repo.maven.apache.org/maven2/org/openrewrite/maven/rewrite-maven-plugin/maven-metadata.xml
Downloaded from central: https://repo.maven.apache.org/maven2/org/openrewrite/maven/rewrite-maven-plugin/maven-metadata.xml (4.7 kB at 7.0 kB/s)
[INFO] 
[INFO] -------------------------< de.moovit.mcc:Cron >-------------------------
[INFO] Building cronjob 4.1.0.8
[INFO]   from pom.xml
[INFO] --------------------------------[ jar ]---------------------------------
[INFO] 
[INFO] >>> rewrite:5.8.0:run (default-cli) > process-test-classes @ Cron >>>
[INFO] 
[INFO] --- build-helper:3.0.0:add-source (add-source) @ Cron ---
[INFO] Source directory: /home/user/prog/moovit/components/cron/target/generated/src/main/java added.
[INFO] 
[INFO] --- openapi-generator:4.0.0-beta:generate (default) @ Cron ---
[INFO] Code generation is skipped because input was unchanged
[INFO] 
[INFO] --- resources:3.1.0:resources (default-resources) @ Cron ---
[INFO] Using 'UTF-8' encoding to copy filtered resources.
[INFO] Copying 1 resource
[INFO] Copying 2 resources
[INFO] 
[INFO] --- compiler:3.8.1:compile (default-compile) @ Cron ---
[INFO] Compiling 1 source file to /home/user/prog/moovit/components/cron/target/classes
[INFO] 
[INFO] --- resources:3.1.0:testResources (default-testResources) @ Cron ---
[INFO] Using 'UTF-8' encoding to copy filtered resources.
[INFO] Copying 2 resources
[INFO] 
[INFO] --- compiler:3.8.1:testCompile (default-testCompile) @ Cron ---
[INFO] Nothing to compile - all classes are up to date
[INFO] 
[INFO] <<< rewrite:5.8.0:run (default-cli) < process-test-classes @ Cron <<<
[INFO] 
[INFO] 
[INFO] --- rewrite:5.8.0:run (default-cli) @ Cron ---
[INFO] Using active recipe(s) [org.openrewrite.java.spring.boot2.UpgradeSpringBoot_2_7]
[INFO] Using active styles(s) []
Downloading from central: https://repo.maven.apache.org/maven2/org/openrewrite/recipe/rewrite-spring/maven-metadata.xml
Downloaded from central: https://repo.maven.apache.org/maven2/org/openrewrite/recipe/rewrite-spring/maven-metadata.xml (2.4 kB at 55 kB/s)
[INFO] Validating active recipes...
[INFO] Project [cronjob] Resolving Poms...
[INFO] Project [cronjob] Parsing source files
[INFO] Running recipe(s)...
line 1:0 missing '@' at 'org'
line 1:0 missing '@' at 'org'
line 1:0 missing '@' at 'org'
line 1:0 missing '@' at 'org'
line 1:0 missing '@' at 'org'
line 1:0 missing '@' at 'org'
line 1:0 missing '@' at 'org'
line 1:0 missing '@' at 'org'
[WARNING] Changes have been made to pom.xml by:
[WARNING]     org.openrewrite.java.spring.boot2.UpgradeSpringBoot_2_7
[WARNING]         org.openrewrite.java.spring.boot2.UpgradeSpringBoot_2_6
[WARNING]             org.openrewrite.java.spring.boot2.UpgradeSpringBoot_2_5
[WARNING]                 org.openrewrite.java.spring.boot2.UpgradeSpringBoot_2_4
[WARNING]                     org.openrewrite.java.spring.boot2.UpgradeSpringBoot_2_3
[WARNING]                         org.openrewrite.java.spring.boot2.UpgradeSpringBoot_2_2
[WARNING]                             org.openrewrite.java.spring.framework.UpgradeSpringFramework_5_2
[WARNING]                                 org.openrewrite.java.spring.framework.UpgradeSpringFramework_5_1
[WARNING]                                     org.openrewrite.java.spring.framework.UpgradeSpringFrameworkDependencies: {newVersion=5.1.x}
[WARNING]                                         org.openrewrite.java.dependencies.UpgradeDependencyVersion: {groupId=org.springframework, artifactId=spring-context, newVersion=5.1.x, overrideManagedVersion=false}
[WARNING]                                             org.openrewrite.maven.UpgradeDependencyVersion: {groupId=org.springframework, artifactId=spring-context, newVersion=5.1.x, overrideManagedVersion=false}
[WARNING]                                 org.openrewrite.java.spring.framework.UpgradeSpringFrameworkDependencies: {newVersion=5.2.x}
[WARNING]                                     org.openrewrite.java.dependencies.UpgradeDependencyVersion: {groupId=org.springframework, artifactId=spring-context, newVersion=5.2.x, overrideManagedVersion=false}
[WARNING]                                         org.openrewrite.maven.UpgradeDependencyVersion: {groupId=org.springframework, artifactId=spring-context, newVersion=5.2.x, overrideManagedVersion=false}
[WARNING]                             org.openrewrite.java.dependencies.UpgradeDependencyVersion: {groupId=org.springframework.boot, artifactId=*, newVersion=2.2.x, overrideManagedVersion=false}
[WARNING]                                 org.openrewrite.maven.UpgradeDependencyVersion: {groupId=org.springframework.boot, artifactId=*, newVersion=2.2.x, overrideManagedVersion=false}
[WARNING]                             org.openrewrite.maven.UpgradeParentVersion: {groupId=org.springframework.boot, artifactId=spring-boot-starter-parent, newVersion=2.2.x}
[WARNING]                         org.openrewrite.maven.UpgradeParentVersion: {groupId=org.springframework.boot, artifactId=spring-boot-starter-parent, newVersion=2.3.x}
[WARNING]                         org.openrewrite.java.dependencies.AddDependency: {groupId=org.springframework.boot, artifactId=spring-boot-starter-validation, version=2.3.x, onlyIfUsing=javax.validation.constraints.*, acceptTransitive=true}
[WARNING]                             org.openrewrite.maven.AddDependency: {groupId=org.springframework.boot, artifactId=spring-boot-starter-validation, version=2.3.x, onlyIfUsing=javax.validation.constraints.*, acceptTransitive=true}
[WARNING]                     org.openrewrite.java.spring.framework.UpgradeSpringFramework_5_3
[WARNING]                         org.openrewrite.java.spring.framework.UpgradeSpringFrameworkDependencies: {newVersion=5.3.x}
[WARNING]                             org.openrewrite.java.dependencies.UpgradeDependencyVersion: {groupId=org.springframework, artifactId=spring-context, newVersion=5.3.x, overrideManagedVersion=false}
[WARNING]                                 org.openrewrite.maven.UpgradeDependencyVersion: {groupId=org.springframework, artifactId=spring-context, newVersion=5.3.x, overrideManagedVersion=false}
[WARNING]                     org.openrewrite.java.spring.boot2.SpringBoot2JUnit4to5Migration
[WARNING]                         org.openrewrite.java.testing.junit5.JUnit4to5Migration
[WARNING]                             org.openrewrite.maven.ExcludeDependency: {groupId=org.junit.vintage, artifactId=junit-vintage-engine}
[WARNING]                     org.openrewrite.maven.UpgradeParentVersion: {groupId=org.springframework.boot, artifactId=spring-boot-starter-parent, newVersion=2.4.x}
[WARNING]                     org.openrewrite.maven.RemoveExclusion: {groupId=org.springframework.boot, artifactId=spring-boot-starter-test, exclusionGroupId=org.junit.vintage, exclusionArtifactId=junit-vintage-engine}
[WARNING]                     org.openrewrite.maven.RemoveExclusion: {groupId=org.springframework.boot, artifactId=spring-boot-starter-test, exclusionGroupId=junit, exclusionArtifactId=junit}
[WARNING]                 org.openrewrite.maven.UpgradeParentVersion: {groupId=org.springframework.boot, artifactId=spring-boot-starter-parent, newVersion=2.5.x}
[WARNING]             org.openrewrite.maven.UpgradeParentVersion: {groupId=org.springframework.boot, artifactId=spring-boot-starter-parent, newVersion=2.6.x}
[WARNING]         org.openrewrite.maven.UpgradeParentVersion: {groupId=org.springframework.boot, artifactId=spring-boot-starter-parent, newVersion=2.7.x, retainVersions=[mysql:mysql-connector-java]}
[WARNING] Changes have been made to src/main/java/cron/api/CronJobApiController.java by:
[WARNING]     org.openrewrite.java.spring.boot2.UpgradeSpringBoot_2_7
[WARNING]         org.openrewrite.java.spring.boot2.UpgradeSpringBoot_2_6
[WARNING]             org.openrewrite.java.spring.boot2.UpgradeSpringBoot_2_5
[WARNING]                 org.openrewrite.java.spring.boot2.UpgradeSpringBoot_2_4
[WARNING]                     org.openrewrite.java.spring.boot2.UpgradeSpringBoot_2_3
[WARNING]                         org.openrewrite.java.spring.boot2.UpgradeSpringBoot_2_2
[WARNING]                             org.openrewrite.java.spring.boot2.UpgradeSpringBoot_2_1
[WARNING]                                 org.openrewrite.java.spring.boot2.UpgradeSpringBoot_2_0
[WARNING]                                     org.openrewrite.java.spring.boot2.SpringBoot2BestPractices
[WARNING]                                         org.openrewrite.java.spring.NoAutowiredOnConstructor
[WARNING] Changes have been made to src/main/java/cron/api/VersionApiController.java by:
[WARNING]     org.openrewrite.java.spring.boot2.UpgradeSpringBoot_2_7
[WARNING]         org.openrewrite.java.spring.boot2.UpgradeSpringBoot_2_6
[WARNING]             org.openrewrite.java.spring.boot2.UpgradeSpringBoot_2_5
[WARNING]                 org.openrewrite.java.spring.boot2.UpgradeSpringBoot_2_4
[WARNING]                     org.openrewrite.java.spring.boot2.UpgradeSpringBoot_2_3
[WARNING]                         org.openrewrite.java.spring.boot2.UpgradeSpringBoot_2_2
[WARNING]                             org.openrewrite.java.spring.boot2.UpgradeSpringBoot_2_1
[WARNING]                                 org.openrewrite.java.spring.boot2.UpgradeSpringBoot_2_0
[WARNING]                                     org.openrewrite.java.spring.boot2.SpringBoot2BestPractices
[WARNING]                                         org.openrewrite.java.spring.NoAutowiredOnConstructor
[WARNING] Changes have been made to src/main/resources/application.properties by:
[WARNING]     org.openrewrite.java.spring.boot2.UpgradeSpringBoot_2_7
[WARNING]         org.openrewrite.java.spring.boot2.UpgradeSpringBoot_2_6
[WARNING]             org.openrewrite.java.spring.boot2.UpgradeSpringBoot_2_5
[WARNING]                 org.openrewrite.java.spring.boot2.UpgradeSpringBoot_2_4
[WARNING]                     org.openrewrite.java.spring.boot2.UpgradeSpringBoot_2_3
[WARNING]                         org.openrewrite.java.spring.boot2.UpgradeSpringBoot_2_2
[WARNING]                             org.openrewrite.java.spring.boot2.SpringBootProperties_2_2
[WARNING]                                 org.openrewrite.java.spring.ChangeSpringPropertyKey: {oldPropertyKey=logging.path, newPropertyKey=logging.file.path}
[WARNING]                     org.openrewrite.java.spring.boot2.SpringBootProperties_2_4
[WARNING]                         org.openrewrite.java.spring.ChangeSpringPropertyKey: {oldPropertyKey=logging.file.max-size, newPropertyKey=logging.logback.rollingpolicy.max-file-size}
[WARNING]                         org.openrewrite.java.spring.ChangeSpringPropertyKey: {oldPropertyKey=logging.file.max-history, newPropertyKey=logging.logback.rollingpolicy.max-history}
[WARNING] Changes have been made to src/test/java/cron/util/services/PreferenceServiceTest.java by:
[WARNING]     org.openrewrite.java.spring.boot2.UpgradeSpringBoot_2_7
[WARNING]         org.openrewrite.java.spring.boot2.UpgradeSpringBoot_2_6
[WARNING]             org.openrewrite.java.spring.boot2.UpgradeSpringBoot_2_5
[WARNING]                 org.openrewrite.java.spring.boot2.UpgradeSpringBoot_2_4
[WARNING]                     org.openrewrite.java.spring.boot2.SpringBoot2JUnit4to5Migration
[WARNING]                         org.openrewrite.java.spring.boot2.RemoveObsoleteSpringRunners
[WARNING]                             org.openrewrite.java.testing.junit5.JUnit5BestPractices
[WARNING]                                 org.openrewrite.java.testing.cleanup.TestsShouldNotBePublic
[WARNING] Changes have been made to src/test/java/cron/InitializeDefaultPreferencesTest.java by:
[WARNING]     org.openrewrite.java.spring.boot2.UpgradeSpringBoot_2_7
[WARNING]         org.openrewrite.java.spring.boot2.UpgradeSpringBoot_2_6
[WARNING]             org.openrewrite.java.spring.boot2.UpgradeSpringBoot_2_5
[WARNING]                 org.openrewrite.java.spring.boot2.UpgradeSpringBoot_2_4
[WARNING]                     org.openrewrite.java.spring.boot2.SpringBoot2JUnit4to5Migration
[WARNING]                         org.openrewrite.java.spring.boot2.RemoveObsoleteSpringRunners
[WARNING]                             org.openrewrite.java.testing.junit5.JUnit5BestPractices
[WARNING]                                 org.openrewrite.java.testing.cleanup.TestsShouldNotBePublic
[WARNING] Please review and commit the results.
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  01:36 min
[INFO] Finished at: 2023-10-05T02:25:45+02:00
[INFO] ------------------------------------------------------------------------

How are you running OpenRewrite?

Using the CLI command to upgrade to Spring Boot 2.7.x:

mvn -U org.openrewrite.maven:rewrite-maven-plugin:run \
  -Drewrite.recipeArtifactCoordinates=org.openrewrite.recipe:rewrite-spring:RELEASE \
  -Drewrite.activeRecipes=org.openrewrite.java.spring.boot2.UpgradeSpringBoot_2_7

What is the smallest, simplest way to reproduce the problem?

Have a test file with a test method that is public and does not use-tab indentation.

What did you expect to see?

No tabs in an exclusively space indented file. Also, i would prefer it to respect the existing formatting – our formatting guidelines put certain Junit5 annotations on the same line. Basically, i would have only needed to replace public with an empty string, like this:

-        @BeforeEach public void init() {
+        @BeforeEach void init() {
             MockitoAnnotations.openMocks(this);
         }

What did you see instead?

-        @BeforeEach public void init() {
+       @BeforeEach
+       void init() {
             MockitoAnnotations.openMocks(this);
         }

timtebeek commented 1 year ago

hi @Philzen ; thanks for reporting your findings here! Sorry to hear you're not getting the changes you had hoped for. Both appear related to our style detection and application. High over I can say we do our best to detect the most common style across a project (not just an individual file) and apply what we find to any code changes. It might then help to ensure your project uses a consistent style when it comes to indentation and where to place annotations on methods, if you haven't already.

In this case when we remove a modifier, we also apply our auto formatter on the remainder as seen here for classes https://github.com/openrewrite/rewrite-testing-frameworks/blob/8e1dffba4890771730f37c5ccfbaa0764f4c4754/src/main/java/org/openrewrite/java/testing/cleanup/TestsShouldNotBePublic.java#L126 And here for methods.

If the automatically detected style does not work for you, you can also pass in an explict style to ensure that any code changes produced as part of a recipe run use a particular format. Hope that helps!

Would you have any further questions that you want to keep this issue open for?

Philzen commented 1 year ago

Hi @timtebeek, thanks for your response! And BTW thanks to you and everybody for all your work on those OpenRecipes, they are a priceless support for those kinds of migrations.

For me, this issue is not really a showstopper as i carefully review the diffs after the recipe's have run, so it's just a little (sometimes more, depending on the project size) manual work involved to fix the diffs, but still i'm grateful about OpenRecipe identifying the points of action for me in the first place. I merely wanted to report back to the team that this recipe may not be working expected. So i'll leave it up to you to decide whether to close it.

Without understanding the internals of the format detection, maybe i can provide some context on the project(s) this happens on:

contains a lot of legacy code where some files use tabs. We usually convert the tabs to indents first whenever something needs to touched for consistent diffs, however there's a certain (unknown) ratio of tabs vs. spaces files used throughout the project (in some files it may even be mixed :see_no_evil: due to negligence).
the test file that this issue report happened on definitely was all spaces indentation, basically all test files consistently use spaces as i've authored all of them since i joined the project
since then, all project contain an .editorconfig that defines there should be 4 spaces per indent

So maybe this information could provide guidance for improving the format detection in those methods you referenced.

Would you have any further questions [...]?

Basically, having this mixed formats in the code-base continues to be a nuisance b/c typically one finds out about it in the middle of coding when we have already touched the files, so i was wondering if there is an automated tool to fix this once and for all. And of course, OpenRewrite in all its awesomeness seems to have us covered:

But from looking at those links i'm a bit lost on how to use them:

what is the difference between the two?
how do they work (will they respect an .editorconfig, Prettier config, or how do they detect what needs to be achieved)?

Of course, long story short:

how can i run an OpenRewrite recipe to convert any tab indentations to indents?
...so that it does nothing more than replace \t to , but only in source code files.
(bonus) is there also a (ideally separate) recipe to fix indentation style, such as adding required indents that where missed (like after the start of a switch statement or after {) or removing extraneous indents (such as too much indentation in a continued statement or before a } on a single line)?

knutwannheden commented 1 year ago

@timtebeek We should try to assert that the formatting auto-detection indeed is still part of the recipe run as implemented by the build plugins. At least as long as we offer that alternative.

timtebeek commented 9 months ago

Hi @Philzen ; sorry this took a while to get to; perhaps Slack is more convenient for follow up questions. :) To answer a quick few comments above:

It appears autoformat detection is still in the Maven plugin, and hadn't changed in the period before this issue was opened
Can say exactly what's the difference between those two tabs/indents normalization recipes, but the latter visitor implementation has more detail than the former. Perhaps we should compare and retain only one of those
Probably best to run org.openrewrite.java.format.TabsAndIndents , optionally with a configured style using our Yaml format; that should also know when to insert newlines as you indicated.

Thanks again for your support of the project!

timtebeek commented 9 months ago

I'll close this issue as it's not specific to the particular recipe in this module, and from the above it appears the mixed use of indentation styles appears to have factored in mostly, since we've not had any similar reports since.

openrewrite / rewrite-testing-frameworks