Bug 3840: --forbid-only doesn't recognize `it.only` when `before` crashes #4256

arvidOtt · 2020-04-28T16:20:31Z

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

Option --forbid-only crashes only in case of approaching describe.only but not it.only. Following the discussion in #3840 and the closed PR #3948 it was decided that this should be prevented at definition phase and not during runtime.

This change checks if the --forbid-only option is set prior to marking a test to be only. This is the same point at which the check is done for the suite. The error message reported will be the same as for the suite. This change also adds integration test cases to cover the situation.

Alternate Designs

Do the check at runtime like in PR #3948. This was explicitly discussed and not wanted.

Why should this be in core?

It fixes a bug.

Benefits

It fixes a bug.

Possible Drawbacks

Applicable issues

#3840

…hrow error

boneskull

Looks good, thanks!

Just a couple comments.

I think this is a bugfix.

lib/interfaces/common.js

test/integration/options/forbidOnly.spec.js

… done before runtime

coveralls · 2020-04-28T17:27:01Z

Coverage increased (+0.2%) to 93.21% when pulling 3d21897 on arvidOtt:issue/3840 into e7add63 on mochajs:master.

boneskull · 2020-04-28T20:40:51Z

This LGTM, but think @juergba should take a look

juergba

@arvidOtt Thank you. I'm not sure wether the priorities are consistent.

when an only-suite is not matched by grep, it does not throw?
when an only-test is not matched by grep, it throws?
when it does not throw when not matched by grep, then why should it throw when statically skipped?

What do you think? Do our docs tell anything about priorities?

arvidOtt · 2020-05-04T16:28:28Z

Thank you @juergba, I agree that this is handled inconsistently.

In the CLI Usage it is described:

--forbid-only

Enforce a rule that tests may not be exclusive (use of e.g., describe.only() or it.only() is disallowed).

--forbid-only causes Mocha to fail when an exclusive ("only'd") test or suite is encountered, and it will abort further test execution.

as well as

--forbid-only Fail if exclusive test(s) encountered [boolean]

I found nothing in the docs about priorities between only, grep or skip.

arvidOtt · 2020-05-04T16:31:16Z

Considering the usage of .only() and therefore --forbid-only (prevent people from accidentally forgetting .only() statements after debugging a test suite or case) as well as your comment I would suggest that no matter if there is a grep or a skip if the --forbid-only option is set an error will be thrown during parsing phase. So the rule would be simple, consistent and suites as well as test cases would be handled the same way.

What is your opinion on that @juergba @boneskull?

juergba · 2020-05-05T08:12:13Z

@outsideris you implemented shouldBeTested(), your opinion please?

[...] no matter if there is a grep or a skip if the --forbid-only option is set an error will be thrown during parsing phase [...]

I could live with your proposition.
But then the same logic could be applied to pending as well. Which here only fails on suites, but not on tests ... Pending tests fail at runtime within the runner, which makes some sense, since we have static and conditional skipping.

We have to decide:

do we need shouldBeTested() for --fobid-only? do we need it for --forbid-pending?
Can we remove shouldBeTested() ==> @outsideris ?
pending tests: have to be checked at runtime. Do we add additional check here at parse time?

Easy solution: we leave the pending stuff as is, and don't think or ask too much. And we remove the grep check for only-suites.

boneskull · 2020-05-05T18:41:02Z

I agree that --forbid-only should be triggered even if --grep or --fgrep causes the suite/test to be omitted.

…elected by grep

… is not selected by grep

arvidOtt · 2020-05-05T21:55:25Z

@juergba @outsideris @boneskull I applied the following changes:

