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

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

Merged
merged 19 commits into from May 12, 2020

Conversation

arvidOtt
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

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

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.

Looks good, thanks!

Just a couple comments.

I think this is a bugfix.

lib/interfaces/common.js Outdated Show resolved Hide resolved
test/integration/options/forbidOnly.spec.js Outdated Show resolved Hide resolved
@boneskull 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 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
@coveralls
Copy link

coveralls commented Apr 28, 2020

Coverage Status

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

@boneskull boneskull requested a review from juergba April 28, 2020 20:40
@boneskull
Copy link
Member

This LGTM, but think @juergba should take a look

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.

@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
Copy link
Contributor Author

arvidOtt commented May 4, 2020

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
Copy link
Contributor Author

arvidOtt commented May 4, 2020

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
Copy link
Member

juergba commented May 5, 2020

@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
Copy link
Member

boneskull commented May 5, 2020

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

@arvidOtt
Copy link
Contributor Author

arvidOtt commented May 5, 2020

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

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!

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.

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 Outdated Show resolved Hide resolved
@arvidOtt
Copy link
Contributor Author

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

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.

ty!

@boneskull boneskull merged commit 184036f into mochajs:master May 12, 2020
@craigtaub craigtaub added this to the next milestone May 21, 2020
craigtaub pushed a commit that referenced this pull request May 21, 2020
…; 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.
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.

None yet

5 participants