Spawn Mocha instead of using its API #151

shellscape · 2017-02-09T17:50:58Z

this PR fixes #20 and provides resolution to most of the other remaining issues.

notes:

using through2 instead of through
require and cache features removed as they were no longer needed
tests have been updated to reflect changes
emitting error and result events. result events provides for testing the command result without having to override output methods.
tabs -> spaces and minor XO rule changes (I'll change this back if there is pushback)

this is a first-pass, and welcoming comments.

ferlores · 2017-02-09T20:42:13Z

+1 we are having the same problems and it seems this will fix it

sindresorhus · 2017-02-13T18:50:00Z

Good start. Thanks for working on this 🎉

Can you make sure the options in the readme matches the CLI flags (in a camelCased version)?

tabs -> spaces and minor XO rule changes (I'll change this back if there is pushback)

Should be changed back. Use EditorConfig and $ xo --fix and you don't even have to think about it being tab indented ;)

sindresorhus · 2017-02-13T18:41:41Z

.gitignore

@@ -1 +1,2 @@
 node_modules
+*-backup.*


This should be in your own local gitignore.

sindresorhus · 2017-02-13T18:41:57Z

.travis.yml

-  - '0.10'
+  - 4
+  - 6
+  - 7


These needs to be quoted

curious. none of my other projects have these quoted and travis works as expected. please do share why

YAML interprets them as number, but versions are not number in this sense. They should be quoted for consistency. It's only a quirk that they work now.

good to know, thanks

sindresorhus · 2017-02-13T18:43:38Z

index.js

+  if (options.suppress) {
+    suppress = true;
+    delete options.suppress;
+  }


Instead of this, just use the excludes option in dargs.

sindresorhus · 2017-02-13T18:44:06Z

index.js

-			}
-		}
-	}
+  options = Object.assign({colors: true}, options || {});


options || {} => options

Object.assign handles it.

good to know. old school habits persist hard :)

sindresorhus · 2017-02-13T18:45:19Z

package.json

@@ -1,6 +1,6 @@
 {
  "name": "gulp-mocha",
-  "version": "3.0.1",
+  "version": "4.0.0",


Don't bump this. We'll do it at publish time.

cool. will revert

sindresorhus · 2017-02-13T18:45:28Z

package.json

@@ -10,7 +10,7 @@
    "url": "sindresorhus.com"
  },
  "engines": {
-    "node": ">=0.10.0"
+    "node": ">=4.6.2"


"node": ">=4"

sindresorhus · 2017-02-13T18:46:16Z

package.json

  },
  "devDependencies": {
+    "mocha": "^3.0.0",


mocha should be a dependency

ah. I had thought the ecosystem was moving towards the pattern that gulp-jshint is using now; where jshint is a peerDep. Will remedy.

Peerdependencies are annoying for everyone, now that npm@3 dedupes by default, the user can just add mocha to their own package.json with a more specific version and it will be deduped to that version and we'll use it too as long as it's in the v3 range.

shellscape · 2017-02-17T13:40:20Z

requested changes made. I manually verified the command line options from camelCase. didn't see a need to add that to tests as dargs tests handle that.

sindresorhus · 2017-02-20T06:55:03Z

index.js

+          process.stdout.write(result.stdout);
+        }
+
+        this.emit('result', result);


This needs to be documented.

was only meant to be used internally for tests

Then it should be prefixed with an underscore: _result

sindresorhus · 2017-02-20T06:55:29Z

index.js

