Command line switch to fail on focus rocket

[c089]

Focus rockets are great, but leaving them in the code before committing is a hazard. For CI servers, I could imagine a command line switch that, when activated will cause buster-test to fail with an error if any rockets are found.

[augustl]

That makes a lot of sense, thanks for the suggestion.

[dominykas]

I'd love to fix this, as I'm getting burned by this myself occasionally, but I'll need some help...

If I understand correctly, I should create a new flag, e.g. --earthworm or --grounded in buster-test-cli, then update prepareOptions and it will eventually propagate down to https://github.com/busterjs/buster-test/blob/master/lib/test-runner.js#L474. That's the easy bit (except figuring out how to properly write tests for the entire thing).

But what's the appropriate thing to do when this.focusMode && this.grounded? throw? suite:end with some sort of a result? Reject the promise?

[dkl-ppi]

@dymonaz, i currently don't know how to realize it, but the behavior should be similar to the behavior in case you have a test-case without a name set. In test-case.js an Error is thrown and that leads to the output

Failed requiring .\test.js: Test case name required

without a stack trace.

[dominykas]

What if instead of failing, the switch ignored the rockets (i.e. on --grounded run all tests, no rockets allowed)? I think that might just be easier to implement... Just an extra condition around here: https://github.com/busterjs/buster-test/blob/master/lib/test-runner.js#L497

[dominykas]

Then again... the above might be a bad idea, since the goal is not only to "make CI run ALL tests" - checking in focus rocket affects other people in the team and should draw attention...

[augustl]

Note that if you return when.reject("An error message") from the runContexts function (i.e. return a rejected promise), I'm pretty sure the test run will fail with that message. So if you figure out how to add a CLI flag you should be all good :)

[dkl-ppi]

@dymonaz, i even think the main purpose of the command line switch is to ensure nobody has checked in a focus rocket.
@augustl, i am quite sure return when.reject("An error message") in test-runner#runContexts won't work, because i have tried it out. The test run just hangs.

[dominykas]

Returning a rejected promise is not good enough, because nobody actually listens for it, i.e. while runSuite() returns a promise, I don't think anyone ever reads its value (I couldn't find a place that does). Tests read runSuite() promises.

The reason the test run just hangs is because if there's a rejected promise in runContexts, runSuite rejects itself, but never fires "suite:end" - which is a separate issue.

I was able to make the suite fail by rejecting the promise, still firing suite:end, but setting results.ok=false. This does not show a pretty error message though... But that's probably up to reporters?

[dominykas]

The code in the pull request doesn't look pretty, but it does the job. The only real problem is that the error is never displayed - I'd normally just console.error that - but is there something else I should do? What about reporters, etc?

A better approach would be a suite:end with an error (rather than results), or even a suite:error - but that would mean a huge change across all the reporters, I guess - and I'm not brave enough to just do it.

But if this approach is kind of OK - it's fairly simple to add if (this.focusMode && this.focusModeDisabled) { runSuitePromise = when.reject(new Error("Focus rockets are grounded."); } and move on to adding the command line argument.

[dominykas]

Can we re-open this? busterjs/buster-test#25 was just a first step - still working on the final solution (I think there'll be one PR on buster-test and one on buster-test-cli).

[dkl-ppi]

Yes we can ;)

[dominykas]

OK, the --no-focus-mode switch is there and it seems to work. The remaining piece is proper error reporting, as mentioned before. Right now I'm thinking that emitting suite:error just before suite:end here: https://github.com/busterjs/buster-test/blob/master/lib/test-runner.js#L498 is the right thing to do - it would then be up to individual reports to pretty-print it. Thoughts?

[cjohansen]

That sounds good. Make sure that the result object emitted with suite:end also indicates an unsuccessful run.

Small nitpick: I prefer boolean options and command line arguments, so I'd like to rename the option here. One suggestion is --fail-focus-rocket, but I'm open to other suggestions. Thoughts?

[dkl-ppi]

We already have a --fail-on option for failing on warnings at the given level. Thus i would prefer --fail-on-focus-rocket, but i don't have an idea for the short option.

[dominykas]

--fail-on-focus (skip the rocket for less typing)?

@cjohansen suite:end already passes a results object with bool ok - but it does not contain the actual error message (and I'm not sure I want to add there, because it's just a very nice and small object with result counts).

[cjohansen]

👍

[cjohansen]

@dymonaz I just meant that the ok flag in suite:end needs to be false. This is in addition to your proposed suite:error event.

[dominykas]

Yeah, I already did the ok bit earlier. I'll probably update both PRs this week.

[dkl-ppi]

@dymonaz 👍 for --fail-on-focus

But which short option we want to use?

[dominykas]

I was thinking about none :)

[cjohansen]

None is fine :)

[dkl-ppi]

That's maximum shortness. :) 👍

[dominykas]

OK, I think busterjs/buster-test-cli#14 is now done - renamed to --fail-on-focus and I think that's all?

As for the suite:error... It now gets emitted just before suite:end, so that part is done, but what do I do about reporters? My initial thought was to blindly reuse the uncaughtException handler, but...

• brief reporter is broken a little - see "brief" reporter fails when exception happens outside of test context in browsers #422 and don't crash when failed to determine context buster-test#27.
• uncaughtException doesn't get displayed in specification reporter at all
• I don't really have a good understanding about json-proxy reporter.
• xml is the only one where it would work right now
• No other reporter implements uncaughtException handler... While I could probably come up with something re html and html2, but teamcity and tap are completely unfamiliar to me.

As I'm not really a fan of long threads that never get released, my suggestion would be this:

• Let's close off don't crash when failed to determine context buster-test#27, so that I can merge it into my "focus rocket" branch - it's the minimum needed to continue with "brief" reporter fails when exception happens outside of test context in browsers #422 anyways
• I'll then update brief and xml to just do the simple stupid suite:error = uncaughtException
• I'll create the issues to implement suite:error / uncaughtException in other reporters

Do you think we'd be close to a release-able state after that? Happy to hear about alternatives on how to carve this up into smaller pieces of work...

[cjohansen]

The json-proxy reporter is used when the test runner is running in the browser. It is connected to a socket which passes the test runner messages back to the server. It should pass the message along like it does with the others.

Sounds like a solid plan!

[dominykas]

Took the liberty to also update the specification reporter - seems that during the API changes printStats was not renamed to be a suite:end handler (well, at least I hope that's what was supposed to happen). Now specification prints the summary at the end again, and also does the suite:error to uncaughtException dance.

I think this is done, once busterjs/buster-test#26 is merged?

Docs probably need an update for this http://docs.busterjs.org/en/latest/overview/#focus-rocket - are there any other places to update?

[cjohansen]

No, I think that's about it. Well done!

[dominykas]

This should be closed, shouldn't it?