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

lein-doo exits on React warnings #83

Open r0man opened 8 years ago

r0man commented 8 years ago

Hi Bensu,

I'm using lein-doo to test sablono and run into an issue with React warnings. Whenever React issues a warning lein-doo exits, because this hook is called.

https://github.com/bensu/doo/blob/master/library/resources/runners/headless.js#L43

I'm not exactly sure how those React warnings end up in lein-doo (via js/Error or console.warn).

Looking at the clojurescript.test runner it looks like it only calls exit when something failed with clojurescript.test itself.

So, my question is, is it neccesarry to have this exit call here? I removed it in a local branch and everything seems to work as expected. lein-doo doesn't exit on warnings.

What do you think?

Roman

r0man commented 8 years ago

Update: I think this has also something to do with different versions of phantomjs. My tests run fine on Travis old infrastructure. Locally with phantomjs v2.0.0 and on Travis container based infrastructure they fail.

bensu commented 8 years ago

Hi @r0man

We just discussed this in #82 with @arichiardi

The problem is that from Phantom we don't know how to distinguish between a legit error that should abort tests and a warning if the js script decides to call console.warn. Removing that bit and catching errors only inside cljs.test would yield very unexpected behavior, i.e., if the script completely fails outside of the tests, it would exit with 0.

Do you mind trying to mock console.warn = function(args) { return null; }; or something similar? Alternative you might want to turn off React's DEV flag (not sure how this works). Also, this might be relevant.

Let me know if any of those workarounds work for you.

r0man commented 8 years ago

Hi Bensu,

I got my tests working locally with phantomjs v2.0.0 with the follwoing snippet:

(set! (.-error js/console) (fn [x] (.log js/console x)))

However, they still fail on Travis container based infrastructure. I'm still investigating this.

Also, I'm still not convinced that calling console.error somewhere in JavaScript is actually a valid reason to exit the whole process. ;)

What about leaving p.onError as it is now, but redefine it without the call to exit after the page has been loaded in the callback of p.open. That's how clojurescript.test did it?

https://github.com/cemerick/clojurescript.test/blob/master/resources/cemerick/cljs/test/runner.js#L35

Roman

bensu commented 8 years ago

I think I'm misunderstanding something, is p.onError only called when console.error or also when there is an unhandled exception?

From the docs it seems to be for both exceptions (it uses "unhandled") and console.error (your case). As long as onError gets unhandled exceptions, it should exit with an error code.

arichiardi commented 8 years ago

You know, I think I saw it exiting on console.error (that was the reason of my PR) as well, but I need to double check.

arichiardi commented 8 years ago

We should create a test case for this in doo to be sure and understand the behavior.

FossiFoo commented 8 years ago

I just ran into this and console.error definitely terminates all my tests. This was really unexpected behaviour. Using phantomjs 2.0.0.

arichiardi commented 8 years ago

I saw this one brought up again, just want to confirm that replumb has the same workaround for this: https://github.com/Lambda-X/replumb/blob/master/test/browser/launcher/runner.cljs#L17

vemv commented 6 years ago

The fault is mostly at React's side - they log warnings as errors, which is quite non-standard. When pointing out this https://github.com/facebook/react/issues/10633#issuecomment-333323186 Dan Abramov suggested to not provoke warnings at all - there are almost no cases where one should ignore them.

At first I opposed this but in the end libraries are meant to be used as intended.

Ultimately one can switch phantom->chrome-headless and move on.

Issue might be good to close!

FossiFoo commented 6 years ago

I strongly oppose this opinion. It doesn't matter why anything (not just react) logs to the error console, it shouldn't result in test aborts imho. Just like logging to console.error in production would not stop the webpage/javascript from executing.

FWIW, react switched their deprecation logs to warn with their 15.6. release, but i think the underlying problem is still valid. simply logging to error should not cause doo to kill the phantom engine.

Switching to chrome headless in this case is like saying "we don't support phantom". If that's the case, it should be removed totally imho.

vemv commented 6 years ago

Reasonable.

But it also might be the case that this issue is a phantom-ism. Should doo attempt to fix something in phantom?

I would say no...

FossiFoo commented 6 years ago

I found https://github.com/ariya/phantomjs/issues/10150, but from that it seems like the reverse is true and the main issue was for console.error to not log on stderr. They do however place a similar workaround, redirecting console.error to something sane.

Either way, i think that doo should behave similar on all target engines, so yes, i believe it's up to doo to work around this issue.