+
+  options = Object.assign(defaults, options);
+
+  if (Object.prototype.toString.call(options.globals) === '[object Array]') {


Use Array.isArray()

that's not compatible with with node < 6. http://node.green/#Array-isArray-support

Yes, it is. You're looking at proxy support. Array.isArray() has been available since at least Node.js 0.10.

Yep, my fault there.

sindresorhus · 2017-02-20T06:56:00Z

readme.md

@@ -42,6 +42,10 @@ gulp.task('default', () =>

 #### options

+gulp-mocha will pass any options defined directly to the `mocha` binary. That
+means you have every [command line option](http://mochajs.org/#usage) available


Needs to be explicit about it users using a camelCased version of those command line options.

dargs doesn't require that you use only camelCased args, it treats the arguments the same if they're in sausage case (the format of the mocha args) as well.

That's not the point. Users don't want to use quotes in the keys. And they wouldn't even know camelCase is supported.

shellscape · 2017-02-20T13:20:53Z

I'm going to leave this to someone else to touch up the finer, stylistic comments on this PR. The high standards you hold this to are admirable, but just a tad on the nitpicky side of the line for my tastes. The turn-around time on review for the stylistic change requests has been a bit long as well. And that's said completely devoid of any amount of sarcasm, snide, ill-will, or negativity. I'm extremely grateful for your time and massive contribution to the node ecosystem.

sindresorhus · 2017-02-20T14:43:54Z

@shellscape I did a thorough review because I thought you wanted to learn. It would have indeed been much much faster for me to just fix everything after merging. You shouldn't expect maintainers to clean up your mess though. Pull requests reviews are meant to get the code into an acceptable state before being merged into master. Once it's in master, it's the maintainers job to maintain it, so obviously they want it to be good before it ends up there.

I appreciate the pull request though :)

shellscape · 2017-02-20T18:42:14Z

I appreciate the sentiment. While I'm always expanding what I know, I consider myself to be a seasoned developer (your opinion may vary, and that's OK). This wasn't a learning exercise for me, so to speak. I was interested in coming on as a maintainer, which is why I contacted and spoke about what direction the code needed to go in, rather than releasing a different module. And in that role (a maintainer) I would have been very comfortable with the PR as it was this morning. That moves to highlight our differences, which is OK. If you're ever to the point where you're good letting go of the reigns, give me a shout.

shellscape added 2 commits February 9, 2017 12:43

refactor to spawn the mocha process

9e579a3

removing unused deps

ab32ebf

shellscape requested a review from sindresorhus February 9, 2017 17:51

shellscape self-assigned this Feb 9, 2017

shellscape added the enhancement label Feb 9, 2017

shellscape mentioned this pull request Feb 9, 2017

gulp-mocha dies silently when running too many test files #52

Closed

sindresorhus requested changes Feb 13, 2017

View reviewed changes

sindresorhus changed the title ~~Spawn mocha~~ Spawn Mocha instead of using its API Feb 13, 2017

requested changes

4a98f7b

sindresorhus requested changes Feb 20, 2017

View reviewed changes

sindresorhus approved these changes Feb 20, 2017

View reviewed changes

sindresorhus merged commit 3e55175 into master Feb 20, 2017

sindresorhus deleted the spawn-mocha branch February 20, 2017 14:44

sindresorhus added a commit that referenced this pull request Feb 20, 2017

Cleanup #151

3a48183

sindresorhus added a commit that referenced this pull request Feb 20, 2017

Cleanup #151

f9de4ec

glenjamin mentioned this pull request Feb 22, 2017

mocha-multi incompatible with latest mocha (gulp) glenjamin/mocha-multi#26

Closed

vercruyssetim mentioned this pull request Mar 21, 2017

Gulp-mocha doesn't pass reporterOptions #167

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Spawn Mocha instead of using its API #151

Spawn Mocha instead of using its API #151

shellscape commented Feb 9, 2017 •

edited by sindresorhus

Loading

ferlores commented Feb 9, 2017

sindresorhus commented Feb 13, 2017

sindresorhus Feb 13, 2017

shellscape Feb 13, 2017

sindresorhus Feb 13, 2017

shellscape Feb 13, 2017

sindresorhus Feb 14, 2017

shellscape Feb 14, 2017

sindresorhus Feb 13, 2017

shellscape Feb 13, 2017

sindresorhus Feb 13, 2017

shellscape Feb 13, 2017

sindresorhus Feb 13, 2017

shellscape Feb 13, 2017

sindresorhus Feb 13, 2017

sindresorhus Feb 13, 2017

shellscape Feb 13, 2017

sindresorhus Feb 14, 2017

shellscape commented Feb 17, 2017

sindresorhus Feb 20, 2017

shellscape Feb 20, 2017

sindresorhus Feb 20, 2017

sindresorhus Feb 20, 2017

shellscape Feb 20, 2017

sindresorhus Feb 20, 2017

shellscape Feb 20, 2017

sindresorhus Feb 20, 2017

shellscape Feb 20, 2017

sindresorhus Feb 20, 2017

shellscape commented Feb 20, 2017

sindresorhus commented Feb 20, 2017 •

edited

Loading

shellscape commented Feb 20, 2017 •

edited

Loading


		options = Object.assign(defaults, options);

		if (Object.prototype.toString.call(options.globals) === '[object Array]') {

Spawn Mocha instead of using its API #151

Spawn Mocha instead of using its API #151

Conversation

shellscape commented Feb 9, 2017 • edited by sindresorhus Loading

ferlores commented Feb 9, 2017

sindresorhus commented Feb 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shellscape commented Feb 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shellscape commented Feb 20, 2017

sindresorhus commented Feb 20, 2017 • edited Loading

shellscape commented Feb 20, 2017 • edited Loading

shellscape commented Feb 9, 2017 •

edited by sindresorhus

Loading

sindresorhus commented Feb 20, 2017 •

edited

Loading

shellscape commented Feb 20, 2017 •

edited

Loading