[WIP] fix encoding issues #1494

martijnhoekstra · 2016-09-03T13:58:04Z

the -encoding compiler flag wasn't honored at all.

Behind a WIP flag, because it depends on the earlier PR with the other test fixes for Windows, as it's very hard to build something isolated when the tests are still failing, and will need to get rabased and cleaned up at some point.

smarter · 2016-09-04T21:11:24Z

tests/run/utf16encoded.flags

@@ -0,0 +1 @@
+-encoding UTF16


.flags file are currently ignored by partest, and I'm not sure if there's any way to have a run test with a special set of flags currently.
/cc @DarkDimius : If we make a run-special folder like pos-special, will its content be treated as run-tests by partest?

@smarter That makes me realize this doesn't actually have to be a run test, as it won't compile anyway if it doesn't have the -encoding flag set to UTF-16. Should it be moved?

Yes, you should move it to pos-special and add a line next to https://github.com/lampepfl/dotty/blob/d84805299a42cb8d2c756aff5a117af24dbeaaf4/test/dotc/tests.scala#L113 like:

@Test def pos_utf16encoded = compileFile(posSpecialDir, "utf16encoded", List("-encoding", "UTF16"))

rename test/pos/valueclasses to pos_valueclasses tests/pos/valueclasses generates a valueclasses.flags file in /tests/partest-generated/pos that conflicts with the valueClasses.flags file that tests/neg/valueClasses.scala tries to create

martijnhoekstra · 2016-09-08T11:01:02Z

And that's a passing test-suite.

I'm particularly uncertain about what I did at 32819e2

Should that be split up in the part where -encoding and default UTF8 encoding are honored, and the test renames? Should neg/valueClasses.scala be renamed rather than pos/valueclasses/?

I'm also not too happy with the manual bitwise copy, which seems out of place in 6bce106

sjrd · 2016-09-08T11:35:58Z

I'll have a closer look at this during the week-end, on Windows at home.

sjrd · 2016-09-11T09:26:19Z

test/test/TestREPL.scala

@@ -49,7 +49,7 @@ class TestREPL(script: String) extends REPL {
    out.close()
    val printed = out.toString
    val transcript = printed.drop(printed.indexOf(config.prompt))
-    if (transcript.toString != script) {
+    if (transcript.toString.lines.toList != script.lines.toList) {


You can avoid the reification as Lists with sameElements:

if (transcript.toString.lines.sameElements(script.lines))

sjrd · 2016-09-11T09:47:51Z

Why renaming some pos tests with pos_test names? That seems like a workaround to a deeper issue. We should fix the original issue instead.

sjrd · 2016-09-11T09:48:43Z

test/test/CompilerTest.scala

@@ -228,7 +228,7 @@ abstract class CompilerTest {

  private def expectedErrors(filePath: String): List[ErrorsInFile] = expectedErrors(List(filePath))

-  private def isNegTest(testPath: String) = testPath.contains(JFile.separator + "neg" + JFile.separator)
+  private def isNegTest(testPath: String) = testPath.contains("/neg/")


Should this change have been part of another commit?

32819e2 should probably be broken up in just the part that fixes dotty not honoring default and passed in -encoding arguments (which is an actual bug in dotty) and the parts that get the tests running on Windows (which is "just" a set of test-suite bugs). WDYT?

That sounds good :)

sjrd · 2016-09-11T09:55:12Z

test/test/CompilerTest.scala

+        out.close()
+      } else {
+        try {
+          SFile(dest)(scala.io.Codec.UTF8).writeAll((s"/* !!!!! WARNING: DO NOT MODIFY. Original is at: $file !!!!! */").replace("\\", "/"), file.slurp("UTF-8"))


Why the .replace("\\", "/")?

some path parts start with u. Then comment then contains \u(something), which gets interpreted as an invalid unicode escape, failing compilation.

Ah ah!
Well, that deserves to be written as a comment above this line, then, IMO.

sjrd · 2016-09-11T09:59:46Z

That's all I have.

Note: on my Windows machine, sbt test still fails on your branch. Is that expected?

[error] Failed: Total 166, Failed 19, Errors 0, Passed 147
[error] Failed tests:
[error]         dotc.tests
[error] (dotty/test:test) sbt.TestsFailedException: Tests unsuccessful

martijnhoekstra · 2016-09-11T10:07:03Z

No, on my Windows machine sbt test passes everything. I'll first get to work on your comments; that's plenty for now. Maybe after that we can see if there are failing tests in different environments?

felixmulder · 2016-10-06T09:40:27Z

As this improves the current state - we're merging this. Thanks for your contribution @martijnhoekstra !

The one-parameter constructor of SourceFile was removed in scala#1494

martijnhoekstra · 2016-10-22T09:23:14Z

@felixmulder thank you for merging it, and my sincere apologies for not coming back to this in a timely manner. I'll investigate the remainder of the cleanup that's still to be done, and make sure it gets solved.

felixmulder · 2016-10-22T09:25:51Z

No need to apologize, your contribution improved the project -- therefore we merged it, simple as that. We'd be very grateful for further improvements on the windows side, as nobody at the lab currently uses windows.

Cheers,
Felix

nicolasstucki added the stat:wip label Sep 4, 2016

smarter reviewed Sep 4, 2016
View reviewed changes

martijnhoekstra force-pushed the wintests branch 2 times, most recently from 8450ff0 to 663db29 Compare September 5, 2016 19:04

odersky assigned sjrd Sep 7, 2016

Martijn Hoekstra added 2 commits September 7, 2016 21:28

normalize paths for tests

2a2f314

ignore REPL transscript line ending differences

e104be1

martijnhoekstra force-pushed the wintests branch from 663db29 to 620394c Compare September 7, 2016 19:45

Martijn Hoekstra and others added 2 commits September 7, 2016 22:32

force UTF-8 for slurp and write

6bce106

honor -encoding compiler flag and defaults

32819e2

rename test/pos/valueclasses to pos_valueclasses tests/pos/valueclasses generates a valueclasses.flags file in /tests/partest-generated/pos that conflicts with the valueClasses.flags file that tests/neg/valueClasses.scala tries to create

martijnhoekstra force-pushed the wintests branch from 620394c to 32819e2 Compare September 7, 2016 20:46

martijnhoekstra mentioned this pull request Sep 7, 2016

some fixes for windows test failures #1493

Closed

test negtest path with / rather than path separator

46b3253

martijnhoekstra force-pushed the wintests branch from 5e8e2c4 to 46b3253 Compare September 8, 2016 10:33

sjrd reviewed Sep 11, 2016
View reviewed changes

felixmulder merged commit a306462 into scala:master Oct 6, 2016

smarter added a commit to smarter/dotty that referenced this pull request Oct 6, 2016

Fix build failure after merging scala#1492

cee862c

The one-parameter constructor of SourceFile was removed in scala#1494

smarter mentioned this pull request Oct 6, 2016

Fix build failure after merging #1492 #1561

Merged

smarter mentioned this pull request Jan 26, 2019

Tests failing on Windows #1480

Closed

[WIP] fix encoding issues #1494

[WIP] fix encoding issues #1494

Uh oh!

Conversation

martijnhoekstra commented Sep 3, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martijnhoekstra commented Sep 8, 2016

Uh oh!

sjrd commented Sep 8, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sjrd commented Sep 11, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sjrd commented Sep 11, 2016

Uh oh!

martijnhoekstra commented Sep 11, 2016

Uh oh!

felixmulder commented Oct 6, 2016

Uh oh!

martijnhoekstra commented Oct 22, 2016

Uh oh!

felixmulder commented Oct 22, 2016

Uh oh!

Uh oh!