Killing partest part 2 - Enter the Vulpix #2194

felixmulder · 2017-04-05T08:52:44Z

fixes: #2183
fixes: #2210

felixmulder · 2017-04-05T08:54:20Z

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


-  def testFilter: Option[Regex] = sys.props.get("dotty.partest.filter").map(r => new Regex(r))
+  def maxDuration = 180.seconds


Note - we have some tests that run for quite a long time. In most cases these are benchmarks. But there are some which aren't and run for up to 60 seconds.

I propose to have a split between these run tests and the regular run tests in:

./tests/run-long/

And have an interface for them which allows for longer execution.

smarter · 2017-04-05T13:16:27Z

compiler/test/dotty/tools/dotc/vulpix/VulpixTests.scala

@@ -1,13 +1,14 @@
 package dotty
 package tools
 package dotc
+package vulpix


I'm wondering if it shouldn't just be dotty.tools.vulpix :)

smarter · 2017-04-05T13:20:49Z

compiler/test/dotty/tools/dotc/vulpix/RunnerOrchestration.scala

+import scala.concurrent.duration.Duration
+import scala.collection.mutable
+
+trait RunnerOrchestration {


Could you add some documentation explaining what this is and how it works here?

smarter · 2017-04-05T13:22:31Z

compiler/test/dotty/tools/dotc/vulpix/RunnerOrchestration.scala

+
+  private[this] val monitor = new RunnerMonitor
+
+  private class RunnerMonitor {


Docs here too plz

smarter · 2017-04-05T13:26:48Z

compiler/test/dotty/tools/dotc/vulpix/RunnerOrchestration.scala

+    private[this] val busyRunners = mutable.Set.empty[Runner]
+
+    private def getRunner(): Runner = synchronized {
+      while (freeRunners.isEmpty) wait()


This looks correct, but I've forgotten everything I ever knew about Java concurrency, are you sure there's no existing higher-level concurrency thing from https://docs.oracle.com/javase/8/docs/api/?java/util/concurrent/package-summary.html you could use instead of the wait/notify dance ?

smarter · 2017-04-05T13:29:37Z

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

-  def testFilter: Option[Regex] = sys.props.get("dotty.partest.filter").map(r => new Regex(r))
+  def maxDuration = 3.seconds
+  def numberOfSlaves = 5
+  def safeMode = sys.env.get("SAFEMODE").isDefined


I would prefer dotty.tests.safemode

smarter · 2017-04-05T13:30:01Z

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

+  def numberOfSlaves = 5
+  def safeMode = sys.env.get("SAFEMODE").isDefined
+  def isInteractive = SummaryReport.isInteractive
+  def testFilter = sys.props.get("dotty.partest.filter").map(r => new Regex(r))


And here dotty.tests.filter, that way all the properties related to testing are in the dotty.tests namespace

By the way, we should have documentation for all these properties in one place, and it would be good if we limited string handling to one place too, I suggest having something like:

object DottyTestsProperties { // *Documentation of what safeMode is* def safeMode = sys.props.get("dotty.partest.filter") ... }

smarter · 2017-04-05T13:34:58Z

compiler/test/dotty/tools/dotc/vulpix/Statuses.java

+        public Failure(String output) { this.output = output; }
+    }
+
+    static class Timeout implements Status, Serializable {}


Is this something that a child JVM can send? Otherwise it should not be part of Status assuming Status defines the communication protocol between the JVMs.

smarter · 2017-04-05T13:35:23Z

compiler/test/dotty/tools/dotc/vulpix/RunnerOrchestration.scala

-    private def createProcess: Process = ???
+    private def createProcess: Process = {
+      val sep = sys.props("file.separator")
+      val cp = sys.props("java.class.path")


This makes me sad, but I won't disallow that for now.

smarter · 2017-04-05T13:37:13Z

compiler/test/dotty/tools/dotc/vulpix/ChildMain.scala

+  private def runMain(dir: JFile): Status = {
+    def renderStackTrace(ex: Throwable): String =
+      ex.getStackTrace
+        .takeWhile(_.getMethodName != "invoke0")


What is this for? I think I'd be happier not having my stack traces mangled in some undocumented way, what if I have a test with an invoke0 method?

smarter

Would be good to add some of the meta-tests proposed by @DarkDimius: #2125 (review)

smarter · 2017-04-05T13:40:22Z

.drone.yml

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


We should still be running the old JUnit tests with the CI.

smarter · 2017-04-05T13:41:30Z

compiler/test/dotty/tools/dotc/vulpix/VulpixTests.scala

 import scala.util.control.NonFatal

-class ParallelTestTests extends ParallelTesting {
+class VulpixTests extends ParallelTesting {


Please add some documentation on what this is for.

smarter · 2017-04-05T13:42:05Z

compiler/test/dotc/comptest.scala


 object comptest extends ParallelTesting {

+  def maxDuration = 3.seconds
+  def numberOfSlaves = 5
+  def safeMode = false


Why is this different from the other overrides of safeMode?

This is a legacy main used by some folks in the lab - it's only running a single test. No need to restart VMs after this single test.

smarter · 2017-04-05T13:42:46Z

compiler/test/dotty/tools/dotc/vulpix/RunnerOrchestration.scala

+trait RunnerOrchestration {
+
+  /** The maximum amount of active runners, which contain a child JVM */
+  def numberOfSlaves: Int


This is always overriden as 5, why not make it the default?

Also, why isn't this based on the number of CPUs?

odersky · 2017-04-06T09:44:12Z

For the moment I cannot yet run CompilationTests instead of the old dotc.test as my standard test suite from Eclipse. There are three problems:

My console window looks like this:

[======================================>] compiling (205/206, 14s)
[======================================>] compiling (205/206, 14s)
[======================================>] compiling (205/206, 14s)
[======================================>] compiling (205/206, 15s)
[======================================>] compiling (205/206, 15s)
[======================================>] compiling (205/206, 15s)
[======================================>] compiling (205/206, 15s)
[======================================>] compiling (205/206, 15s)

... and so on with 10 new entries per second. This fills all available lines and crowds out useful output.

When a test fails I see something like this:

java.lang.AssertionError: Neg test shouldn't have failed, but did
at org.junit.Assert.fail(Assert.java:88)

It should at least tell me the name of the test that failed.

It would be good to have an option to disable running tests. It takes a lot of time and if I work on the frontend I am rarely interested in that aspect.

felixmulder · 2017-04-06T09:51:31Z

My console window looks like this:

Hopefully fixed by 4d76b10

Gotta board the plane! Will look at the rest on board

felixmulder · 2017-04-06T13:18:41Z

@odersky:

Fixed (1) in 84917a3 instead. You'll now need to set property "dotty.tests.interactive" to "FALSE" to have the tests run without interactive progress bar.

Apparently eclipse had to revert a fix that handled \r. It's the 21st century, what's going on?

(2): could you gist or otherwise add the console output here? You can use

<details>paste all the things here</details>

Which results in:

paste all the things here

It would be good to have an option to disable running tests. It takes a lot of time and if I work on the frontend I am rarely interested in that aspect.

How do you want to run this, from commandline or from eclipse? An sbt alias like testFrontend could be added, but then you'd have to invoke it from sbt.

We could also do it via properties or env...

smarter · 2017-04-06T13:21:02Z

We could also do it via properties or env...

I'd say, make everything a flag and a property, so people who use IDEs can set the property, and people who use sbt can use the flag which is more convenient.

DarkDimius · 2017-04-06T13:23:24Z

We could also do it via properties or env...

I'd say, make everything a flag and a property, so people who use IDEs can set the property, and people who use sbt can use the flag which is more convenient.

I'm not sure if having two side-channels is a good thing. I'd strongly prefer to have no more than one.

smarter · 2017-04-06T13:24:30Z

I'm not sure if having two side-channels is a good thing. I'd strongly prefer to have no more than one.

No it's fine, the sbt flag just needs to set the property, so you only have one side-channel in the end.

smarter · 2017-04-10T12:06:32Z

compiler/test/dotty/tools/vulpix/SummaryReport.java

+/** This class adds summary reports to `ParallelTesting`
+ *
+ *  It is written in Java because we currently cannot explicitly write static
+ *  methods in Scala without SIP-25 (`@static` fields and methods in Scala)


If you only need static methods, isn't using a scala-defined object sufficient? The compiler will emit a corresponding class with static methods forwarding to the object's methods for interoperability with Java.

Doesn't work with JUnit.

But why though.

smarter

Otherwise LGTM, looking forward to playing with it more!

smarter · 2017-04-11T20:50:43Z

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

@@ -52,6 +51,15 @@ trait ParallelTesting { self =>
    def outDir: JFile
    def flags: Array[String]

+    def classPath: String = {
+      val (beforeCp, cpAndAfter) = flags.toList.span(_ != "-classpath")


I would just do:

flags.dropWhile(_ != "-classpath").drop(1).headOption

smarter · 2017-04-11T20:53:15Z

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

    protected final val realStdout = System.out
    protected final val realStderr = System.err

+    /** A runnable that logs its contents in a buffer */
+    trait LoggedRunnable extends Runnable {


If this doesn't use anything from the outer class, it should not be an inner class, and preferably it should be in its own file.

smarter · 2017-04-11T21:22:16Z

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

+      summaryReport.addStartingMessage {
+        """|WARNING
+           |-------
+           |Run tests were only compiled, not run - this is due to `dotty.tests.norun`


due to the

smarter · 2017-04-11T21:25:48Z

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

+        if (!compilerCrashed && errorCount == 0 && hasCheckFile) verifier()
+        else if (!compilerCrashed && errorCount == 0) {
+          if (Properties.testsNoRun) addNoRunWarning()
+          else runMain(testSource.classPath) match {


I don't like the repetition with verifyOutput above here, would be nicer if we pretended that all tests had a check file (no check file would be equivalent to empty check file).

smarter · 2017-04-11T21:29:49Z

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

+        childStdin = null
+      }
+
+      private[this] var didAddCleanup = false


Can you document this variable, and maybe give it a nicer name?

smarter · 2017-04-11T21:34:57Z

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

+import scala.collection.mutable
+import dotc.reporting.TestReporter
+
+trait SummaryReporting {


Could you add some documentation in this file, what's a SummaryReporting? What's a SummaryReport? Also, the fact that these are two different classes with very similar name is confusing, maybe use SummaryReport and DefaultSummaryReport; I'm not sure.

smarter · 2017-04-11T21:35:35Z

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

+
+    failedTests.map(x => "    " + x).foreach(rep.append)
+
+    // If we're compiling locally, we don't need reproduce instructions


reproduction, not reproduce

Also, I disagree, I want reproduction instructions, please make it an option at least :).

smarter · 2017-04-11T21:37:07Z

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

+  implicit val defaultOutputDir: String = "../out/"
+
+  implicit class RichStringArray(val xs: Array[String]) extends AnyVal {
+    def and(args: String*): Array[String] = {


smarter · 2017-04-11T21:43:01Z

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

@@ -55,4 +61,16 @@ class ParallelTestTests extends ParallelTesting {

  @Test def runOutRedirects: Unit =
    compileFile("../tests/partest-test/i2147.scala", defaultOptions).expectFailure.checkRuns()
+
+  @Test def infiteNonRec: Unit =


typo: infite -> infinite, same below

smarter · 2017-04-11T21:43:40Z

project/Build.scala

        val args: Seq[String] = spaceDelimited("<arg>").parsed
-        testOptions := Seq()
+        val realArgs = if (args.nonEmpty) args else Seq(".*")


Please, just change the filter to contain the regexp you pass as argument instead of being equal to it :).

Note that merging this as-is will not protect us against fork bombs. This is because the timeout of tests is currently 180 seconds. A forkbomb that is allowed to run for that long...

felixmulder requested a review from smarter April 5, 2017 08:52

felixmulder commented Apr 5, 2017

View reviewed changes

smarter reviewed Apr 5, 2017

View reviewed changes

felixmulder force-pushed the topic/hydra-part-2 branch 2 times, most recently from 081d928 to 7a63f42 Compare April 5, 2017 18:01

felixmulder force-pushed the topic/hydra-part-2 branch from 4d76b10 to 84917a3 Compare April 6, 2017 13:08

felixmulder force-pushed the topic/hydra-part-2 branch 2 times, most recently from 7966234 to a44d8f8 Compare April 7, 2017 13:23

felixmulder mentioned this pull request Apr 10, 2017

Missing information in Vulpix test logs #2210

Closed

felixmulder force-pushed the topic/hydra-part-2 branch from a44d8f8 to 1fff140 Compare April 10, 2017 12:00

smarter reviewed Apr 10, 2017

View reviewed changes

felixmulder force-pushed the topic/hydra-part-2 branch 3 times, most recently from bd0e317 to 280a0c8 Compare April 11, 2017 11:21

smarter approved these changes Apr 11, 2017

View reviewed changes

felixmulder added 8 commits April 12, 2017 11:21

Rename test suite to "vulpix"

d65e4ea

Add reproduction instr for pos separate compilation

5c573d3

Add cleanup hooks to SummaryReporter

c15b83b

Add initial RunnerOrchestration interface

2969101

Implement inter-VM communication logic

429066b

Fix settings having uninitialized values

a3078ee

Complete subprocess communication protocol

a5b9d07

Add RunnerOrchestration to ParallelTesting trait

eff50df

felixmulder added 13 commits April 12, 2017 11:21

Remove partest, keeping dotc.tests for now

2f1a542

Rename filter shorthand to vulpix and update drone.yml

7dcfbd7

Move vulpix to dotty.tools.vulpix

923533e

Make inter JVM communication be string based

c1e787f

Improve test failure output

95732a3

Disable scalac internals tests

0ee9977

Add vulpix docstrings and change defines

67065d0

Only complete tests after run has been performed

1fced2b

Add Properties object for dotty testing props and env

07fab41

Fix interactive mode in eclipse

8fe0d14

Implement meta tests suggested by @DarkDimius

55803b2

Note that merging this as-is will not protect us against fork bombs. This is because the timeout of tests is currently 180 seconds. A forkbomb that is allowed to run for that long...

Add ability to only compile run tests

d42a28d

Make sure that everything is dumped to log files

f891b22

felixmulder force-pushed the topic/hydra-part-2 branch from 280a0c8 to e8b695d Compare April 12, 2017 09:31

felixmulder added 4 commits April 12, 2017 11:48

Remove need for java written summary reporter

bcdacee

Make vulpix test filter work on substring instead of regexp

7c42277

Remove duplication with run test output verification

ebd1670

Add docs to SummaryReport.scala

f6d519a

felixmulder force-pushed the topic/hydra-part-2 branch from e8b695d to f6d519a Compare April 12, 2017 09:49

felixmulder merged commit d541452 into scala:master Apr 12, 2017

allanrenucci deleted the topic/hydra-part-2 branch December 14, 2017 19:25


		def testFilter: Option[Regex] = sys.props.get("dotty.partest.filter").map(r => new Regex(r))
		def maxDuration = 180.seconds


		private[this] val monitor = new RunnerMonitor

		private class RunnerMonitor {


		failedTests.map(x => " " + x).foreach(rep.append)

		// If we're compiling locally, we don't need reproduce instructions

Killing partest part 2 - Enter the Vulpix #2194

Killing partest part 2 - Enter the Vulpix #2194

Uh oh!

Conversation

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

Uh oh!

Uh oh!

felixmulder Apr 5, 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!

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!

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!

smarter left a comment

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!

smarter Apr 5, 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!

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!

odersky commented Apr 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

felixmulder commented Apr 6, 2017

Uh oh!

felixmulder commented Apr 6, 2017

Uh oh!

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

Uh oh!

Uh oh!

DarkDimius commented Apr 6, 2017

Uh oh!

smarter commented Apr 6, 2017

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!

smarter left a comment

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

felixmulder commented Apr 5, 2017 •

edited

Loading

felixmulder Apr 5, 2017 •

edited

Loading

smarter Apr 5, 2017 •

edited

Loading

odersky commented Apr 6, 2017 •

edited

Loading

smarter commented Apr 6, 2017 •

edited

Loading