Change --forbid-only logic to be consistent for tests and suites throwing error even if omitted by grep or f-grep
Refactor suite class improving encapsulation by adding markOnly() method. (Implemented similar for tests in 🛠️ Repo: Refactor to fix encapsulation issues #3689 )

Concerning the handling of pending I would suggest to open a new PR if required as this one is primarily aiming to fix the bug #3840 , however, I would be happy to support on that one as well!

boneskull

ty for updating. I'm sorry, I think I changed my mind. Yes, I do think --forbid-only should throw an error even if --grep is used. But I don't think it should be in this PR. Possible to revert that and submit another?

I'll merge this once it's pulled out. If you don't have the cycles, I can do it myself.

thanks!!

lib/suite.js

This reverts commit cffc71a.

…ly suite is not selected by grep" This reverts commit 336425a.

…is not selected by grep" This reverts commit f871782.

This reverts commit c2c8dc8.

This reverts commit 4b37e3c.

arvidOtt · 2020-05-12T07:51:34Z

@boneskull I reverted the commits concerning the refactoring of the suites handling of --grep, --fgrep and pushed them to a new PR #4282.

boneskull

ty!

…; closes #3840 * add fixtures that result in it.only combined with --forbid-only bug * adapt forbidonly test cases to cover it.only bug * check if forbid only option is set prior to marking a test only and throw error * change name of only test back to previous * use custom assertion for expecting error in forbidOnly tests * remove empty line * use createUnsupportedError instead of throw new Error * use createUnsupportedError instead of throw new Error * remove check if suite hasOnly and forbidOnly option is set as this is done before runtime * implement markOnly instance method in suite class * add unit test for suites markOnly method * throw exception if --forbid-only option is set even if suite is not selected by grep * adapt forbidOnly integration tests to check for failure if only suite is not selected by grep * fix jsdocs of suite markonly * Revert "fix jsdocs of suite markonly" This reverts commit cffc71a. * Revert "adapt forbidOnly integration tests to check for failure if only suite is not selected by grep" This reverts commit 336425a. * Revert "throw exception if --forbid-only option is set even if suite is not selected by grep" This reverts commit f871782. * Revert "add unit test for suites markOnly method" This reverts commit c2c8dc8. * Revert "implement markOnly instance method in suite class" This reverts commit 4b37e3c.

arvidOtt added 4 commits April 28, 2020 17:52

add fixtures that result in it.only combined with --forbid-only bug

c0c1dea

adapt forbidonly test cases to cover it.only bug

63d6144

check if forbid only option is set prior to marking a test only and t…

2f0f134

…hrow error

change name of only test back to previous

3c8af1b

boneskull suggested changes Apr 28, 2020

View reviewed changes

lib/interfaces/common.js Outdated Show resolved Hide resolved

test/integration/options/forbidOnly.spec.js Outdated Show resolved Hide resolved

boneskull added type: bug a defect, confirmed by a maintainer semver-patch implementation requires increase of "patch" version number; "bug fixes" labels Apr 28, 2020

arvidOtt changed the title ~~Issue 3840: --forbid-only doesn't recognize it.only when before crashes~~ Bug 3840: --forbid-only doesn't recognize it.only when before crashes Apr 28, 2020

arvidOtt added 5 commits April 28, 2020 18:58

use custom assertion for expecting error in forbidOnly tests

7d6110a

remove empty line

39e89fe

use createUnsupportedError instead of throw new Error

11d5fe1

use createUnsupportedError instead of throw new Error

7015048

remove check if suite hasOnly and forbidOnly option is set as this is…

956cef2

… done before runtime

boneskull approved these changes Apr 28, 2020

View reviewed changes

boneskull requested a review from juergba April 28, 2020 20:40

juergba reviewed May 4, 2020

View reviewed changes

arvidOtt added 4 commits May 5, 2020 23:42

implement markOnly instance method in suite class

4b37e3c

add unit test for suites markOnly method

c2c8dc8

throw exception if --forbid-only option is set even if suite is not s…

f871782

…elected by grep

adapt forbidOnly integration tests to check for failure if only suite…

336425a

… is not selected by grep

fix jsdocs of suite markonly

cffc71a

boneskull suggested changes May 12, 2020

View reviewed changes

lib/suite.js Outdated Show resolved Hide resolved

Revert "fix jsdocs of suite markonly"

419f584

This reverts commit cffc71a.

arvidOtt added 4 commits May 12, 2020 08:13

Revert "adapt forbidOnly integration tests to check for failure if on…

189b5cf

…ly suite is not selected by grep" This reverts commit 336425a.

Revert "throw exception if --forbid-only option is set even if suite …

053867f

…is not selected by grep" This reverts commit f871782.

Revert "add unit test for suites markOnly method"

0834199

This reverts commit c2c8dc8.

Revert "implement markOnly instance method in suite class"

3d21897

This reverts commit 4b37e3c.

arvidOtt mentioned this pull request May 12, 2020

Consistent handling of --forbid-only for suites and tests #4282

Merged

boneskull approved these changes May 12, 2020

View reviewed changes

boneskull merged commit 184036f into mochajs:master May 12, 2020

craigtaub added this to the next milestone May 21, 2020

Omrisnyk mentioned this pull request Dec 7, 2023

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

Open

tpicard586 mentioned this pull request Jul 21, 2024

[Snyk] Upgrade mocha from 7.1.1 to 7.2.0 tpicard586/damn-vulnerable-defi#3

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 3840: --forbid-only doesn't recognize `it.only` when `before` crashes #4256

Bug 3840: --forbid-only doesn't recognize `it.only` when `before` crashes #4256

arvidOtt commented Apr 28, 2020

boneskull left a comment

coveralls commented Apr 28, 2020 •

edited

Loading

boneskull commented Apr 28, 2020

juergba left a comment

arvidOtt commented May 4, 2020 •

edited

Loading

arvidOtt commented May 4, 2020 •

edited

Loading

juergba commented May 5, 2020

boneskull commented May 5, 2020 •

edited

Loading

arvidOtt commented May 5, 2020 •

edited

Loading

boneskull left a comment

arvidOtt commented May 12, 2020

boneskull left a comment

Bug 3840: --forbid-only doesn't recognize it.only when before crashes #4256

Bug 3840: --forbid-only doesn't recognize it.only when before crashes #4256

Conversation

arvidOtt commented Apr 28, 2020

Requirements

Description of the Change

Alternate Designs

Why should this be in core?

Benefits

Possible Drawbacks

Applicable issues

boneskull left a comment

Choose a reason for hiding this comment

coveralls commented Apr 28, 2020 • edited Loading

boneskull commented Apr 28, 2020

juergba left a comment

Choose a reason for hiding this comment

arvidOtt commented May 4, 2020 • edited Loading

arvidOtt commented May 4, 2020 • edited Loading

juergba commented May 5, 2020

boneskull commented May 5, 2020 • edited Loading

arvidOtt commented May 5, 2020 • edited Loading

boneskull left a comment

Choose a reason for hiding this comment

arvidOtt commented May 12, 2020

boneskull left a comment

Choose a reason for hiding this comment

Bug 3840: --forbid-only doesn't recognize `it.only` when `before` crashes #4256

Bug 3840: --forbid-only doesn't recognize `it.only` when `before` crashes #4256

coveralls commented Apr 28, 2020 •

edited

Loading

arvidOtt commented May 4, 2020 •

edited

Loading

arvidOtt commented May 4, 2020 •

edited

Loading

boneskull commented May 5, 2020 •

edited

Loading

arvidOtt commented May 5, 2020 •

edited

Loading