Cleanup tests #3926

OlivierBlanvillain · 2018-01-26T14:34:20Z

This PR cleans up the tests, to summarize:

Activate all the passing tests from tests/pending/* and tests/untried/*
Disable all the the pending tests that use scala.reflect._, existential types and are depending on things from scala.tools.partest._
Clean up warnings from pending tests
Clean up all orphan .check and .flags files

This is best reviewed commit by commit

liufengyun · 2018-01-27T08:00:45Z

Need to rebase because of #3892

OlivierBlanvillain · 2018-01-30T16:49:27Z

@allanrenucci This is ready for review!

allanrenucci

Two remarks:

9cfe19b moves tests that fail under legacy. Why do they fail under legacy and not with vulpix?
I think a lot of tests in pending/run are not meant to be ran but only meant to check warnings. We should convert them to neg tests with -Xfatal-warnings. Also, until they are converted, we should keep their .flags

allanrenucci · 2018-01-30T17:08:53Z

compiler/test/dotty/tools/vulpix/ParallelTesting.scala

      case self: JointCompilationSource =>
        if (self.files.length > 1) name
        else self.files.head.getPath

      case self: SeparateCompilationSource =>
        self.dir.getPath
-    }
+    }).stripPrefix("../")


What about we change the working directory when running tests? Something like:

baseDirectory in test := baseDirectory in ThisBuild

We could get rid of all the "../ in CompilationTest

allanrenucci · 2018-01-30T17:11:31Z

tests/run/Meter.scala

@@ -69,7 +69,7 @@ object Test extends dotty.runtime.LegacyApp {


  println("x.hashCode: "+x.hashCode)
-  println("x == 1: "+(x == 1))
+  println("x == 1: "+false) // Values of types a.Meter and Int cannot be compared with == or !=


I would remove this line or comment it out

allanrenucci · 2018-01-30T17:11:39Z

tests/run/MeterCaseClass.scala

@@ -66,7 +66,7 @@ object Test extends dotty.runtime.LegacyApp {


  println("x.hashCode: "+x.hashCode)
-  println("x == 1: "+(x == 1))
+  println("x == 1: "+false) // Values of types a.Meter and Int cannot be compared with == or !=


I would remove this line or comment it out

allanrenucci · 2018-01-30T17:12:43Z

tests/run/literals.scala

@@ -90,10 +90,10 @@ object Test {
    check_success("1e1f == 10.0f", 1e1f, 10.0f)
    check_success(".3f == 0.3f", .3f, 0.3f)
    check_success("0f == 0.0f", 0f, 0.0f)
-    check_success("01.23f == 1.23f", 01.23f, 1.23f)
+    check_success("01.23f == 1.23f", /*0*/1.23f, 1.23f) // Non-zero numbers may not have a leading zero.


I would remove this line or comment it out

allanrenucci · 2018-01-30T17:43:01Z

compiler/test/dotty/tools/dotc/CompilationTests.scala

