Issue/55 timedout #85

som-snytt · 2017-04-12T17:56:50Z

Let a timeout fail the build.

Runner.run determines the test state instead of throwing.

lrytz · 2017-04-13T08:17:56Z

src/main/scala/scala/tools/partest/nest/Runner.scala

@@ -759,7 +759,7 @@ class SuiteRunner(
    // |Java Classpath:             ${sys.props("java.class.path")}
  }

-  def onFinishTest(testFile: File, result: TestState): TestState = result
+  def onFinishTest(@deprecated("unused","") testFile: File, result: TestState): TestState = result


this is fighting a symptom..

Is the disease called "object-orientation"?

Shouldn't it only warn on effectively final methods?

@dwijnand I think that weakens the warning. Maybe two modes? strict vs non-strict? By the time you've written final, you're less likely to have made a careless mistake such as this is designed to catch. So strict mode would mean "I think I was careful."

Sure, it weakens it, but I think I prefer a warning that you don't have to workaround.

(As a complete aside, IMO things should be final by default and you should have to annotate it open, or something, rather than have to declare things final).

som-snytt · 2017-04-13T16:48:21Z

src/main/scala/scala/tools/partest/nest/AbstractRunner.scala

-            comment(passFailString(passed.size, failed.size, 0) + " in " + kind)
+          if (limping) comment(s"suite already in failure, skipping") else {
+            val results = suiteRunner.runTestsForFiles(paths.map(_.jfile.getAbsoluteFile)) match {
+              case Left((e, states)) => limping = true ; states


Shouldn't the unused e warn? maybe that was one of the todo's noted in the tests for warnings.

No, it's not built with the latest warnings yet. But for some reason it warns twice.

The result `TestState` is determined when the test is run (rather than throwing exceptions to enclosing infrastructure). Tests which haven't completed are deemed pending. Previously, tests that hadn't completed were deemed skipped. But skipping tests doesn't fail the build. Sample output with very short partest.timeout: ``` Selected 1815 tests drawn from 1 named test categories ok 1 - run/Course-2002-01.scala Thread pool timeout elapsed before all tests were complete! !! 2 - run/Course-2002-03.scala [non-zero exit code] !! 4 - run/Course-2002-02.scala [non-zero exit code] !! 3 - run/Course-2002-04.scala [non-zero exit code] !! 5 - run/Course-2002-05.scala [non-zero exit code] !! 6 - run/Course-2002-06.scala [non-zero exit code] !! 7 - run/Course-2002-08.scala [non-zero exit code] partest --update-check \ /home/apm/projects/snytt/test/files/run/Course-2002-02.scala \ /home/apm/projects/snytt/test/files/run/Course-2002-03.scala \ /home/apm/projects/snytt/test/files/run/Course-2002-04.scala \ /home/apm/projects/snytt/test/files/run/Course-2002-05.scala \ /home/apm/projects/snytt/test/files/run/Course-2002-06.scala \ /home/apm/projects/snytt/test/files/run/Course-2002-07.scala \ /home/apm/projects/snytt/test/files/run/Course-2002-08.scala \ /home/apm/projects/snytt/test/files/run/Course-2002-09.scala \ /home/apm/projects/snytt/test/files/run/Course-2002-10.scala \ /home/apm/projects/snytt/test/files/run/Course-2002-13.scala \ /home/apm/projects/snytt/test/files/run/Meter.scala \ /home/apm/projects/snytt/test/files/run/MeterCaseClass.scala \ /home/apm/projects/snytt/test/files/run/MutableListTest.scala \ /home/apm/projects/snytt/test/files/run/NestedClasses.scala \ /home/apm/projects/snytt/test/files/run/OrderingTest.scala \ /home/apm/projects/snytt/test/files/run/Predef.readLine.scala \ [etc] ``` Incomplete tests (in the category running when interrupted) are reported as pending: ``` ok 38 - pos/cls1.scala Thread pool timeout elapsed before all tests were complete! ok 39 - pos/cls.scala !! 40 - pos/collectGenericCC.scala [compilation failed] !! 41 - pos/chang [timed out] ok 42 - pos/clsrefine.scala [error] Failed: Total 41, Failed 2, Errors 0, Passed 39, Pending 1371 [error] Failed tests: [error] partest [error] (test/it:testOnly) sbt.TestsFailedException: Tests unsuccessful [error] Total time: 14 s, completed Apr 12, 2017 10:37:58 AM ```

dwijnand · 2017-12-06T10:54:25Z

src/main/scala/scala/tools/partest/nest/Runner.scala

@@ -759,7 +761,7 @@ class SuiteRunner(
    // |Java Classpath:             ${sys.props("java.class.path")}
  }

-  def onFinishTest(testFile: File, result: TestState): TestState = result
+  def onFinishTest(testFile: File, result: TestState): TestState = { unused(testFile) ; result }


This is also wrong, imho..

@dwijnand do you prefer pre or post syntax? an annotation to ignore or a reporter that knows to ignore it? This is a middle solution.

I prefer the pre-syntax. IMO the unused param checker shouldn't warn on non-effectively final methods.

(Sorry I'm a little salty atm as I really want to use "warn unused"/Xlint in sbt/sbt but I've been dealing with lots and lots of these..)

Finding the sweet spot for usability is hard, so input is appreciated. Warning on params was always assumed to be dicey. It sounds like: warn on final, unless asking for non-strict. Then, let me disable it with an annotation if I want.

I proposed this solution here because there's no extra machinery.

Btw another false positive (ish) is enriching objects, e.g implicit def enrichFoo(f: Foo.type).

SethTisue · 2018-05-03T14:22:11Z

@som-snytt feel free to resubmit in scala/scala after https://github.com/scala/scala/pull/6566/files lands

I'm sorry this was neglected :-/

in the new sbt-runner-only, in-sourced-partest world, I think we'll all be a lot more willing to review — and make — partest improvements.

som-snytt · 2018-05-03T16:18:39Z

Somehow I didn't miss this feature. But all those beautiful whitespace improvements!

Also thanks @dwijnand @lrytz for feedback on unused usability. I may have forgotten to say that -Xlint enables the uncontroversial checks, -Ywarn-unused is hard core.

SethTisue · 2018-05-03T17:14:15Z

-Ywarn-unused once told me I had a jar of capers in the fridge that were going to expire soon

lrytz reviewed Apr 13, 2017

View reviewed changes

som-snytt commented Apr 13, 2017

View reviewed changes

som-snytt mentioned this pull request Apr 14, 2017

Partest mark test that timed out as skipped instead of failed #55

Closed

som-snytt mentioned this pull request Dec 4, 2017

Remove unused import #89

Merged

som-snytt added 3 commits December 4, 2017 00:45

Clean unused imports

d71137d

Compiles with -Ywarn-unused

f56d0ee

som-snytt force-pushed the issue/55-timedout branch from 005c698 to 8b5f550 Compare December 4, 2017 15:48

Elidably consume unused param

453d4e4

dwijnand reviewed Dec 6, 2017

View reviewed changes

SethTisue closed this May 3, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue/55 timedout #85

Issue/55 timedout #85

som-snytt commented Apr 12, 2017

lrytz Apr 13, 2017

som-snytt Apr 13, 2017

dwijnand Dec 6, 2017

som-snytt Dec 6, 2017

dwijnand Dec 6, 2017

som-snytt Apr 13, 2017

som-snytt Apr 14, 2017 •

edited

Loading

dwijnand Dec 6, 2017

som-snytt Dec 6, 2017

dwijnand Dec 6, 2017

som-snytt Dec 6, 2017

dwijnand Dec 6, 2017

SethTisue commented May 3, 2018

som-snytt commented May 3, 2018

SethTisue commented May 3, 2018

Issue/55 timedout #85

Issue/55 timedout #85

Conversation

som-snytt commented Apr 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

som-snytt Apr 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SethTisue commented May 3, 2018

som-snytt commented May 3, 2018

SethTisue commented May 3, 2018

som-snytt Apr 14, 2017 •

edited

Loading