search

ferlores / mochify-istanbul

Instrument + create reports using istanbul with the mochify stream
Other
20 stars 11 forks source link

fix exit without any report when tests fail #24

Closed m90 closed 7 years ago

m90 commented 7 years ago

I'm running a test suite against mochify. When starting to add coverage reports using mochify-istanbul I am facing the following issue:

When one of the tests fails I will not see the report, but simply:

# phantomjs:
Error: Exit 1

which makes it rather hard to find out what broke.

My test command looks like:

"mochify": "NODE_ENV=test mochify --plugin [ mochify-istanbul --report html --dir ./js_coverage --exclude '**/*.{json,toml,html,test.js}' ] --timeout=10000 --reporter=spec ./assets/scripts/test/unit/**/*.js",

When no tests fail, the report will be printed all at once.

This is due to the fact that this currently buffers all output in a variable and then passes it on in the flushFunction of the through2 stream. Yet, this flushFunction will apparently never be called when one of the tests fail, resulting in the behavior above.

Instead, this PR splits the input on newlines and the checks if we are handling coverage or not.

mantoni commented 7 years ago

I crashed into this also, but didn't get to look into the cause. Great to finally have a fix for this! Can you check the build? There are tests failing.

m90 commented 7 years ago

Yeah, I am looking into the failing build right now, apparently it is still eating blank lines from the output. Will ping you once that is fixed.

mantoni commented 7 years ago

👍 Looks very good to me. Do you want me to merge or fancy adding a unit test?

m90 commented 7 years ago

Since this is abou tests, why not add a unit test? Will do so.

I'm also currently running this against our very own CI suite, so we'll know if there are any unwanted side effects soonish.

mantoni commented 7 years ago

Awesome. Another good test is the Sinon.JS test suite.

m90 commented 7 years ago

There seems to be still some issue with the flushFunction not being properly called, so we should not merge this as is. Will look into this and fix.

m90 commented 7 years ago

Ok, so this runs smoothly now in our CI and has a (rather naive) unit test that fails on master and passes on this branch. I'd be fine with merging, but in case you have an idea on how to test this better (the issue is CLI only), I'd be happy about any hints.

mantoni commented 7 years ago

The CLI integration test for this is fine. Merging.

mantoni commented 7 years ago

Almost all of the tests fail on my environment. I guess it's PhantomJS 2.1 which it doesn't like, even though it downloaded 1.9 on install 🤔

However, I also verified with the Sinon.JS test suite that (1) this was an issue and (2) it is now fixed.

Released as v2.4.2. Thanks 💯 @m90