Operation Kill Partest (part 1) #2125

felixmulder · 2017-03-20T15:46:24Z

Partest is dead, long live [name for new testing "framework"]!

smarter · 2017-03-20T18:34:10Z

Awesome! I think this is missing one great feature from partest: A good summary report. When I scroll to the end of the logs I should be able to see at a glance:

What test methods failed
In each failed test method, what files failed to compile/run
The exact command-line invocation I should use to run this test manually

A related feature is log files, which partest also outputs (but delete for non-failed tests), when the compiler output is really huge, it's very helpful to be able to navigate a log file instead of scrolling for hours. Also related: partest keeps output directories for failed run tests, this makes it easy to inspect them with javap or a decompiler to figure out what's going on.

smarter · 2017-03-20T18:41:15Z

Also, I know this was already the case but it's really bugging me now: can we fix dotty.tools.dottydoc.TestWhitelistedCollections so that its output does not fill most of the log? I'm talking about all the:

Compiling (1/546): Platform.scala
Compiling (2/546): ScalaBeanInfo.scala
Compiling (3/546): BeanInfoSkip.scala
...

smarter · 2017-03-20T18:43:26Z

Basically all tests that instantiate a compiler should use a custom reporter that writes things to a logfile, that logfile should be kept only if specified by the user or if the test failed.

felixmulder · 2017-03-20T18:51:55Z

Awesome! I think this is missing one great feature from partest: A good summary report. When I scroll to the end of the logs I should be able to see at a glance:

Sure

Also, I know this was already the case but it's really bugging me now: can we fix dotty.tools.dottydoc.TestWhitelistedCollections so that its output does not fill most of the log? I'm talking about all the:

Ah huh

Basically all tests that instantiate a compiler should use a custom reporter that writes things to a logfile, that logfile should be kept only if specified by the user or if the test failed.

Mhm

All good ideas, I'll get them in tomorrow.

smarter · 2017-03-20T19:17:42Z

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

+      ),
+      defaultOptions.and("-Xprompt")
+    )
+  }.times(2).pos()


Did you try making a -Ytimes option instead, as we discussed recently?

No - let's discuss this afternoon

