Passing custom parameters to Reporters via JS API, logic from bin script to CoffeeScript

[kuba-kubula]

Because clone cannot clone process.stdout, custom configuration is passed to Dredd class as a second argument

[netmilk]

I thought this part was supposed to be moved and handled in apiary-reporter class

[netmilk]

And tested there :)

[netmilk]

I'm sorry, but:

• I can't find out any clear purpose of this pull request. It's not explained anywhere and it should be evident.
• I assume it's a combination of:
  1. refactoring of layer handling CLI
  2. extension of Dredd class JS API to support custom configuration data e.g. for reporters

My 0,02$:

• Please never refactor and introduce new functionality in one PR
• It's fine that junk form bin/dredd was refactored and sliced into separate, more readable, JS testable layer under DreddCommand class
• It's not fine that test/unit/command-test.coffee does not test functionality in DreddCommand, it's only a copy of CLI integration test which is end-to-end testing whole Dredd functionality through CLI interface. Unit tests for DreddCommand should test functionality and units in DreddComand class, such is:
  • Handling expanded globs from shell and filling option['path']
  • Showing help
  • Showing version
  • Error handling on missing arguments for blueprint and server
  • Running Dredd
  • Error handling for Dredd#run
• It's** not** fine extending surface of DreddCommand class interface only for purpose of testing, it should be through mocking output from process.cwd etc..

[kuba-kubula]

@netmilk Understood most of the problems about "why is there console, why is there the config.options.custom.env.

Unfortunately, you can't simply mock process or even process.stdout. Those two can't be even cloned. If you try to mock these two, you are facing problems deep in your dependencies. Really.
There are some articles about it, programers suggest to actually pass console or stdout into the script, and work on top of that. That is the reason I am passing stdout or console separately.
http://yahooeng.tumblr.com/post/75054690857/code-coverage-for-executable-node-js-scripts

Other option would be to use config.custom instead of taking it from the really deep config.options.custom.

Why I chose custom.env? Because there are other possible things you actually want to wrap. One is env, the other one would be a stdout, then maybe anything else like process.exit function? The list gets longer really soon.

[netmilk]

Ok, thanks for the explanation. In my opinion it's not needed at all, because if you want to test behavior of DreddCommand class, you can spy on console.log and use proxyquire with stubs and spies for loading dredd-command module. Get rid with it please and please don't be afraid to write proper unit tests. ;p

[netmilk]

👍

[netmilk]

Get what? I prefer to name methods/functions based on what they are doing.