JUnit: populate sbt.testing.Event.duration #5134
Conversation
|
@sjrd any comments? |
| @@ -117,12 +117,14 @@ private[junit] final class Reporter(eventHandler: EventHandler, | |||
| private def emitEvent( | |||
| method: Option[String], | |||
| status: Status, | |||
| throwable: OptionalThrowable = new OptionalThrowable | |||
| throwable: OptionalThrowable = new OptionalThrowable, | |||
| timeInSeconds: Double = 0.001 | |||
There was a problem hiding this comment.
That default value looks very suspicious. I think it should be an Option[Double] = None, if anything, and that None really translates to the constant -1L, as specified in the doc of sbt.testing.Event:
https://github.com/sbt/test-interface/blob/e0db1f531713ac7b66d3159513d2c044bc2cdca0/src/main/java/sbt/testing/Event.java#L37-L40
There was a problem hiding this comment.
Thanks!! Not only was it ugly and suspicious, it was also wrong (positive instead of negative).
I reworked it the way you suggested.
I also reworked the throwable parameter the same way and dropped the defaults for both: let's be explicit.
This surfaced an issue: on JVM, time is reported for skipped tests (which TestMetadata calls ignored),
but on Scala.js it is not; this is why in JUnitTest I record duration as not populated for such tests.
There was a problem hiding this comment.
Alternatively, we can report duration of 0 in Reporter.reportIgnored()... Please advise ;)
There was a problem hiding this comment.
Our design is to emulate what the original JUnit does to the greatest extent possible. So if JUnit-on-JVM reports a duration for ignored tests, we should do the same, even if it's 0 as you suggest.
| case Log(level, msg) => "l" + level + msg | ||
| case Done(msg) => "d" + msg | ||
| case Event(s, n, t) => "e" + s.ordinal + n + separator + t.getOrElse("") | ||
| case Event(s, n, t, d) => "e" + s.ordinal + n + separator + t.getOrElse("") + separator + d |
There was a problem hiding this comment.
Please keep the arrows aligned:
| case Log(level, msg) => "l" + level + msg | |
| case Done(msg) => "d" + msg | |
| case Event(s, n, t) => "e" + s.ordinal + n + separator + t.getOrElse("") | |
| case Event(s, n, t, d) => "e" + s.ordinal + n + separator + t.getOrElse("") + separator + d | |
| case Log(level, msg) => "l" + level + msg | |
| case Done(msg) => "d" + msg | |
| case Event(s, n, t) => "e" + s.ordinal + n + separator + t.getOrElse("") | |
| case Event(s, n, t, d) => "e" + s.ordinal + n + separator + t.getOrElse("") + separator + d |
1727e43 to
4a2d0e1
Compare
No worries. Thank you for the review! |
If duration associated with the test event is available, bubble it up through the SBT's test interface using the slot dedicated to just such information: sbt.testing.Event.duration. In addition to it being a shame to throw away information about the duration that is literally in our hands, this avoids weird-looking negative duration values in test reports.
4a2d0e1 to
8220951
Compare
|
Thank you, @sjrd! |
If duration associated with the test event is available, bubble it up through the SBT's test interface using the slot dedicated to just such information:
sbt.testing.Event.duration.In addition to it being a shame to throw away information about the duration that is literally in our hands, this avoids weird-looking negative duration values in test reports.