Skip to content

Conversation

@Anmol202005
Copy link
Collaborator

Part of: #41
Updated HeaderTest to extend AbstractRecipeTestSupport.

@Anmol202005
Copy link
Collaborator Author

Anmol202005 commented Jul 22, 2025

@rdiachenko
The cause of CI failure is:
How the format method works here:

sourceFile = sourceFile.withPrefix(
                            Space.format(fixedHeader + System.lineSeparator()));

format method treats \r and \n as separate values, therefore it fails in windows where System.lineSeprator() return \r\n and hence an extra space is added in each comment.

Will raise a separate PR for fix,
but i am confused how it worked earlier.
please share your insight.

i tried replicating the issue with AbstractRecipeTest where we manually provided the config but it worked well. i compared the header being passed to the main method it was exactly same.

@rdiachenko
Copy link
Member

@Anmol202005

Let's step back. Why do we need + System.lineSeparator() at all? I think we should rely on what is in the header file and just concatinate it.

Additionally, could you resolve conflicts please

@Anmol202005
Copy link
Collaborator Author

Anmol202005 commented Jul 23, 2025

Let's step back. Why do we need + System.lineSeparator() at all? I think we should rely on what is in the header file and just concatinate it.

@rdiachenko its necessary so that the code after the header starts from its own line.

Although IMO in the existing code there are two issues(bugs) that balances out.

  • use of System.lineseprator()
  • use of Files.readString() , to read input and output file.

AbstractPathTestSupport uses the following method to read files:

protected static String readFile(String filename) throws IOException {
        return toLfLineEnding(Files.readString(Path.of(filename)));
    }
    
protected static String toLfLineEnding(String text) {
        return text.replaceAll("(?x)\\\\r(?=\\\\n)|\\r(?=\\n)", "");
    }

to convert Windows-style line endings (\r\n) to Unix-style line endings (\n).

https://github.com/checkstyle/checkstyle/blob/master/src/test/java/com/puppycrawl/tools/checkstyle/AbstractPathTestSupport.java#L121

@Anmol202005
Copy link
Collaborator Author

@rdiachenko
Since the readFile() bug is already resolved in this PR, I prefer to address the header bug here as well, instead of making changes to AbstractRecipeTest in a separate PR. Moreover, since AbstractRecipeTest will be removed in this PR, it’s cleaner to handle all related changes together.

@Anmol202005 Anmol202005 force-pushed the addSupportToHeader branch 2 times, most recently from e52a3c0 to 3d3b783 Compare July 23, 2025 15:09
@Anmol202005
Copy link
Collaborator Author

@rdiachenko Done.

@rdiachenko rdiachenko self-assigned this Jul 23, 2025
Copy link
Member

@rdiachenko rdiachenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@rdiachenko rdiachenko merged commit 9279400 into checkstyle:main Jul 24, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants