Fix flaky tests #434
Fix flaky tests #434
Conversation
reibitto
left a comment
reibitto
left a comment
There was a problem hiding this comment.
So it seems like the key missing piece was ProcessPlatformSpecific.wait(inputStream). I think that makes sense. This should at the very least fix all those "terminal" operators where the entire stream is consumed.
I think there might still be issues for streaming output that's only partially consumed and left alone after that, but I could tackle that on my own after writing some more tests in my branch and seeing if it's actually an issue for Scala.js or Scala Native. It may already be fine but it was just a concern I had.
Anyway thanks for looking into this! Once merged I'll do the final pieces in my branch and finally make an official non-snapshot release. After that I'll try supporting those other minor features that are currently marked as TestAspect.exceptNative, TestAspect.exceptJS, and so on.
| import zio.{ durationInt, Chunk } | ||
|
|
||
| trait ZIOProcessBaseSpec extends ZIOSpecDefault { | ||
| override def aspects = Chunk(TestAspect.timeout(30.seconds), TestAspect.flaky) |
There was a problem hiding this comment.
Hmm, I don't think we want to make the default TestAspect.flaky. That could cover up legitimate problems. If any tests are flaky (and can't easily be fixed), I'd rather mark them @@ TestAspect.flaky at the individual level so that it's clear which tests have problems.
But maybe this was unintentional or temporary code? Because I see you also do @@ TestAspect.nonFlaky at the suite level below.
There was a problem hiding this comment.
Sorry, this is a typo, it was intended to be nonFlaky... The nonFlaky at suite level is just for one suite because in CommandSpec for Scala Native the nonFlaky times out and it passed it anyway.
| ZIO.scoped { | ||
| close *> ProcessPlatformSpecific.wait(inputStream) *> ZIO.attemptBlockingCancelable { |
There was a problem hiding this comment.
I'm not sure I understand the purpose of the initial close as well as the ZIO.scoped. Could this implementation be this instead?
(for {
_ <- ProcessPlatformSpecific.wait(inputStream)
s <- ZIO.attemptBlockingCancelable {
new String(inputStream.readAllBytes(), charset)
}(ZIO.succeed(inputStream.close()))
} yield s).refineOrDie { case CommandThrowable.IOError(e) =>
e
}There was a problem hiding this comment.
I tried the code I pasted locally and it seems like all the tests pass.
There was a problem hiding this comment.
Technically, close closes the OutputStream from where the InputStream that we want to read is getting data while ProcessPlatformSpecific.wait in ScalaJS waits for that OutputStream to end so it seems like they might be equivalent. Maybe they could have different behaviour with infinite streaming? In current tests they should behave the same.
|
Now there are two flaky tests in ScalaJS that do not get the correct exit code, but the |
2 participants
@reibitto It seems like this fixes the flaky tests! I have added a
TestAspect.nonFlakyexcept on Scala Native because it takes too much time but it was green before.