search

bensu / doo

doo is a library and lein plugin to run cljs.test on different js environments.
Eclipse Public License 1.0
324 stars 63 forks source link

Add a dummy definition for `document` to the Nashorn runner. #98

Closed mikesperber closed 8 years ago

mikesperber commented 8 years ago

This enables a few programs (for example, headless React tests), and outputs better error messages for others.

bensu commented 8 years ago

Hi @mikesperber

Thanks for the PR, I'm taking extra time merging the changes because the tests for the library (cd library && lein test) hang. Can you try to run them?

mikesperber commented 8 years ago

They work for us, except for this:

FAIL in (integration) (core.clj:110) We can compile a cljs project expected: (let [compiler-opts' (merge compiler-opts {})] (cljs/build srcs compiler-opts') (->> [:phantom :chrome :firefox] (mapv (fn [env] (-> env (doo/run-script compiler-opts' doo-opts) doo-ok?))) (every? true?))) actual: false

But this fails regardless of the change.

mikesperber commented 8 years ago

Could this pull request get some love? (I just updated the patch to the current master.)

I don't understand the CircleCI test results:

So I think in summary everything is fine.

bensu commented 8 years ago

Hey @mikesperber,

Thank you for updating the tests to master and checking this. You are right, the CircleCI failures are npm problems and not related to your branch. But when I run lein doo nashorn once on the example project it hangs, which doesn't happen for master. After looking at the compiled out/testable.js, I see:

return "undefined"!==typeof document&&"onreadystatechange"in document.createElement("SCRIPT")?...

which comes from goog.async.nextTick. Changing the type of document from undefined to object gives that snippet confidence it can use document to access the DOM, when in fact it can't. This makes it a show stopper for any core.async users which relies on goog.async.

To address your issue, we can maybe add document as a :foreign-lib as:

:foreign-libs [{:file "resources/document.js"
                      :provides ["document"]}]

Do you have any better ideas on how we should proceed?

mikesperber commented 8 years ago

Hi @bensu,

thanks for the feedback - good points! By "we" you mean "us", right? :-)

So I'm OK with closing this issue.

bensu commented 8 years ago

Hi @mikesperber,

Sorry for not being clear :) Yes. I meant that you should try putting it under :foreign-libs and that could be the standard solution for this problem.

Closing this, I'm sorry this didn't work out, it is a valid use case. At some point I'll probably write some docs on how to polyfill node/nashorn for dom testing.