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

EclipseCommonTests are no longer sensitive to line-ending problems. #300

Merged
merged 1 commit into from
Sep 12, 2018

Conversation

nedtwigg
Copy link
Member

FormatterStep are supposed to return unix line endings, but Formatter silently corrects for the case that they don't. To keep the windows build durable, we can resolve #298 by just making the tests ignore line ending problems.

@nedtwigg
Copy link
Member Author

Btw @simschla, with this merged, gradlew clean, gradlew build works on my windows 10 box.

@nedtwigg nedtwigg merged commit a2fe993 into master Sep 12, 2018
@nedtwigg nedtwigg deleted the feature/windowsEclipseCommonTests branch September 12, 2018 08:43
@jbduncan
Copy link
Member

@nedtwigg I doubt that this is the best fix. IIRC, formatter steps are meant to guarantee that their outputs have Unix line endings, however this test now liberally accepts both Unix and Windows line endings.

How about changing GrEclipseFormatterStep directly to return just Unix line endings instead?

@nedtwigg
Copy link
Member Author

formatter steps are meant to guarantee that their outputs have Unix line endings

Nope. They are guaranteed that their inputs will have unix line endings. It is suggested that their outputs have unix line endings, but we don't take their word for it, and we always convert them to unix ourselves no matter what. There are quite a few steps that misbehave - easier to put this burden on the infrastructure rather than the step themselves.

// Should already be unix-only, but some steps might misbehave.
unix = LineEnding.toUnix(formatted);

@nedtwigg
Copy link
Member Author

Also, this is how our existing test harnessing works:

public static StepHarness forStep(FormatterStep step) {
// We don't care if an individual FormatterStep is misbehaving on line-endings, because
// Formatter fixes that. No reason to care in tests either. It's likely to pop up when
// running tests on Windows from time-to-time
return new StepHarness(input -> LineEnding.toUnix(step.format(input, new File(""))));
}

@jbduncan
Copy link
Member

jbduncan commented Sep 12, 2018

Oh, I'd completely forgotten that Formatter has this line ending normalisation built in! Thank you for correcting me. 🙇

I don't have an IDE in front of me ATM to check, but I'm curious to know why StepHarness doesn't resolve this line ending issue on the testing side automatically for us. Do you know why?

@fvgh
Copy link
Member

fvgh commented Sep 12, 2018

@nedtwigg Thanks for the quick fix of my mistake.
@jbduncan I am afraid, each formatter is a little bit different. For JDT, for example, I explicitly use the com.diffplug.spotless.extra.eclipse.base.SpotlessEclipseFramework.LINE_DELIMITER.
Groovy and WTP using the (or kind of) org.eclipse.jface.text.TextUtilities.getDefaultLineDelimiter, which basically determines what the first delimiter in the existing document is, and uses it as default. Unfortunately the test value had no line delimiter... I was thinking in the past about mocking it, but decided it's to much digging in internals for little benefit, since Spotless anyway formats everything to UNIX first.

@nedtwigg
Copy link
Member Author

It's not a mistake :) There's no need for FormatStep to sanitize with LineEnding.toUnix() - we'll always sanitize in infrastructure anyway, so there's not even a perf advantage.

@fvgh
Copy link
Member

fvgh commented Sep 12, 2018

With "mistake", I was referring to the error in the test.
And since you added a "fast path" to LineEnding.toUnix checking whether changes are required, there is a performance gain. Just wanted to point out that the Eclipse formatter steps should behave correctly in nominal scenarios (can't imagine unformatted code without any line breaks at all).
Again, thanks for your quick fix ❤️

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.

GrEclipseFormatterStepTest fails on windows 10
3 participants