depend on junit from partest, not vice versa. and reduce warnings #7817

SethTisue · 2019-03-05T01:14:25Z

three commits:

first commit reverses the dependency relationship between our junit and partest subprojects
second commit converts an example test from JUnit to partest
third commit reduces build-time noise in the junit subproject

the unifying theme here is warning elimination. the second commit converts some warnings from build noise to something that actually gets tested. the first commit started as just something the second commit needed, but then it turned out to be the nicest part.

SethTisue · 2019-03-05T01:34:22Z

also tried out this new "draft pull request" feature. it seems that on a draft PR, Jenkins triggers but Travis-CI does not. (but once you convert it to a regular pull request, Travis does then trigger.)

SethTisue · 2019-03-05T02:25:41Z

admittedly the new dependency is still kind of gross (it might be nice to eventually split up test/junit so that the testing-utils code is in a separate directory from the actual tests, that would make it less gross), but meh, I decided my ambitions for this particular PR stopped before that point

the new dependency may be gross, but the old one was considerably grosser IMO

SethTisue · 2019-03-05T02:38:57Z

test/junit/scala/reflect/FieldAccessTest.scala

    private val x = 123
-    // Uncommenting the following line would make the test fail
-    () => x
+    def fn = () => x


I checked using the 2.11.12 REPL, this alternate form of the test does still fail in 2.11.x

SethTisue · 2019-03-05T05:49:41Z

there seems to be a problem with t6411a intermittently failing. very difficult for me to see what it might have to do with the changes in this PR

the failures look like

@@ -61,7 +61,7 @@ result = 2
 meth = method zi_2
 as seen by Scala reflection: def zi_2(z: Z[Int]): Int
 as seen by Java reflection: public int a$.zi_2(Z)
-result = class java.lang.ClassCastException: java.lang.String cannot be cast to java.lang.Integer
+result = class java.lang.ClassCastException: null
 meth = method zs_3
 as seen by Scala reflection: def zs_3(z: Z[String]): String
 as seen by Java reflection: public java.lang.String a$.zs_3(Z)

which seems like a JVM-level problem

som-snytt

Partest was a baggy monster when it used actors as in scala.actor. More like, "tis an unweeded garden that grows to seed: things rank and gross in nature possess it merely."

IIRC, I recently copied some testing code for a partest test. If I can find it, I'll retrofit it.

test/junit/scala/tools/nsc/interpreter/CompletionTest.scala

test/files/run/junitForwarders/Test.java

test/files/run/WeakHashSetTest.scala

test/junit/scala/collection/SetMapConsistencyTest.scala

SethTisue · 2019-03-05T19:50:27Z

gah well this test (6411a) is now very flaky on CI, it fails most of the time

locally, it's the opposite: the test nearly always passes when run in isolation, but I did see it fail once that way

I'll have to dig deeper into this 😞

som-snytt · 2019-03-05T20:05:50Z

The run tests run sequentially but not on the same thread. Since reflection isn't thread-safe, anything is possible. You added a run test, maybe that was enough to disturb or perturb the delicate balance.

I haven't drilled into how the message in the class cast exception could be unset.

Or, the test is testing that reflective invocation fails at (Integer)i in BoxesRunTime.unboxToInt, maybe something weird happens in class initialization?

Actually, some poor slob on SO also saw this behavior. Maybe it's just something that happens in Hotspot and you're not guaranteed to see the message.

With a test class, I can retain the exception message by avoiding compilation with -XX:CompileCommand=exclude, but it didn't work right away under partest; the other solution is to force the two tests to run forked by supplying a .javaopts file. I'll try that and report the result.

Edit: fork with exclusion from compilation at #7819

SethTisue · 2019-03-06T01:51:28Z

rebased to begin with Som's commit from #7819. if all seems well, we'll merge #7819, then I'll remove the commit again from this PR and rebase.

SethTisue · 2019-03-06T02:15:37Z

the CLA checker is confused, I've opened a ticket on it: scala/scabot#103

SethTisue · 2019-03-06T03:12:25Z

@sjrd might this mess with Scala.js's use of partest? (like because the partest JAR will be missing the classes from the junit subproject, something like that?)

sjrd · 2019-03-06T06:56:02Z

We use fixed versions of partest itself, so at least it's not going to break Scala.js right now. It might make it more difficult for us to upgrade our partest later, but I don't think so.

the dependency the other way just seemed wrong to me. partest is a big hairy legacy mess, other things should not depend on it but also, it's convenient in partests to be able to: * use JUnit assertions * use utility code in scala.tools.testing in making this change, I only turned up one case where the dependency of junit on partest was being used, namely ASMConverters. this was easy to fix by relocating it to scala.tools.testing

there were a bunch of compiler warnings when compiling this test so, two motivations for the change: * reduce build-time noise * actually test that the warnings we want (or don't want) emitted for this code are (or aren't emitted) as a bonus, it demonstrates the utility of having our partest subproject depend on our junit subproject, rather than vice versa

not touching the 680 deprecation warnings right now, but this commit eliminates the others the MethodSymbol one is as per 2014 Som at https://stackoverflow.com/a/23274960/86485

SethTisue · 2019-03-06T17:15:31Z

I'll merge tomorrow if there's no further feedback by then.

In the junit project, I'm still considering whether to tackle separating the shared sources from the actual tests, but if I do, it will be a separate PR.

sequel to scala#7817 this is needed in order to avoid using "compile->test" which we can't replicate in the IntelliJ project (we don't think) it's better this way anyway, the old partest->junit dependency was unnecessarily thick, this replaces it with a thin dependency. I had strongly considered doing this anyway as part of scala#7817

