rogeliog / jest-runner-mocha

A Mocha runner for Jest
70 stars 12 forks source link

Add support for addFiles config parameter. #8

Closed hswolff closed 6 years ago

hswolff commented 6 years ago

Hello!

First of all, thank you for this project! It's truly saving me from laborious long test runs when using mocha directly.

For the tests that I run I have the need to run additional files before each test file to set up some additional Mocha hooks.

i.e. in one file I have a

before(function() {
  // Do stuff before any of my tests run.
});

I was able to add this functionality through this change.

Sorry about not creating an issue first. I've been hacking on this all this day to get this working with our test suite and this is part 1 of 2 of changes that I'd love to upstream.

ljharb commented 6 years ago

This seems like the sort of thing that mocha itself should support directly?

hswolff commented 6 years ago

Mocha supports require however that includes the file prior to the mocha test suite being created. We require this to be included after the mocha test suite exists.

ljharb commented 6 years ago

I totally agree and understand; airbnb does this by defining a mocha script that does mocha setupFile.js and then in the "test" script, npm run mocha path/to/files.

I'm more saying that rather than patching it into this one runner, maybe it'd be better added to mocha core :-)

hswolff commented 6 years ago

Aha, now I understand. Thank you for the clarification. (We do the same thing at Mongo, with a mocha script).

I agree, would definitely be better in core. I'll look into filing an issue in the mocha repo for this.

rogeliog commented 6 years ago

Hi! Thanks for reporting and sorry for the late response, I've been sick

Thanks for working on this! It looks good to me but I would love @ljharb to approve it before merging

ljharb commented 6 years ago

I'm not going to block it or anything; but I think that this sort of thing should wait on being supported in actual mocha and not just in the runner.

hswolff commented 6 years ago

So I'm writing up the issue on the Mocha repo however I just realized that this runner is not using mocha via the CLI, we're using it via the node API, which does support the ability to addFile as used in this ticket.

I'm a little lost how adding CLI support to mocha would benefit the runner? It would allow us to keep all arguments via the cliOption object? That seems like an artificial constraint.

hswolff commented 6 years ago

Opened mocha issue for discussion: https://github.com/mochajs/mocha/issues/3181

ljharb commented 6 years ago

I wasn’t aware mocha had this facility; that it does, and we’d match it’s name, sounds good to me.

The reason to wait would be so that this runner had no command line/config options that mocha lacks, besides those that are unique to this runner.

hswolff commented 6 years ago

Mocha 5 was just released with the addition of the --file arg!

I'm planning on updating this PR to make use of that new flag however that will require using at least Mocha 5.

I know @ljharb has an interest in keeping support back to node 0.10 (I'm curious as to the why behind supporting an unsupported version of node) so I'm looking for some guidance on how to move forward.

One thought would be to move mocha to a peerDependency of mocha >3 and then have mocha 5.x installed as a dev dependency to test out the functionality.

Any other suggestions?

ljharb commented 6 years ago

"supported version of node" is something that node core deals with; that a node version is unsupported in no way means individual tools must, or even should, drop support for that version.

Since this runner uses the API, and not the cli, there's no reason it should use the --file command; however, the existence of that arg in mocha 5 means that this runner has a clear guide as to the name and semantics of this PR's config parameter.

I think that indeed the proper way to test this out is to run travis builds in all of mocha 3, 4, and 5, but only run the 4 and 5 versions in node 4+ (and to set the peer dep not to >=, ever, but to ^3 || ^4 || ^5).

hswolff commented 6 years ago

Updated to follow the mocha CLI option of file.

Let me know if you want me to include the travis changes in this ticket as well. Seems unnecessary as we are just using the node API.

rogeliog commented 6 years ago

@hswolff Just published 0.5.0 which includes this and #9

hswolff commented 6 years ago

You da real MVP.