Upgrade to Scala.js 1.0.0-M5

[sjrd]

This is completely broken at the moment, but I'm leaving tomorrow for a week, so I'll just let it sit there.

[sjrd]

@gzm0 I think I'm good with PhantomJSEnv per se. However I'm stuck on RetryingComJSEnv. I think I've implemented it correctly, but it has become impossible to test: the same instance of JSEnv is reused across all tests in the test suite, so we cannot keep track of when we should fail and when we should not in isolation. This is particularly problematic for the test multiEnvTest.

I'm not even sure that it is otherwise correctly implemented.

Any idea?

[sjrd]

Bigger problem with RetryingComJSEnv: it is not able to deal with non-inherited I/O. Indeed, it would call onOutputStream several times if it retries, which is not permitted.

I think it's not possible to implement RetryingComJSEnv with the new API. Should we just remove it? (Or implement it as non-retrying wrapper and deprecate it.)

[gzm0]

Re: RetryingEnv:

but it has become impossible to test:

It probably is necessary to write specific tests for that functionality (rather then re-using the conformance suite). IIUC a correctly implemented (non mocked) RetryingComJSEnv will pass conformance without issue.

it is not able to deal with non-inherited I/O

It seems that RetryingComJSEnv doesn't have the same behavior for com than for I/O (it only retries if com is reproducible, otherwise fails). If we want to retain this behavior, you'll have to use a pipe when I/O is non-inherited so you can invoke onOutputStream only once and pipe I/O forward from the underlying env (that will call your own onOutputStream again if it gets restarted).

However, given the inconsistency of definition, I'm OK with removing RetryingComJSEnv entirely and expect users to retry on a higher level if necessary. (Maybe we should run this against the community though :S).

[sjrd]

It probably is necessary to write specific tests for that functionality (rather then re-using the conformance suite). IIUC a correctly implemented (non mocked) RetryingComJSEnv will pass conformance without issue.

Ah, wow, I should have thought about that! Following that strategy, I actually got the Suite to pass on the one hand, and to write a meaningful test for the retrying behavior on the other hand.

I think this PR is in a sufficiently good shape for a review, now.

Question: maybe we should declare that RetryingComJSEnv does not support non-inherited I/O, using the Validator? If we do that the Suite does not pass anymore. Some of it (the compare-output tests) requires support for non-inherited I/O ...

[sjrd]

Rebased.

I currently get intermittent failures like this on Travis :s

[error] Test org.scalajs.jsenv.test.RunTests.allowScriptTags[config = PhantomJS, withCom = true] failed: java.util.concurrent.TimeoutException: Futures timed out after [1 minute], took 60.143 sec
[error]     at scala.concurrent.impl.Promise$DefaultPromise.ready(Promise.scala:255)
[error]     at scala.concurrent.impl.Promise$DefaultPromise.result(Promise.scala:259)
[error]     at scala.concurrent.Await$.$anonfun$result$1(package.scala:215)
[error]     at scala.concurrent.BlockContext$DefaultBlockContext$.blockOn(BlockContext.scala:53)
[error]     at scala.concurrent.Await$.result(package.scala:142)
[error]     at org.scalajs.jsenv.test.TestKit.awaitAfterClose(TestKit.scala:42)
[error]     at org.scalajs.jsenv.test.TestKit$RunMatcher.hasOutput(TestKit.scala:59)
[error]     at org.scalajs.jsenv.test.RunTests.allowScriptTags(RunTests.scala:42)
[error]     ...

[sjrd]

I currently get intermittent failures like this on Travis :s

Looks like I tamed that with the latest commit. Now state management is very similar to how it's done in Node.js' com runs.

[sjrd]

Updated. I think I've addressed all the comments, except the thing about logging. But if you review with ?w=1 in the URL, you'll see that I haven't actually changed that code. It was like that before.

[sjrd]

I'm OK with going forward without this, but we should keep this in mind if we get bug reports.

I suggest we go ahead with the status quo. This PR is the last thing blocking the announcement of Scala.js 1.0.0-M5. This PR is about upgrading to M5, not fixing everything that didn't work before (otherwise we'll never get it done).