Skip to content

Tests pin the formatter's pieces, not the routes users run #225

Open

Description

@abashev

What happens

The tests that pin what the formatter writes call pieces of it, not the routes users run, so two routes could drift apart without a test failing (#219).

Tests What they call What users run
FormatterCasesTest, 915 cases Formatter.formatSourceAndFixImports the Maven plugin and the Spotless step; the command line, the native binary and the GitHub Action go through FormatFileCallable.formatFile, a second implementation of the same steps
PartialFormattingTest, 60 tests Main --lines without --output-replacements, and Formatter.getFormatReplacements IntelliJ IDEA and formatDiff use --output-replacements and FormatterService.getFormatReplacements; before #220 no test ran --output-replacements
FormatterIntegrationTest, 2 pairs left formatSource and then StringWrapper.wrap, put together in the test no route

The cases also never reach the places where the routes differ: all 915 give their expected output with the imports fixed before formatting and after it. The one test that did, ModuleImportTest.keepsACommentBetweenThePartsOfTheModuleName, pinned what formatSourceAndFixImports wrote, which the command line never printed.

And the tests that need a newer JDK are skipped on JDK 21, where a local build runs: 14 skipped there against 2 on JDK 25. The first push of #220 passed locally and failed CI's JDK 25 to 28 builds on that ModuleImportTest.

Proposal

  1. One implementation. FormatFileCallable.formatFile calls the same method as formatSourceAndFixImports, with the flags of the command line, as google-java-format now does with one Formatter.formatSource(input, ranges, removeUnusedImports, sortImports, reflowLongStrings). Then the 915 cases test the command line.
  2. Cases for the seams. Four cases are ready for test-cases/imports: commentOfARemovedImportGoesWithTheImportAboveIt, commentOfARemovedOwnPackageImportGoesWithTheImportAboveIt, commentUnderARemovedImportStaysOnTheClass and semicolonAfterTheLastImportStaysOnALineOfItsOwn. Each passes with Format the same on every route: formatDiff on the JVM, ranges, and the order of fixing imports #220 and fails with the imports fixed first. What a case file cannot say yet, a failure, a JDK it needs, a range, stays in FormatterServiceImplTest and ModuleImportTest.
  3. Cases through the routes. FormatterCasesTest runs each case through Main, through FormatterServiceImpl, and in CI's native jobs through the native binary.
  4. Ranges on the cases. For each case, the lines where the code before and after differ are formatted as a range, as formatDiff does, through --output-replacements and through the in-process service, and the two compared; and formatting any range of the expected output leaves it as it is.
  5. A rule and a task. CLAUDE.md says that a test which pins the output users get goes through Main or FormatterService, while ImportOrderer, RemoveUnusedImports and StringWrapper keep their unit tests. A mise task runs the tests as CI's JDK 25 and 27 builds do, with their TestKit Gradle versions.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or request

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions