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

Fix #4348 #4383

Merged
merged 13 commits into from Jul 29, 2020
Merged

Fix #4348 #4383

merged 13 commits into from Jul 29, 2020

Conversation

cspotcode
Copy link
Contributor

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions.

Description of the Change

Rewrites a hook's title always, instead of only when the hook fails. Fixes issues described in #4348.

Alternate Designs

None

Why should this be in core?

It's a bug as described in #4348

Benefits

Bug is fixed

Possible Drawbacks

Any change is potentially breaking, even a bugfix.

Applicable issues

#4348

  • Is this a breaking change (major release)? no
  • Is it an enhancement (minor release)? no
  • Is it a bug fix, or does it not impact production code (patch release)? yes

@cspotcode cspotcode changed the title Fix #4348 WIP Fix #4348 Jul 27, 2020
@coveralls
Copy link

coveralls commented Jul 27, 2020

Coverage Status

Coverage increased (+0.004%) to 93.936% when pulling 46cf90a on cspotcode:fix-4348 into 02bdb6b on mochajs:master.

@cspotcode cspotcode changed the title WIP Fix #4348 Fix #4348 Jul 27, 2020
@cspotcode
Copy link
Contributor Author

@boneskull this is ready for review.

Copy link
Member

@juergba juergba left a comment

Choose a reason for hiding this comment

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

IMO we should not change the hook title like this:

  • it's the one defined by the user and we have Runnable#fullTitle to get the title incl. all suites names.
  • does grep/fgrep still work?
  • for beforeAll/afterAll hooks, the suite defines the context and not a test.
  • we have no primary keys for our tests/hooks yet, e.g. for re-running specific tests. Maybe we should think about introducing ID's, using a string title for this purpose doesn't seem a good solution.

@@ -514,8 +502,25 @@ Runner.prototype.hook = function(name, fn) {
}
self.emit(constants.EVENT_HOOK_END, hook);
delete hook.ctx.currentTest;
setHookTitle(hook);
Copy link
Member

Choose a reason for hiding this comment

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

What is this for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When hook.ctx.currentTest is deleted, this changes the hook's title so that it no longer mentions the test case and only mentions the test suite. It affects the "multiple done calls" errors.

@cspotcode
Copy link
Contributor Author

@juergba it sounds like you're saying that the title rewriting, which mocha is already doing, is bad. Do you want to submit an alternative PR which fixes that? Do we know why the current rewriting behavior was implemented?

@juergba
Copy link
Member

juergba commented Jul 28, 2020

No, not bad. When a hook fails, normally no more related tests are executed. E.g. when a beforeAll hook fails, all corresponding tests "disappear". Renaming such a failing hook is a different thing than renaming all hooks. It's easier to identify the failing hook in the reporter output like this.

Actually I don't like how failing beforeAll and afterAll hooks are renamed, based on hook.ctx.currentTest.
Setting if (name === HOOK_TYPE_BEFORE_ALL) { hook.ctx.currentTest = hook.parent.tests[0]; } ... is not really correct. The context should be the suite not the first/last test. But this topic is out of scope and should be checked carefully before changing anything.

@cspotcode
Copy link
Contributor Author

cspotcode commented Jul 28, 2020 via email

Copy link
Member

@boneskull boneskull left a comment

Choose a reason for hiding this comment

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

thanks! this looks good. the only blocker would be the wording on the unintelligible error message that I commented on. feel free to suggest whatever makes sense to you.

otherwise just a couple suggestions/comments.

test/integration/multiple-done.spec.js Outdated Show resolved Hide resolved
fullTitle: 'suite "before each" hook in "suite"',
err: {
message: /done\(\) called multiple times in hook <suite "before each" hook in "suite"> of file.+multiple-done-before-each\.fixture\.js/,
multiple: [
Copy link
Member

Choose a reason for hiding this comment

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

this error object is weird, but I guess that's what it does.

you can combine this into a single assertion:

expect(res.failures[0], 'to equal', res.failures[1])
  .and('to satisfy', {
    // etc.
  });

Copy link
Member

Choose a reason for hiding this comment

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

(likewise the assertion(s) in the new test may be able to be combined this way)

Copy link
Contributor Author

@cspotcode cspotcode Jul 28, 2020

Choose a reason for hiding this comment

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

Yeah, you probably already know this, but I rewrote the assertion to point out that both items of the array actually refer to the same object. I can't do a === check in the tests, because it's been converted to/from JSON, so the assertions are running against 2 different parsed copies of the same object. Does 'to equal' handle that? EDIT: disregard; I'm already using 'to equal' I see what you're saying.

This happens because when a single test/hook has multiple failures, the first error is reused, and the multiple array is created to store the others. Ideally I think mocha should be creating a new "hookinvocation" instance for each time the hook runs, where each "hookinvocation" can store a reference to the test case it's paired to. But I don't think I'll have time for that size of code change.

lib/runner.js Outdated
@@ -419,20 +419,6 @@ Runner.prototype.fail = function(test, err, force) {
* @param {Error} err
*/
Runner.prototype.failHook = function(hook, err) {
Copy link
Member

Choose a reason for hiding this comment

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

could probably just remove failHook and replace its usage with calls directly to fail

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do.

Copy link
Member

@boneskull boneskull left a comment

Choose a reason for hiding this comment

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

OK, n/m, LGTM unless you want to address my two suggestions (removing failHook and updating the assertion/assertions)

@cspotcode
Copy link
Contributor Author

@boneskull thanks, I think I addressed all your feedback and tests are passing.

@boneskull boneskull merged commit 7884893 into mochajs:master Jul 29, 2020
@boneskull boneskull added this to the next milestone Jul 29, 2020
@boneskull boneskull added type: bug a defect, confirmed by a maintainer semver-patch implementation requires increase of "patch" version number; "bug fixes" labels Jul 29, 2020
@boneskull boneskull linked an issue Jul 29, 2020 that may be closed by this pull request
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch implementation requires increase of "patch" version number; "bug fixes" type: bug a defect, confirmed by a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hook only renamed when it fails, not when it begins and ends successfully
4 participants