Upgrade to Scala.js 1.0.0-M5. #12
Conversation
sjrd
force-pushed
the
upgrade-scalajs-1.0.0-M5
branch
from
21a8746 to
e8020fd
Compare
| @@ -0,0 +1,159 @@ | |||
| package org.scalajs.jsenv.jsdomnodejs | |||
|
|
|||
| // !!! This entire file is copied from TestKit.scala upstream | |||
There was a problem hiding this comment.
This here is absolutely disgusting. What was I supposed to do, here?
There was a problem hiding this comment.
Do you really need all this? From what I can see is all you want to test is that the window API doesn't fail (I guess that is to check there is a DOM).
Wouldn't it be enough to check:
- That the code
window.history.pushState({}, "", "/foo"); window.close()terminates successfully. - That the code
window.history.inexistent();does fail.
That way you only need to rely on the public interface of the Env.
There was a problem hiding this comment.
Well TestKit only relies on the public API of JSEnv too. I first tried to do it by hand, but quickly ended up redoing several steps already provided by the test kit: handling of futures, comparison of the output, etc. Eventually I decided that I would have less to maintain by copying it over.
There was a problem hiding this comment.
Well TestKit only relies on the public API of JSEnv too.
Yes, but your test will then also rely on TestKit.
I understand that output handling is difficult, but do you need to read output here? Future handling would be just calling Await.result, or am I missing something?
There was a problem hiding this comment.
I added a commit that shows what the test looks like if I don't use TestKit. It does not read the output, which indeed makes the thing simpler. It would be significantly more complicated if it did have to check the output, though.
Should I proceed with the new test, and rip out the one using TestKit?
There was a problem hiding this comment.
Yes, let's go ahead with this for now and then re-asses whether we need to expose something TestKit like (IMHO TestKit needs better design before we expose it).
| | var scalajsComWrapper = scalajsCom === (void 0) ? scalajsCom : ({ | ||
| | init: function(recvCB) { | ||
| | scalajsCom.init(function(msg) { | ||
| | process.nextTick(recvCB, msg); |
There was a problem hiding this comment.
I'm pretty sure this should be integrated inside ComSupport.scala upstream.
If I don't do this, I get an IllegalStateException because the message for JSEndPoints.detectFrameworks is delivered before TestAdapterBridge.start() gets to setup the handler. Indeed, the messages are delivered during the constructor of TestAdapterBridge, through the constructor of JSRPC.
There was a problem hiding this comment.
Hmmm... Yes, but why does this not happen in the base env :-S
There was a problem hiding this comment.
Because in the base Node.js env, there is no return to the event loop between the startup and start(), so the network messages haven't had a chance to be processed by a JS event handler, so they're not yet in the message buffer in JS. They're still in a C++ buffer somewhere. In the jsdom env, there is a come back to the event loop before this code is executed.
It took me a while to figure it out ^^
There was a problem hiding this comment.
Ah, ah, I see. Fair enough. So yes, it should definitely be moved to ComSupport.
sjrd
force-pushed
the
upgrade-scalajs-1.0.0-M5
branch
from
e8020fd to
9463ff2
Compare
| /* In Scala 2.10.x, NotImplementedError was considered fatal. | ||
| * We need this case for the conformance tests to pass on 2.10. | ||
| */ | ||
| JSRun.failed(t) |
There was a problem hiding this comment.
Given scala/scala@68f62d7, perhaps the conformance tests should use a different exception than NotImplementedError. Anything else that would be considered NonFatal by 2.10.x, really.
sjrd
force-pushed
the
upgrade-scalajs-1.0.0-M5
branch
from
386de74 to
9403c96
Compare
No description provided.