Skip to content

Conversation

@jord1e
Copy link
Contributor

@jord1e jord1e commented Jan 31, 2022

The printer was printing \r\n sequences instead of \n, because it was using PrintWriter#println

Fixes the "can print a schema as AST elements" test in SchemaPrinterTest.groovy on Windows

See
https://docs.oracle.com/javase/8/docs/api/java/io/PrintWriter.html#println--

The line separator string is defined by the system property line.separator, and is not necessarily a single newline character ('\n').

The printer was printing `\r\n` sequences instead of `\n`, because it was using `PrintWriter#println`

Fixes the `"can print a schema as AST elements"` test in `SchemaPrinterTest.groovy` on Windows

See
https://docs.oracle.com/javase/8/docs/api/java/io/PrintWriter.html#println--

> The line separator string is defined by the system property line.separator, **and is not necessarily a single newline character ('\n')**.
@bbakerman bbakerman added this to the 18.0 milestone Jan 31, 2022
@bbakerman
Copy link
Member

But why is \r\n wrong in Windows terms - isnt that what Windows programs want / handle?

@jord1e
Copy link
Contributor Author

jord1e commented Jan 31, 2022

Yes, for text programs, but the test "can print a schema as AST elements"

def "can print a schema as AST elements"() {

fails because Groovy multiline strings compile to use \n

You can see this when debugging and extracting the string to be compared to a variable
afbeelding
result is before the patch and contains \r at 9 places (coincidentally there are 9 extend types, this is how I found the bug)
expected is how the test is compiled by groovy (without \r's)

Notepad and other software correctly recognize \n line feeds
afbeelding

afbeelding

This behaviour is only present when using extend types. The regular SchemaPrinter and AstPrinter use \n everywhere, which is consistent across all operating systems

@jord1e
Copy link
Contributor Author

jord1e commented Jan 31, 2022

Now that I am reworking the CI with Windows support you can view the error:

https://github.com/jord1e/graphql-java/actions/runs/1773061590
gradle-test-summary-b8-t8-temurin-windows-latest, and open index.html

@bbakerman bbakerman merged commit 7258ece into graphql-java:master Feb 1, 2022
@bbakerman
Copy link
Member

thanks for the detailed explanation

@jord1e jord1e deleted the fix-ast-test-windows branch February 1, 2022 09:56
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