lein-doo exits on React warnings

[r0man]

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]

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]

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]

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]

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]

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]

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

[FossiFoo]

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

[arichiardi]

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]

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]

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]

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]

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.