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.

Sign up for GitHub

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

Jump to bottom

fix improper warnings for invalid reporters #4275

Merged
merged 3 commits into from
May 14, 2020
Merged

fix improper warnings for invalid reporters #4275

merged 3 commits into from
May 14, 2020

Conversation

boneskull
Copy link
Contributor

also: reorganize, add, and refactor a bunch of problematic unit tests for lib/mocha.js. better isolation except where we can't really do that (calling require)

there's still missing tests in here, but this is an improvement.

@craigtaub this touches some stuff regarding warnings for invalid reporters. it looks like the conditionals were backwards?

@boneskull boneskull added type: bug a defect, confirmed by a maintainer type: chore generally involving deps, tooling, configuration, etc. semver-patch implementation requires increase of "patch" version number; "bug fixes" labels May 8, 2020
@boneskull boneskull requested a review from craigtaub May 8, 2020 21:27
@boneskull boneskull self-assigned this May 8, 2020
@boneskull boneskull force-pushed the boneskull/mocha-unit-tests branch from 65b5a7d to 1ba7cc6 Compare May 8, 2020 21:30
@boneskull
Copy link
Contributor Author

I should note that I started out adding more coverage and refactoring, but I found a bug while I was doing that...

@craigtaub
Copy link
Contributor

craigtaub commented May 9, 2020 •
edited
Loading

Odd the logic been broken for so long for such an important check. Would have been good to see an issue or details in the description to help replicate locally.

Would this logic apply all the time or just programmatic use of mocha only? (cli catches bad reporters when I try locally)

craigtaub
craigtaub reviewed May 9, 2020
View reviewed changes
lib/mocha.js Show resolved Hide resolved
@boneskull
Copy link
Contributor Author

can't use require.resolve in unit tests because browser. will need to move some of these into test/node-unit/mocha.spec.js

@boneskull boneskull force-pushed the boneskull/mocha-unit-tests branch from 1ba7cc6 to 89cb1e7 Compare May 11, 2020 19:01
@boneskull
Copy link
Contributor Author

OK, we can't use rewiremock in test/unit/*.spec.js because it's apparently incompatible with browserify as written. so I've ripped it out of there, and moved some tests for Mocha#reporter() into test/node-unit/mocha.spec.js where we can safely test reporter resolution.

Ref: theKashey/rewiremock#113

cc @Munter -- how's the transpilation coming? 😄

@coveralls
Copy link

coveralls commented May 11, 2020 •
edited
Loading

Coverage Status

Coverage increased (+0.08%) to 93.228% when pulling 0bd7e14 on boneskull/mocha-unit-tests into 81e203c on master.

@boneskull
Copy link
Contributor Author

tests broke because you can't use Object.assign() in IE11 (I forgot), but I noticed we also didn't have a lint rule preventing it, so I added one.

@boneskull boneskull force-pushed the boneskull/mocha-unit-tests branch from 263c7b1 to 0929e14 Compare May 12, 2020 19:03
@boneskull
Copy link
Contributor Author

@nicojs I had heavily refactored test/unit/mocha.spec.js, where most of your new tests live--so they, too, are assimilated in this PR. May want to take a look.

@nicojs
Copy link
Contributor

nicojs commented May 12, 2020

@boneskull You're not kidding, file looks like a Christmas tree in my browser 🎄. Will checkout this branch locally to take a better look. Will have to wait till tomorrow.

@boneskull boneskull force-pushed the boneskull/mocha-unit-tests branch from 0929e14 to f210e64 Compare May 12, 2020 21:15
@boneskull
Copy link
Contributor Author

your changes highlight that Mocha.prototype.run has a very strange API.

It accepts an optional callback, and that callback is called with a single parameter, failures, a number. This doesn't conform to a typical convention. If it did use an error-first style, that convention expects the function to not throw, and rather call the callback w/ an Error instance.

It would likely be highly disruptive to break this, unfortunately. Off the top of my head, two ways we could improve the situation:

  1. The way forward may instead be to create a new runAsync (or whatever) in the future and return a Promise. If the Runner instance is needed (the current return value) then this would not be appropriate.
  2. In Mocha.prototype.run, inspect the callback. If it exists and has an arity greater than 1, use an error-first style; otherwise use the current behavior.

Consumers of Mocha.prototype.run don't currently expect it to throw (for any reason), and the current situation may be awkward to work with.

@boneskull boneskull force-pushed the boneskull/mocha-unit-tests branch from f210e64 to cf53102 Compare May 12, 2020 21:25
nicojs
nicojs approved these changes May 13, 2020
View reviewed changes
Copy link
Contributor

@nicojs nicojs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Some minor suggestions.

test/node-unit/mocha.spec.js Outdated Show resolved Hide resolved
test/node-unit/mocha.spec.js
});

describe('when the reporter throws upon load', function() {
it('should throw "invalid reporter" exception', function() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love the fact that this is split out from the should warn ... test 😍. Also for the mocha unit tests.

boneskull added 2 commits May 14, 2020 12:24
@boneskull
fix improper warnings for invalid reporters
5b7798a 
also: reorganize, add, and refactor a bunch of problematic unit tests for `lib/mocha.js`.  better isolation except where we can't really do that (calling `require`)

there's still missing tests in here, but this is an improvement.
@boneskull
restrict use of Object.assign
4f01095
@boneskull boneskull force-pushed the boneskull/mocha-unit-tests branch from cf53102 to 4f01095 Compare May 14, 2020 19:24
@boneskull
add wrapper around process.cwd()
0bd7e14 
- better isolation
- a use of `process.cwd()` in `lib/runner.js` was left in place to avoid conflicts, because another PR will remove it
@boneskull boneskull merged commit 1a4646d into master May 14, 2020
@boneskull boneskull deleted the boneskull/mocha-unit-tests branch May 14, 2020 21:49
@craigtaub craigtaub added this to the next milestone May 21, 2020
craigtaub pushed a commit that referenced this pull request May 21, 2020
@boneskull @craigtaub
fix improper warnings for invalid reporters (#4275)
ab59bfa 
* fix improper warnings for invalid reporters

    also: reorganize, add, and refactor a bunch of problematic unit tests for `lib/mocha.js`.  better isolation except where we can't really do that (calling `require`)

    there's still missing tests in here, but this is an improvement.

* restrict use of Object.assign in ESLint config
* add wrapper around process.cwd()
    - better isolation
    - a use of `process.cwd()` in `lib/runner.js` was left in place to avoid conflicts, because another PR will remove it
@Omrisnyk Omrisnyk mentioned this pull request Dec 7, 2023
Open
@tpicard586 tpicard586 mentioned this pull request Jul 21, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@craigtaub craigtaub craigtaub approved these changes

@nicojs nicojs nicojs approved these changes

Assignees

@boneskull boneskull

Labels
semver-patch implementation requires increase of "patch" version number; "bug fixes" type: bug a defect, confirmed by a maintainer type: chore generally involving deps, tooling, configuration, etc.
Projects
None yet
Milestone
v7.2.0
Development

Successfully merging this pull request may close these issues.
4 participants
@boneskull @craigtaub @coveralls @nicojs