… a real one Previously, we sometimes ended up forcing a companion class symbol from a previous run or from the classpath which lead to weird issues like in `false-companion`. Even if we end up not forcing such a symbol, its presence can still lead to issue: before this commit incremental compilation of `dotty-compiler-bootstrapped` was broken because we recorded a false dependency on the non-bootstrapped `dotty-compiler` jar. The added test is currently marked pending because it does not work with JUnit (which doesn't handle separate compilation), only partest. I didn't managed to get it to work right, and this won't be necessary once our testing framework is overhauled by scala#2125 anyway, so I'll just have to remember to enable this test afterwards.

DarkDimius · 2017-03-27T12:12:54Z

@felixmulder, is this PR ready for review?

felixmulder · 2017-03-27T12:17:11Z

@DarkDimius - yes mostly, I have two things left to add before I want this merged:

being able to run a single test / or set of files
testing output of meta tests

maybe the first one will affect the implementation somewhat, but the second one most likely won't. If you want to review it now that would be great, but you can also wait until I've fixed the first point. Up to you 🙂

felixmulder · 2017-03-27T16:07:01Z

@DarkDimius - done with point 1

smarter · 2017-03-28T10:51:42Z

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

+        if (seen.contains(elem.getMethodName)) false
+        else { seen += elem.getMethodName; true }
+      }
+      .take(6).find { elem =>


Magic number?

sort of magic, we shouldn't go through more than a few levels of indirection... but maybe I should take out the magic number

odersky · 2017-03-29T11:13:57Z

Currently there is a check of creation dates. The compiler always picks the file that's newer. It has been done like this since the days of javac and I think it's a good strategy to keep.

smarter · 2017-03-29T11:15:03Z

@DarkDimius Compiling is not the problem, the problem is that we use the same JVM to classload the compiler output for run tests, so if you classload Test and Test tries to use your custom implementation of List, it will fail because an existing List has already been classloaded.

DarkDimius · 2017-03-29T11:15:42Z

@odersky, yes, that what I've meant by an old copy. Didn't know javac did the same.

DarkDimius · 2017-03-29T11:18:39Z

@smarter, JVM doesn't have global notion of classloading. It has class loading - per class loader.
The fact that another classloader in the same VM has already classloaded a class is unrelated.

By setting up classloaders correctly you should be able to get behavior that custom implementation of the List will be preferred.
Here's an example of how this is done. http://stackoverflow.com/questions/5445511/how-do-i-create-a-parent-last-child-first-classloader-in-java-or-how-to-overr

smarter · 2017-03-29T11:20:53Z

@DarkDimius Yes you can do really crazy things if you try hard enough, but I don't think it's worth it since we'll go to multiple JVMs in the future. Also consider that currently the standard collection is on the bootclasspath, so I'm not even sure if you can override that.

DarkDimius · 2017-03-29T11:26:14Z

@smarter, I wouldn't consider 16 lines of trivial implementation code in java as "crazy things if you try hard enough". This is a lot simpler than things we do everyday here.

I don't think this is going to be fixed by going multiple JVM approach if you'll reuse the VMs.
For the same reason: you'll need to have separate classpath for every test. All of those tests share stdlib, but have classes that may take priority over stdlib.

smarter · 2017-03-29T11:35:38Z

I don't think we'll need a custom ClassLoader implementation if we use multiple JVMs even if they're persistent. We can just launch the JVM with nothing from scala in the classpath, and everytime we want to run a test we ask the JVM to create a new URLClassLoader with the complete classpath we want to use and classload Test using it.

felixmulder · 2017-03-29T12:56:45Z

Can we agree to put this classloading business into part 2 of killing partest? 🙂

DarkDimius · 2017-03-29T12:59:00Z

Given that it became such a contentious topic - yes.
There's one more question left: #2125 (comment)

felixmulder · 2017-03-29T13:00:05Z

Let's remove partest after having this run for about a week or so alongside the new suite.

DarkDimius · 2017-03-29T13:01:26Z

I agree, but doesn't this pr remove partest from drone.yml?

felixmulder · 2017-03-29T13:04:09Z

.drone.yml

+    - ;set testOptions in LocalProject("dotty-compiler") += Tests.Argument(TestFrameworks.JUnit, "--exclude-categories=dotty.tools.dotc.ParallelTesting") ;test ;dotty-bin-tests/test
+    - ;set testOptions in LocalProject("dotty-compiler-bootstrapped") += Tests.Argument(TestFrameworks.JUnit, "--exclude-categories=dotty.tools.dotc.ParallelTesting") ;publishLocal ;dotty-bootstrapped/test
+    - ;set testOptions in LocalProject("dotty-compiler") += Tests.Argument(TestFrameworks.JUnit, "--exclude-categories=dotty.tools.dotc.ParallelTesting") ;partest-only-no-bootstrap --show-diff --verbose
+    - ;set testOptions in LocalProject("dotty-compiler-bootstrapped") += Tests.Argument(TestFrameworks.JUnit, "--exclude-categories=dotty.tools.dotc.ParallelTesting") ;partest-only --show-diff --verbose


@DarkDimius - here you can see that partest is still included, with the new unit tests filtered out.

Once we want to remove partest from the CI these, lines dissappear and the only two left will be:

- ;test ;dotty-bin-tests/test - ;publishLocal ;dotty-bootstrapped/test

@felixmulder, thank you for clarification.

felixmulder · 2017-03-29T13:06:35Z

Alright - before we merge this. Can we decide upon a strategy for the memory settings?

I have not been able to see much of a difference when altering them from the current settings. Especially now that partest is kept.

DarkDimius · 2017-03-29T13:11:36Z

For metaspace, the current change I believe is unnecessary.
@felixmulder, I propose we postpone this decision till we decide what to do with classloaders.
The approach proposed by @smarter requires substantially more metaspace & code cache memory for JIT. I won't be surprised if it will simply disable some parts of JIT due to too aggressive classloading.

I would expect my proposed approach to be able to fit in <1gb of metaspace and <256mb of code-cache with a lot of spare room and be a lot more JIT friendly.

It is necessary to do this in a new commit because of having to make sure that the signed drone file is current

… a real one Previously, we sometimes ended up forcing a companion class symbol from a previous run or from the classpath which lead to weird issues like in `false-companion`. Even if we end up not forcing such a symbol, its presence can still lead to issue: before this commit incremental compilation of `dotty-compiler-bootstrapped` was broken because we recorded a false dependency on the non-bootstrapped `dotty-compiler` jar. The added test is currently marked pending because it does not work with JUnit (which doesn't handle separate compilation), only partest. I didn't managed to get it to work right, and this won't be necessary once our testing framework is overhauled by scala#2125 anyway, so I'll just have to remember to enable this test afterwards.

felixmulder added stat:wip area:infrastructure labels Mar 20, 2017

felixmulder changed the title ~~Kill partest~~ [WIP] Kill partest Mar 20, 2017

felixmulder changed the title ~~[WIP] Kill partest~~ Kill partest Mar 20, 2017

felixmulder requested a review from DarkDimius March 20, 2017 16:24

smarter reviewed Mar 20, 2017

View reviewed changes

SethTisue mentioned this pull request Mar 20, 2017

convert partests that only need the reference compiler to be run by JUnit instead scala/scala-dev#295

Closed

felixmulder force-pushed the topic/kill-partest branch from 0d3adda to 7df4465 Compare March 21, 2017 16:16

felixmulder mentioned this pull request Mar 22, 2017

Fix varargs in methods #2135

Merged

felixmulder force-pushed the topic/kill-partest branch 2 times, most recently from 1978085 to da8e352 Compare March 23, 2017 13:23

smarter mentioned this pull request Mar 23, 2017

Fix #2137: Create dummy companions for top-level objects without a real one #2139

Merged

felixmulder force-pushed the topic/kill-partest branch 2 times, most recently from b669dcb to b66f5d3 Compare March 27, 2017 11:25

felixmulder force-pushed the topic/kill-partest branch from 4809abc to b581ca1 Compare March 27, 2017 12:15

felixmulder force-pushed the topic/kill-partest branch 5 times, most recently from c2b606b to 4eb8cba Compare March 27, 2017 15:09

felixmulder force-pushed the topic/kill-partest branch from 012a215 to a28c497 Compare March 28, 2017 09:43

smarter reviewed Mar 28, 2017

View reviewed changes

felixmulder force-pushed the topic/kill-partest branch from 2b495fb to 73ad257 Compare March 29, 2017 11:29

felixmulder added a commit to dotty-staging/dotty that referenced this pull request Mar 29, 2017

Address reviewer feedback on scala#2125

4527e5b

felixmulder added 4 commits March 29, 2017 13:34

Address reviewer feedback on scala#2125

7d8303d

Make run tests run even if there isn't a check file

af4bb48

Add test to check failure of non-tail recursive

c8321d6

Preserve stack trace when invoking main in run tests

8dc162c

felixmulder added 2 commits March 29, 2017 13:36

Fix test reporting for exceptions in compiler

b5b6f5e

Make summary report come at the end of test suite

02ebe0f

felixmulder force-pushed the topic/kill-partest branch from 73ad257 to 02ebe0f Compare March 29, 2017 12:52

felixmulder commented Mar 29, 2017

View reviewed changes

DarkDimius approved these changes Mar 29, 2017

View reviewed changes

Revert CI memory changes

08d75b5

It is necessary to do this in a new commit because of having to make sure that the signed drone file is current

felixmulder removed the stat:wip label Mar 29, 2017

felixmulder merged commit 4757a6b into scala:master Mar 29, 2017

felixmulder deleted the topic/kill-partest branch March 29, 2017 13:39

DarkDimius mentioned this pull request Apr 27, 2017

[Vulpix] Abort compilation after maxCompileTime #2318

Closed

Operation Kill Partest (part 1) #2125

Operation Kill Partest (part 1) #2125

Uh oh!

Conversation

felixmulder commented Mar 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smarter commented Mar 20, 2017

Uh oh!

smarter commented Mar 20, 2017

Uh oh!

smarter commented Mar 20, 2017

Uh oh!

felixmulder commented Mar 20, 2017

Uh oh!

smarter Mar 20, 2017

Choose a reason for hiding this comment

Uh oh!

felixmulder Mar 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DarkDimius commented Mar 27, 2017

Uh oh!

felixmulder commented Mar 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

felixmulder commented Mar 27, 2017

Uh oh!

smarter Mar 28, 2017

Choose a reason for hiding this comment

Uh oh!

felixmulder Mar 28, 2017

Choose a reason for hiding this comment

Uh oh!

odersky commented Mar 29, 2017

Uh oh!

smarter commented Mar 29, 2017

Uh oh!

DarkDimius commented Mar 29, 2017

Uh oh!

DarkDimius commented Mar 29, 2017

Uh oh!

smarter commented Mar 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DarkDimius commented Mar 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smarter commented Mar 29, 2017

Uh oh!

felixmulder commented Mar 29, 2017

Uh oh!

DarkDimius commented Mar 29, 2017

Uh oh!

felixmulder commented Mar 29, 2017

Uh oh!

DarkDimius commented Mar 29, 2017

Uh oh!

felixmulder Mar 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DarkDimius Mar 29, 2017

Choose a reason for hiding this comment

Uh oh!

felixmulder commented Mar 29, 2017

Uh oh!

DarkDimius commented Mar 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

felixmulder commented Mar 20, 2017 •

edited

Loading

felixmulder Mar 21, 2017 •

edited

Loading

felixmulder commented Mar 27, 2017 •

edited

Loading

smarter commented Mar 29, 2017 •

edited

Loading

DarkDimius commented Mar 29, 2017 •

edited

Loading

felixmulder Mar 29, 2017 •

edited

Loading

DarkDimius commented Mar 29, 2017 •

edited

Loading