Only warn about init function being unset when it actually isn't set

[MatthewDarling]

Discussion started in #122.

I think this is what the original code was trying to do: if *main-cli-fn* was nil, then you'd get an error trying to use nil as a function, catch that error, and report it. But there are many other reasons an exception might be thrown. A spurious warning will only distract people when they're trying to identify the real issue.

Are there other kinds of invalid values for *main-cli-fn*? If so, those should be checked as well as for nil.

[miikka]

It is definitely an improvement! Printing a stacktrace would be great, too. If you can't make the approach you tried in #102 to work, let's have (println (.-stack e)) at least. It may not work on some platforms, but you could add a check for that like the browser repl does.

*main-cli-fn* must be a callable, so I think that (fn? *main-cli-fn*) is not an unreasonable test.

[MatthewDarling]

fn? returns false for both nil and false, so, I've switched to just (if-not (fn? *main-cli-fn*) .... Thanks :)

I'm wondering whether to change the message though. If *main-cli-fn* were set to something invalid, like a literal value, the message would say "init function was not set". But that wouldn't really be accurate.

The simplest change I can think of is "WARNING: doo's init function was not set correctly". But more extensive changes would be fine by me, too.

---

Aside from that, here's the behaviour I've pushed for the default exception handling:

Testing budb.cljs.register-test

ERROR: Exception outside tests:
#object[TypeError TypeError: undefined is not an object (evaluating '"foo".js.Error')]

ERROR: Stacktrace:
file:///Users/matthewdarling/code/budb/out/budb/cljs/register_test.js:9:18
cljs$test$run_block@file:///Users/matthewdarling/code/budb/out/cljs/test.js:378:17
file:///Users/matthewdarling/code/budb/out/running/doo_runner.js:1214:32
doo$runner$run_BANG_@file:///Users/matthewdarling/code/budb/out/doo/runner.js:60:50


global code
evaluateJavaScript@[native code]
evaluate@phantomjs://platform/webpage.js:390:39
phantomjs://code/phantom1145161256762945379.js:124:19
_onPageOpenFinished@phantomjs://platform/webpage.js:286:27

It seems like this code isn't hit when there's an exception inside a deftest, because those are caught and reported as an error in the tests. So "exception outside tests" seems to describe when you might hit this problem. But I'm not confident that covers all the cases, plus this is some classic terse Clojurian documentation. Would be glad to have writing help on this :)

[bensu]

The messaging for those exceptions is more accurate than the original one. While it is rare to get exceptions outside of deftest it is definitely possible. If it works on most platforms, I'd merge as is.

[MatthewDarling]

Tested a few things on my Mac running Sierra:

Environment	Stacktrace?	Warns about exit function?
PhantomJS 2.1.1	Yes	No
Chrome 61.0.3163	Yes	Yes
Safari 11.0.0	Yes	Yes
Opera 47.0.2631	Yes	Yes
Firefox 52.0.0	No	Yes
SlimerJS 0.10.3	No	No

Makes sense that if Firefox had no stacktrace, Slimer wouldn't either. And the rest are all WebKit based so I guess that makes sense too.

I didn't try Node or Nashorn, since they seemed to need a bit more setup. And can't test IE.

---

Now that I've seen it firsthand, I think this looks silly:

ERROR: Stacktrace:
No stacktrace available.

Maybe it would be better to move all the printing for the stacktrace inside the (if (.hasOwnProperty e "stack") ... )?

[miikka]

I added my suggestions for the messages as review comments. After those changes, I'd be happy to merge this.

What do you mean with "Warns about exit function" in your table?

[miikka]

I'd suggest "doo's init function was not set or is not a function"

[miikka]

I think that in case there's no stacktrace, you should either see No stacktrace available or nothing at all, but not ERROR: Stacktrace:. I.e. please move the printing inside the if block, as you said.

[MatthewDarling]

"Warns about exit function" means the behaviour from this line. The Karma runners I tried all produce that warning. Looks like this:

LOG: 'WARNING: doo's exit function was not properly set'
LOG: '#object[TypeError TypeError: doo.runner._STAR_exit_fn_STAR_ is null]'

Possibly might need to have a setting for the *exit-fn* for Karma as well. Although their docs have an example using process.exit, so maybe this ought to be working...

---

Anyway, here's how things look within Karma (running on Firefox, so no stacktrace):

LOG: 'Testing budb.cljs.register-test'
LOG: ''
LOG: 'ERROR: Exception outside tests:'
LOG: 'ERROR: #object[TypeError TypeError: "wololo".js is undefined]'
LOG: ''
LOG: 'No stacktrace available.'

And within PhantomJS:

Testing budb.cljs.register-test

ERROR: Exception outside tests:
ERROR: #object[TypeError TypeError: undefined is not an object (evaluating '"wololo".js.Error')]

ERROR: Stacktrace:
ERROR: file:///Users/matthewdarling/code/budb/out/budb/cljs/register_test.js:9:18
cljs$test$run_block@file:///Users/matthewdarling/code/budb/out/cljs/test.js:378:17
file:///Users/matthewdarling/code/budb/out/running/doo_runner.js:1214:32
doo$runner$run_BANG_@file:///Users/matthewdarling/code/budb/out/doo/runner.js:60:50


global code
evaluateJavaScript@[native code]
evaluate@phantomjs://platform/webpage.js:390:39
phantomjs://code/phantom6072078159243578225.js:124:19
_onPageOpenFinished@phantomjs://platform/webpage.js:286:27

At this point, the formatting seems okay.

I took a small detour to check out whether console.error would be better to use than println. Karma has nice support for it, replacing that LOG: prefix on each line with ERROR:. However, Slimer treats it as a regular print and it crashes PhantomJS. So it's more portable to just use println, but it could maybe be a Karma-specific feature.

[miikka]

Right, now I get what you mean by exit function. I'm going to merge this pull request now -- it's a good improvement as it is -- but if you want to do something to the exit function, don't hesitate to open a new PR. 😃

Thanks for making this better!

[bensu]

Great work! I'm very happy that this made it in.