SethTisue · 2019-03-11T17:25:54Z

In the junit project, I'm still considering whether to tackle separating the shared sources from the actual tests, but if I do, it will be a separate PR.

didn't take long for the need to arise. sequel in #7847

sequel to scala#7817 this is needed in order to avoid using "compile->test" which we can't replicate in the IntelliJ project (we don't think) it's better this way anyway, the old partest->junit dependency was unnecessarily thick, this replaces it with a thin dependency. I had strongly considered doing this anyway as part of scala#7817

thanks Martijn for catching it and Som for digging in a bit: scala@d25f845#r38613066

scala-jenkins added this to the 2.13.1 milestone Mar 5, 2019

SethTisue force-pushed the junit-vs-partest branch from 37e273f to fb0e9cb Compare March 5, 2019 01:47

SethTisue marked this pull request as ready for review March 5, 2019 01:47

SethTisue requested a review from som-snytt March 5, 2019 02:16

SethTisue changed the title ~~depend on junit from partest, not vice versa~~ depend on junit from partest, not vice versa. and reduce warnings Mar 5, 2019

SethTisue commented Mar 5, 2019

View reviewed changes

SethTisue force-pushed the junit-vs-partest branch 2 times, most recently from 520c3a9 to f8b7837 Compare March 5, 2019 04:25

This comment has been minimized.

Sign in to view

som-snytt approved these changes Mar 5, 2019

View reviewed changes

test/junit/scala/tools/nsc/interpreter/CompletionTest.scala Outdated Show resolved Hide resolved

test/files/run/junitForwarders/Test.java Show resolved Hide resolved

test/files/run/WeakHashSetTest.scala Show resolved Hide resolved

som-snytt reviewed Mar 5, 2019

View reviewed changes

test/junit/scala/collection/SetMapConsistencyTest.scala Show resolved Hide resolved

adriaanm modified the milestones: 2.13.1, 2.13.0-RC1 Mar 5, 2019

This comment has been minimized.

Sign in to view

SethTisue added the WIP label Mar 5, 2019

SethTisue modified the milestones: 2.13.0-RC1, 2.13.1 Mar 5, 2019

SethTisue force-pushed the junit-vs-partest branch from f8b7837 to e8572a0 Compare March 6, 2019 01:50

SethTisue mentioned this pull request Mar 6, 2019

CLA checker gets confused by multi-person PRs? scala/scabot#103

Open

SethTisue mentioned this pull request Mar 6, 2019

Avoid CCE with no message #7819

Merged

SethTisue removed the WIP label Mar 6, 2019

SethTisue modified the milestones: 2.13.1, 2.13.0-RC1 Mar 6, 2019

SethTisue force-pushed the junit-vs-partest branch from e8572a0 to 2ef0924 Compare March 6, 2019 03:02

SethTisue requested a review from sjrd March 6, 2019 03:06

sjrd approved these changes Mar 6, 2019

View reviewed changes

SethTisue added 3 commits March 6, 2019 07:26

reduce build noise in junit subproject

d25f845

not touching the 680 deprecation warnings right now, but this commit eliminates the others the MethodSymbol one is as per 2014 Som at https://stackoverflow.com/a/23274960/86485

SethTisue force-pushed the junit-vs-partest branch from 2ef0924 to d25f845 Compare March 6, 2019 15:26

SethTisue merged commit 7cfa084 into scala:2.13.x Mar 7, 2019

SethTisue deleted the junit-vs-partest branch March 7, 2019 15:24

SethTisue mentioned this pull request Mar 11, 2019

split out shared testing code into new subproject #7847

Merged

SethTisue mentioned this pull request Aug 27, 2019

Move files of utilities in the testkit package to the testkit project #8378

Merged

SethTisue added a commit to SethTisue/scala that referenced this pull request May 7, 2020

fix a tiny mistake in scala#7817

d5e1c65

thanks Martijn for catching it and Som for digging in a bit: scala@d25f845#r38613066

SethTisue mentioned this pull request May 7, 2020

fix a tiny mistake in #7817 #8967

Closed

dwijnand mentioned this pull request Aug 14, 2020

[nomerge][2.12] Welcome back Partest! #9169

Merged

SethTisue mentioned this pull request Jul 22, 2021

2.12.x: Windows CI (GHA): add JDK 17 (alongside 8) #9678

Merged

depend on junit from partest, not vice versa. and reduce warnings #7817

depend on junit from partest, not vice versa. and reduce warnings #7817

Uh oh!

Conversation

SethTisue commented Mar 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SethTisue commented Mar 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SethTisue commented Mar 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SethTisue Mar 5, 2019

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

SethTisue commented Mar 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

som-snytt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

SethTisue commented Mar 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

som-snytt commented Mar 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SethTisue commented Mar 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SethTisue commented Mar 6, 2019

Uh oh!

SethTisue commented Mar 6, 2019

Uh oh!

sjrd commented Mar 6, 2019

Uh oh!

SethTisue commented Mar 6, 2019

Uh oh!

SethTisue commented Mar 11, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

SethTisue commented Mar 5, 2019 •

edited

Loading

SethTisue commented Mar 5, 2019 •

edited

Loading

SethTisue commented Mar 5, 2019 •

edited

Loading

SethTisue commented Mar 5, 2019 •

edited

Loading

SethTisue commented Mar 5, 2019 •

edited

Loading

som-snytt commented Mar 5, 2019 •

edited

Loading

SethTisue commented Mar 6, 2019 •

edited

Loading