@@ -189,7 +189,6 @@ class CompilationTests extends ParallelTesting {
    compileFile("../tests/neg-custom-args/xfatalWarnings.scala", defaultOptions.and("-Xfatal-warnings")) +
    compileFile("../tests/neg-custom-args/i3561.scala", defaultOptions.and("-Xfatal-warnings")) +
    compileFile("../tests/neg-custom-args/pureStatement.scala", defaultOptions.and("-Xfatal-warnings")) +
-    compileFile("../tests/neg-custom-args/i3589-a.scala", defaultOptions.and("-Xfatal-warnings")) +


Nice! We support emitting switch on value classes. Maybe open an issue to figure out if this is expected or not.

OlivierBlanvillain · 2018-01-31T15:53:21Z

@allanrenucci

No idea about the tests that fail only on legacy or with dotty bootstrapped. But that fact that you see these in commits is just an artifact of the way I did moves things around: first move the tests that were green under sbt testCompilation, then fix what broke.
I don't see how the tests in pending/run would check warnings, then only tests that could potentially check warnings would be in pending/neg with the fatal warnings flag. I manually checked all the flags touched by 6a950cd, and none of them where neg/*.flags, so I think I didn't lose any warning related flags.
I tried the change in Build.scala that you suggested, let see if the CI is happy with it!

OlivierBlanvillain · 2018-01-31T17:51:48Z

@allanrenucci There is an unfortunate side effect of baseDirectory in Test := baseDirectory.value / "..", which is that .class at the root of the dotty repo will be picked up when compiling the tests. I'm not sure that's better than the original .stripPrefix("../") solution...

This is to simplify the scripts written in the next commits. I don't believe this separation brings a lot of value, so propose to just keep it that way.

Using the following script: find tests/untried/???/ tests/pending/???/ -maxdepth 1 -mindepth 1 -not -name "*.check" -not -name "*.flags" | while read t; do pref=$(echo "$t" | cut -f 1-2 -d "/") name=$(echo "$t" | rev | cut -f 1 -d "/" | rev) type=$(echo "$t" | cut -f 3 -d "/") mv "$t" "tests/$type" base=$(basename $name .scala) mv "$pref/$type/$base.check" "tests/$type" 2>/dev/null mv "$pref/$type/$base.flags" "tests/$type" 2>/dev/null echo ";testCompilation $name ;eval scala.tools.nsc.io.File(\"/home/olivier/works\").appendAll(\"$t\\n\")" done | sbt git stash cat /home/olivier/works | while read t; do # same loop as above

Some of these warnning are also emitted by dotc, but they are not captured by the testing infreastructure and shouldn't be part of check files.

I used the following script: mkdir filtered filtered/pos filtered/neg filtered/run find tests/untried/???/ tests/pending/???/ -name *.scala | while read t; do grep --silent '.partest.' "$t" && ( pref=$(echo "$t" | cut -f 1-2 -d "/") type=$(echo "$t" | cut -f 3 -d "/") name=$(echo "$t" | cut -f 4 -d "/") echo "$pref/$type/$name" mv "$pref/$type/$name" "filtered/$type" base=$(basename $name .scala) mv "$pref/$type/$base.check" "filtered/$type" 2>/dev/null mv "$pref/$type/$base.flags" "filtered/$type" 2>/dev/null ) done mv filtered tests/untried/partest With '.partest.', '.reflect.' and ' forSome {'.

Using the following script: find tests -name "*.check" -or -name "*.flags" | while read t; do name=$(echo "$t" | cut -f -1 -d .) test -f "$name.scala" -o -d "$name" || rm "$t" done

See tests/pos/i2104.scala

This reverts commit a9aaff8.

OlivierBlanvillain · 2018-02-02T16:15:53Z

@allanrenucci Review addressed and CI green, let me know what you think!

allanrenucci

LGTM. Thanks @OlivierBlanvillain for taking care of this tedious work

After scala#3926, test logs were outputted to the dotty project parent folder

OlivierBlanvillain requested a review from allanrenucci January 26, 2018 14:34

OlivierBlanvillain force-pushed the cleanup-tests branch 2 times, most recently from 8f6701c to aacd5ca Compare January 26, 2018 16:04

OlivierBlanvillain force-pushed the cleanup-tests branch from aacd5ca to 0891373 Compare January 27, 2018 10:12

allanrenucci self-assigned this Jan 29, 2018

OlivierBlanvillain force-pushed the cleanup-tests branch 4 times, most recently from 2835fdc to 1442757 Compare January 30, 2018 15:06

allanrenucci reviewed Jan 30, 2018

View reviewed changes

OlivierBlanvillain force-pushed the cleanup-tests branch 3 times, most recently from ef1bd27 to 9af5e08 Compare January 31, 2018 15:46

OlivierBlanvillain force-pushed the cleanup-tests branch from 9af5e08 to 68a67e1 Compare January 31, 2018 17:50

OlivierBlanvillain force-pushed the cleanup-tests branch 3 times, most recently from 80518fa to a3fa888 Compare February 2, 2018 11:16

OlivierBlanvillain added 9 commits February 2, 2018 14:19

Combine tests from /neg-with-implicits into /neg

9799018

This is to simplify the scripts written in the next commits. I don't believe this separation brings a lot of value, so propose to just keep it that way.

Fix "\n " pattern in all error messages

d27f9a6

Fix pending/run/{Meter,MeterCaseClass} tests

8cd6050

Fix pending/run/literals test

70766ba

Remove warnings from pending tests

1a80d78

Some of these warnning are also emitted by dotc, but they are not captured by the testing infreastructure and shouldn't be part of check files.

Fix t4047.scala

cbe2291

Strip "../" from test output

cd09060

OlivierBlanvillain added 13 commits February 2, 2018 14:19

Clean up all orphan .check and .flags files

f333cfb

Using the following script: find tests -name "*.check" -or -name "*.flags" | while read t; do name=$(echo "$t" | cut -f -1 -d .) test -f "$name.scala" -o -d "$name" || rm "$t" done

Update tests blacklists in FromTastyTests.scala

1d4a093

Fix vulpix to fail on error free neg tests

f9ef6b6

Remove flags

1d3480f

Remove duplicate test

cc1f9ba

See tests/pos/i2104.scala

Remove test that fails bootstrapped

895af78

Removes tests that fail under legacy

58ee725

Address review

0851931

Revert "Strip "../" from test output"

6ab970d

This reverts commit a9aaff8.

Set baseDirectory to ".." in tests

58b55fa

Remove . from test classpath

a24ccf3

Revert/fix warnings changes on pending/run tests

d2a590a

Factor out fatal-warnings & double-bindings tests

133b8ff

OlivierBlanvillain force-pushed the cleanup-tests branch from a3fa888 to 133b8ff Compare February 2, 2018 13:20

Merge branch 'master' into cleanup-tests

a7592b2

OlivierBlanvillain mentioned this pull request Feb 5, 2018

Regression introduced by #3918 #3963

Closed

Open scala#3963

8547e85

OlivierBlanvillain force-pushed the cleanup-tests branch from 41fda8f to 8547e85 Compare February 5, 2018 12:45

allanrenucci approved these changes Feb 5, 2018

View reviewed changes

OlivierBlanvillain merged commit e011862 into scala:master Feb 5, 2018

allanrenucci deleted the cleanup-tests branch February 5, 2018 14:57

allanrenucci added a commit to dotty-staging/dotty that referenced this pull request Feb 7, 2018

Fix test log folder location

48b7f22

After scala#3926, test logs were outputted to the dotty project parent folder

allanrenucci mentioned this pull request Feb 7, 2018

Fix test log folder location #3973

Merged

griggt mentioned this pull request Sep 25, 2020

Fix #9776: Don't issue a @switch warning when fewer than 4 cases #9852

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Cleanup tests #3926

Cleanup tests #3926

Uh oh!

OlivierBlanvillain commented Jan 26, 2018

Uh oh!

liufengyun commented Jan 27, 2018

Uh oh!

OlivierBlanvillain commented Jan 30, 2018

Uh oh!

allanrenucci left a comment

Uh oh!

allanrenucci Jan 30, 2018

Uh oh!

allanrenucci Jan 30, 2018

Uh oh!

allanrenucci Jan 30, 2018

Uh oh!

allanrenucci Jan 30, 2018

Uh oh!

allanrenucci Jan 30, 2018

Uh oh!

OlivierBlanvillain commented Jan 31, 2018 •

edited

Loading

Uh oh!

OlivierBlanvillain commented Jan 31, 2018

Uh oh!

OlivierBlanvillain commented Feb 2, 2018 •

edited

Loading

Uh oh!

allanrenucci left a comment

Uh oh!

Uh oh!

Cleanup tests #3926

Cleanup tests #3926

Uh oh!

Conversation

OlivierBlanvillain commented Jan 26, 2018

Uh oh!

liufengyun commented Jan 27, 2018

Uh oh!

OlivierBlanvillain commented Jan 30, 2018

Uh oh!

allanrenucci left a comment

Choose a reason for hiding this comment

Uh oh!

allanrenucci Jan 30, 2018

Choose a reason for hiding this comment

Uh oh!

allanrenucci Jan 30, 2018

Choose a reason for hiding this comment

Uh oh!

allanrenucci Jan 30, 2018

Choose a reason for hiding this comment

Uh oh!

allanrenucci Jan 30, 2018

Choose a reason for hiding this comment

Uh oh!

allanrenucci Jan 30, 2018

Choose a reason for hiding this comment

Uh oh!

OlivierBlanvillain commented Jan 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

OlivierBlanvillain commented Jan 31, 2018

Uh oh!

OlivierBlanvillain commented Feb 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

allanrenucci left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

OlivierBlanvillain commented Jan 31, 2018 •

edited

Loading

OlivierBlanvillain commented Feb 2, 2018 •

edited

Loading