sysgears / mochapack

Mocha test runner with integrated webpack precompiler
https://sysgears.github.io/mochapack/
MIT License
183 stars 28 forks source link

Mocha CLI Args #63

Closed Jack-Barry closed 4 years ago

Jack-Barry commented 4 years ago

What's the problem this PR addresses?

Unfortunately, solving any one of these issues on its own wasn't necessarily an easy route to go, so they're all in a pretty big PR, however all issues needed to be addressed in some capacity.

How did you fix it?

I'd like to ask for help from anyone interested to test this and/or contribute automated tests for particular arguments that you'd like to see covered. I didn't write any new tests for new arguments, however you can see which arguments are not passed directly to the Mocha constructor in this types.ts file. The arguments in the MochaCliOptions interface must be handled in some way by Mochapack itself, which happens in the cli and initMocha functions. All other arguments are passed directly to the Mocha constructor, so any options that can be passed to it in a MochaOptions object are directly handed off in initMocha.

This makes it very clear whether Mochapack needs to do some special handling of Mocha args (that impact the CLI usage) or it just needs a quick update to the mochaOptions.ts file (until hopefully this gets merged, in which case Mochapack no longer has to update references to Mocha's CLI args).

Jack-Barry commented 4 years ago

From the Travis run, looks like a few issues will need to be addressed:

  1. Since the goal is to minimize the amount of duplicate work, some files are imported from Mocha now. The lib/cli/run-option-metadata.js, lib/cli/config.js, lib/mocharc.json, lib/errors.js, lib/cli/one-and-dones.js, and lib/cli/run-helpers.js files used in the new args parsing mechanism look like they were introduced at some point in Mocha version 5.2.0, so this breaks compatibility with Mocha 4 unless we want to reimplement the contents of those files. Are there any strong objections to dropping Mocha 4 support? Seems like the quickest fix would be to drop Mocha 4 support and ensure that Travis runs with 5.2.0 or above. (The last release of Mocha within version 4 was December 2017.)
  2. I'm not sure what's happening with Mocha 6 🤷🏻‍♂️ Probably related to a difference in the files mentioned above.
  3. The tests for initMocha need to have a less strict check of equality for the expected output

Issue 1 I just need to know if it's alright to drop Mocha 4 🤞🏼 and issue 2 if anybody can take a look at that and see if it's clear what the reason for failure is I'd appreciate it.

I should be able to address issue 3 pretty quickly tomorrow.

larixer commented 4 years ago

@Jack-Barry 1. Yep, lets bump mochapack to a new major version and drop Mocha 4

codecov-io commented 4 years ago

Codecov Report

Merging #63 into master will increase coverage by 13.99%. The diff coverage is 87.87%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #63       +/-   ##
===========================================
+ Coverage   51.13%   65.13%   +13.99%     
===========================================
  Files          26       42       +16     
  Lines         878     1219      +341     
  Branches      125      135       +10     
===========================================
+ Hits          449      794      +345     
+ Misses        414      403       -11     
- Partials       15       22        +7     
Impacted Files Coverage Δ
src/runner/runnerUtils/initMocha/loadUI.ts 100.00% <ø> (ø)
src/runner/testRunnerReporter.ts 15.11% <10.00%> (+0.99%) :arrow_up:
src/runner/TestRunner.ts 16.92% <11.11%> (+0.48%) :arrow_up:
src/webpack/compiler/registerReadyCallback.ts 22.22% <33.33%> (+9.72%) :arrow_up:
src/webpack/compiler/registerInMemoryCompiler.ts 18.91% <66.66%> (+2.25%) :arrow_up:
...c/cli/argsParser/parseArgv/mocha/parseMochaArgs.ts 70.96% <70.96%> (ø)
src/Mochapack.ts 75.00% <75.00%> (ø)
src/cli/argsParser/parseArgv/mocha/mochaOptions.ts 81.25% <81.25%> (ø)
src/runner/runnerUtils/initMocha/initMocha.test.ts 84.31% <84.31%> (ø)
...romParsedArgs/mocha/mergeMochaConfigWithOptions.ts 85.00% <85.00%> (ø)
... and 43 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update ff12edd...9885c83. Read the comment docs.

Jack-Barry commented 4 years ago

Some egg on my face: The files imported from Mocha as part of this PR were added while their repo was under 5.2.0, but not made available in their published package until 6.0.0. Since 5.2.0 was the last release of 5 and was published in May of 2018, I don't think it's a huge difference. But the tests are now configured to run under Mocha 6 and 7, and the package.json for Mochapack reflects the peer dependency should be Mocha 6 or 7.

If all's clear here, all that's left is a version bump. (Figured I'd save that until everything was resolved).

larixer commented 4 years ago

I remember about this PR, but its quite big and I'm very busy this week. Hopefully I can review it the next week.

Jack-Barry commented 4 years ago

Sounds good, thanks for the update amigo

larixer commented 4 years ago

@Jack-Barry Bravo! Awesome work, thank you so much!

larixer commented 4 years ago

Published new major version: mochapack@2.0.0

larixer commented 4 years ago

@Jack-Barry Could you check and close issues that should be fixed by your PR in a new major version, or maybe just provide some feedback in the issues that have some impact from your PR?

Jack-Barry commented 4 years ago

@larixer Thanks for merging this in, will make sure to make notes in applicable issue threads.

Looks like I broke something here as per #65 but I figured there would be an untested bug or two to pop up. Will look into that one once a repro project is available or somebody else can provide a similar problem repro project.