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

split _mocha file #3420

Closed
wants to merge 3 commits into from
Closed

split _mocha file #3420

wants to merge 3 commits into from

Conversation

Bamieh
Copy link
Contributor

@Bamieh Bamieh commented Jun 20, 2018

The change

bin/_mocha file is responsible for both configuring mocha from the cli flags and running mocha. This causes an issue, every tool that wants to add on top of the mocha cli must copy-paste the bin/_mocha code in their own repos.

This change separates the bin/_mocha file to two files.

  • bin/base.js file which collects the cli flags and creates a configured mocha instance.
  • bin/_mocha file which requires bin/base.js and runs the exported mocha instance.

Advantages:

  • Libraries now can require mocha/bin/base instead of copy-pasting the code.
  • I also needed this for mocha-watcher.
  • separate concerns for easier code comprehension and maintenance.
  • non-breaking change, just a win-win

@Bamieh Bamieh added semver-minor implementation requires increase of "minor" version number; "features" type: cleanup a refactor area: node.js command-line-or-Node.js-specific labels Jun 20, 2018
@coveralls
Copy link

coveralls commented Jun 20, 2018

Coverage Status

Coverage decreased (-61.8%) to 28.218% when pulling 6f7ff71 on _mocha into babd958 on master.

@Bamieh Bamieh changed the title separate _mocha file split _mocha file Jun 21, 2018
@outsideris
Copy link
Member

Is this related with #2292 or #2578 ?

@outsideris
Copy link
Member

outsideris commented Jun 21, 2018

Why the coverage decreased like that?

@Bamieh
Copy link
Contributor Author

Bamieh commented Jun 21, 2018

@outsideris i'm not sure! i added the file to the ignore but still, i'll look into it tomorrow

@Bamieh
Copy link
Contributor Author

Bamieh commented Jun 21, 2018

@outsideris i believe this is related to #2578 since it makes the CLI more usable, since the base file now exports the program and the configured mocha. Anyone that wants to run mocha from their own CLI tool can simply import the base.

I would definitely love to merge bin/_mocha and bin/mocha but this PR is not related to it.

@boneskull
Copy link
Member

I'm going to guess the coverage drop (the Travis PR build shows many tests skipped) is because the files array is simply discarded in bin/base.js.

I'm not sure why some stuff is in bin/base.js and other stuff remains in bin/_mocha.

Would prefer bin/base.js had a more descriptive name and actually lived in lib/ (as well as bin/options.js).

@Bamieh
Copy link
Contributor Author

Bamieh commented Jun 26, 2018

@boneskull the base.js is responsible only for creating a mocha instance and configuring it. The _mocha file requires this file and runs the mocha instance, it also adds a custom behavior to what happens on exit and what to do in the watch mode.

I am +1 for moving these .js files to lib/bin/

@craigtaub
Copy link
Contributor

@Bamieh does this change still apply now we have the separation inside /cli!?

@boneskull
Copy link
Member

It's likely invalid, but I'd like to know if the refactors around yargs and such solve the problem about wrapping the Mocha CLI...

@boneskull
Copy link
Member

closed by #3556

@boneskull boneskull closed this Jan 15, 2019
@boneskull boneskull deleted the _mocha branch January 15, 2019 17:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: node.js command-line-or-Node.js-specific semver-minor implementation requires increase of "minor" version number; "features" type: cleanup a refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants