Fix #4348 #4383

cspotcode · 2020-07-26T18:58:52Z

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

coveralls · 2020-07-27T05:40:14Z

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

cspotcode · 2020-07-27T14:28:45Z

@boneskull this is ready for review.

juergba

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.

juergba · 2020-07-28T16:05:02Z

lib/runner.js

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


What is this for?

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 · 2020-07-28T16:44:09Z

@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 · 2020-07-28T17:33:04Z

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 · 2020-07-28T17:38:54Z

Right, so the current title rewriting is bad and should be changed. What's the right approach for teams affected by #4348?

…

On Tue, Jul 28, 2020, 1:33 PM Juerg B. ***@***.***> wrote: 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. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#4383 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAC35ODA7FHWBGUSAW7WCNTR54DWBANCNFSM4PIDQ5WA> .

boneskull

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

boneskull · 2020-07-28T20:38:14Z

test/integration/multiple-done.spec.js

+        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: [


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. });

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

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.

boneskull · 2020-07-28T20:39:48Z

lib/runner.js

@@ -419,20 +419,6 @@ Runner.prototype.fail = function(test, err, force) {
 * @param {Error} err
 */
 Runner.prototype.failHook = function(hook, err) {


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

boneskull

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

cspotcode · 2020-07-28T22:58:28Z

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

cspotcode added 8 commits July 26, 2020 14:56

fix mochajs#4348

fe806f4

fix

2e03e57

fix

597597e

fix

002c432

fix

680b14f

fix

aea7b65

fix

3ad9fa3

fix

31febbf

cspotcode changed the title ~~Fix #4348~~ WIP Fix #4348 Jul 27, 2020

cspotcode added 2 commits July 27, 2020 01:27

fix

c149bdb

fix

46cf90a

cspotcode changed the title ~~WIP Fix #4348~~ Fix #4348 Jul 27, 2020

juergba reviewed Jul 28, 2020

View reviewed changes

boneskull suggested changes Jul 28, 2020

View reviewed changes

boneskull approved these changes Jul 28, 2020

View reviewed changes

cspotcode added 3 commits July 28, 2020 17:15

Addressing PR feedback

299cfa4

rename "suite" to "suite1" to make test cases clearer

564b189

addressing PR feedback

fe88c03

boneskull merged commit 7884893 into mochajs:master Jul 29, 2020

boneskull added this to the next milestone Jul 29, 2020

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 linked an issue Jul 29, 2020 that may be closed by this pull request

Hook only renamed when it fails, not when it begins and ends successfully #4348

Closed

4 tasks

Omrisnyk mentioned this pull request Dec 7, 2023

[Snyk] Fix for 1 vulnerabilities Omrisnyk/npm-lockfiles#131

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #4348 #4383

Fix #4348 #4383

cspotcode commented Jul 26, 2020

coveralls commented Jul 27, 2020 •

edited

Loading

cspotcode commented Jul 27, 2020

juergba left a comment

juergba Jul 28, 2020

cspotcode Jul 28, 2020

cspotcode commented Jul 28, 2020

juergba commented Jul 28, 2020

cspotcode commented Jul 28, 2020 via email •

edited

Loading

boneskull left a comment

boneskull Jul 28, 2020

boneskull Jul 28, 2020

cspotcode Jul 28, 2020 •

edited

Loading

boneskull Jul 28, 2020

cspotcode Jul 28, 2020

boneskull left a comment

cspotcode commented Jul 28, 2020

Fix #4348 #4383

Fix #4348 #4383

Conversation

cspotcode commented Jul 26, 2020

Requirements

Description of the Change

Alternate Designs

Why should this be in core?

Benefits

Possible Drawbacks

Applicable issues

coveralls commented Jul 27, 2020 • edited Loading

cspotcode commented Jul 27, 2020

juergba left a comment

Choose a reason for hiding this comment

juergba Jul 28, 2020

Choose a reason for hiding this comment

cspotcode Jul 28, 2020

Choose a reason for hiding this comment

cspotcode commented Jul 28, 2020

juergba commented Jul 28, 2020

cspotcode commented Jul 28, 2020 via email • edited Loading

boneskull left a comment

Choose a reason for hiding this comment

boneskull Jul 28, 2020

Choose a reason for hiding this comment

boneskull Jul 28, 2020

Choose a reason for hiding this comment

cspotcode Jul 28, 2020 • edited Loading

Choose a reason for hiding this comment

boneskull Jul 28, 2020

Choose a reason for hiding this comment

cspotcode Jul 28, 2020

Choose a reason for hiding this comment

boneskull left a comment

Choose a reason for hiding this comment

cspotcode commented Jul 28, 2020

coveralls commented Jul 27, 2020 •

edited

Loading

cspotcode commented Jul 28, 2020 via email •

edited

Loading

cspotcode Jul 28, 2020 •

edited

Loading