fix tests which failed on Windows because \r\n vs \n

[SethTisue]

with these changes, all of our JUnit tests are passing again on Windows.
(I'll deal with partest failures, if there any, in a separate PR.)

review by @retronym (thx for the string interpolator!)

[SethTisue]

attn @martijnhoekstra. thx for calling this to my attention

[som-snytt]

That's fine for tests, but the interpolator is a general (internal) facility. It might make more sense to have a normalizator for testing purposes (possibly normalizing test output and expected value).

Cf https://github.com/scala/scala/blob/v2.11.7/src/compiler/scala/tools/util/PathResolver.scala#L40

Utils for tests: https://github.com/scala/scala/tree/2.11.x/test/junit/scala/tools/testing

Typical normalizing: https://github.com/scala/scala/blob/2.11.x/src/partest-extras/scala/tools/partest/ReplTest.scala#L49

Similarly: https://github.com/scala/scala/blob/2.11.x/test/junit/scala/StringContextTest.scala#L68

[retronym]

There is a ready made implicit class importable from scala.reflect.internal.util.StringContextStripMarginOps.

scala> import scala.reflect.internal.util.StringContextStripMarginOps
import scala.reflect.internal.util.StringContextStripMarginOps

scala> sm"foo"
res0: String = foo

[retronym]

Maybe the mental tax of the escaped $ in these expectations is enough to prefer a the """...""".stripMargin idiom.

I think I prefer the idea of a custom assertEquivalentText method that is insensitive to line endings, and maybe to leading and trailing whitespace.

[SethTisue]

a number of the tests are contains based, so assertEquivalentText by itself wouldn't be enough. I'm not sure I agree that approach is preferable; I'd rather learn one interpolator and then use string processing and JUnit calls I already know, than have to learn several new calls, depending on exactly what kind of string-slinging I'm doing. at least, I don't think it's a clear win over what I did here.

I have pushed an additional commit that makes it so that import is always used to bring the interpolator into scope (including cleaning up one that wasn't mine).

fwiw, the interpolator was already in use in test/junit/scala/reflect/internal/PrintersTest.scala, so that's a precedent for using it in additional tests.

[retronym]

StripMarginInterpolator is used within the compiler itself. Changing its behaviour will introduce platform specific line endings into some error messages (perhaps partest is graciously ignoring that change, though.)

[SethTisue]

ah, that's a more serious objection. hmm...

EDIT: or not that serious? I assume it's not uncommon for people to sling both \n and println around (certainly this has been the case on other projects I've worked on :-) ), resulting in a mixture of \ns and \r\ns in the output.

[SethTisue]

Jason and I discussed this by video chat just now. He argues that we should 1) configure our Windows CI setup to check the files out with Unix line endings and 2) tell Windows developers experiencing test failures to do the same. Then no code changes are needed — not now, and not in the inevitable "long tail" of future incidents where somebody understandably forgets to use sm.

@martijnhoekstra how about just telling your git client to use LF endings on the Scala repo; does that resolve it for you?

[SethTisue]

I've verified on jenkins-worker-windows-publish that adding * text eol=lfto .gitattributes (and then following the "Refreshing a repository after changing line endings" instructions at https://help.github.com/articles/dealing-with-line-endings/#platform-windows) makes these tests pass on Windows, as one would expect.

[martijnhoekstra]

@SethTisue I'll double check if it resolves my issues as well, but seeing that it works for Jenkins it should also work for me. This is most likely the most pragmatic solution.

Now that I am looking in to this deeper, there are still newline related issues remaining that are probably not in the scope of this PR anymore, but it seems to be rather messy as a whole.

[SethTisue]

opened https://issues.scala-lang.org/browse/SI-9472 on this. closing this PR since it doesn't look like we'll be merging it.