Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for addFiles config parameter. #8

Merged
merged 1 commit into from Feb 20, 2018

Conversation

hswolff
Copy link
Contributor

@hswolff hswolff commented Dec 20, 2017

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
Copy link
Collaborator

ljharb commented Dec 20, 2017

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

@hswolff
Copy link
Contributor Author

hswolff commented Dec 21, 2017

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
Copy link
Collaborator

ljharb commented Dec 21, 2017

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
Copy link
Contributor Author

hswolff commented Dec 21, 2017

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
Copy link
Owner

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
Copy link
Collaborator

ljharb commented Dec 26, 2017

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
Copy link
Contributor Author

hswolff commented Dec 27, 2017

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
Copy link
Contributor Author

hswolff commented Dec 27, 2017

Opened mocha issue for discussion: mochajs/mocha#3181

@ljharb
Copy link
Collaborator

ljharb commented Dec 27, 2017

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
Copy link
Contributor Author

hswolff commented Jan 18, 2018

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
Copy link
Collaborator

ljharb commented Jan 18, 2018

"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
Copy link
Contributor Author

hswolff commented Feb 9, 2018

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 rogeliog merged commit 105943c into rogeliog:master Feb 20, 2018
@hswolff hswolff deleted the add-files branch February 20, 2018 21:07
@rogeliog
Copy link
Owner

@hswolff Just published 0.5.0 which includes this and #9

@hswolff
Copy link
Contributor Author

hswolff commented Feb 20, 2018

You da real MVP.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants