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

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

Merged
merged 7 commits into from Jun 5, 2020

Conversation

arvidOtt
Copy link
Contributor

Description of the Change

The handling of the --forbid-only option is currently inconsistent for theTest and Suite classes. Suites that are marked as only and excluded by --grep or --fgrep don't throw an error. Test that are marked as only but excluded by grep or f-grep do throw an error.

Following the discussion in #4256 it was suggested that due to the nature of the --forbid-only option it should not matter if a suite or test is excluded by a grep. An error should be thrown either way to protect the user from forgetting to remove .only() statements from his test code. I moved the changes concerning this inconsistency, including a refactoring of the Suite class to use a markOnly() instance method, into this PR to separate them from the fix #4256 for bug #3840.

Alternate Designs

An alternative to make it consistent would be to adapt the logic so it will be checked if test cases are excluded by grep or fgrep as well. (see discussion in #4256)

Why should this be in core?

It improves the consistency of the --forbid-only option and improves encapsulation between classes.

Benefits

It improves the consistency of the --forbid-only option and improves encapsulation between classes.

Possible Drawbacks

If the --forbid-only option is set and a test or suite is marked only but excluded by grep it will now throw an error.

Applicable issues

#4256
#3840

@arvidOtt arvidOtt changed the title Consistent handling of --forbid-only in suites and tests Consistent handling of --forbid-only for suites and tests May 12, 2020
@coveralls
Copy link

coveralls commented May 12, 2020

Coverage Status

Coverage increased (+0.006%) to 93.571% when pulling 5c58d0a on arvidOtt:forbid-only-grep into 63eb80b on mochajs:master.

@boneskull
Copy link
Member

@arvidOtt looks like this now conflicts with master

@boneskull
Copy link
Member

@mochajs/core This is one of those things that, I think, makes sense. However, it's a) a breaking change, and b) maybe not worth breaking. Opinions?

@boneskull boneskull added semver-major implementation requires increase of "major" version number; "breaking changes" area: usability concerning user experience or interface labels May 12, 2020
@boneskull
Copy link
Member

Before we merge this--or any breaking change, really--we should answer these questions:

  1. Who does this potentially help?
  2. Who does this potentially harm?
  3. For those who it harms, are there any workarounds, and what are they?

I'll try to answer these:

This change ensures that, when used with --grep/--fgrep, --forbid-only will cause a failure, even if the test in question is to be skipped due to not matching the grep clause. Users who are using --forbid-only with --grep (of which there are an unknown number) may find that --forbid-only now catches a usage of only that it did not previously catch, causing tests to exit with an error.

  1. This potentially helps those who expect any usage of only in the entire list of files to exit with an error. The current behavior results in a sort of "false positive", since it won't catch the usage if the tests are skipped using --grep. Upon upgrade, these users will need to remove only.
  2. This potentially harms those who expect a usage of only in the list of tests to be run to exit with an error. The current behavior, then, is the expected behavior for this group of users.
  3. For those who this negatively impacts, there is no complete workaround. Restricting the list of files to a subset may help. Using a ESLint plugin to detect these statements can also be an effective solution. Reserving --forbid-only if process.env.CI is set may also be a better way to consume the option.

Generally, I don't see a lot of usage of --grep outside of ad-hoc Mocha runs, but these usages do exist. However--and this is the reason for --forbid-only--it's highly unlikely that using only in e.g., a CI build is intended; it is usually a mistake.

Since the intent of --forbid-only is to avoid false positives in CI, I believe this PR gets us closer to that goal, regardless of any potential difficulty this change may introduce to a (likely small) subset of users.

@boneskull
Copy link
Member

@arvidOtt FWIW this needs conflict resolution again, but you may want to just wait until someone else reviews it. Or, if it's approved by others, I can resolve the conflicts myself and merge.

@arvidOtt
Copy link
Contributor Author

Maybe one thing to highlight here is that the inconsistency in the behaviour of suites and tests also might "harm" users by unexpected behaviour.

@arvidOtt
Copy link
Contributor Author

arvidOtt commented May 29, 2020

@juergba as you brought up this point in your comment in #3840, what is your opinion on this PR aiming to solve that problem?

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.

Code change looks good. I've got a small remark about reusing a sandbox.

@boneskull I agree with you that the intent of --forbid-only is to avoid false positives. I don't believe a lot of people are using it in any other way. In conclusion, I think releasing this in our next major version is a good idea.

If users are using --forbid-only in conjunction with --grep, they should be able to work around it by making multiple config's for example. We should clearly state so in the changelog, so users know what to do.

test/unit/suite.spec.js Outdated Show resolved Hide resolved
@arvidOtt arvidOtt requested a review from nicojs June 2, 2020 11:34
@boneskull
Copy link
Member

If users are using --forbid-only in conjunction with --grep, they should be able to work around it by making multiple config's for example. We should clearly state so in the changelog, so users know what to do.

@nicojs Can you explain further?

@boneskull
Copy link
Member

@arvidOtt I've rebased to fix the conflict; please look at the result. In another PR, I had created a createForbiddenExclusivityError function, which should be called instead of createUnsupportedError.

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.

I agree with this change.
We throw during the parsing phase - similar to linting - and grep grips later during runtime. Parsing comes first and therefore has higher priority.

lib/interfaces/common.js Outdated Show resolved Hide resolved
@arvidOtt
Copy link
Contributor Author

arvidOtt commented Jun 3, 2020

@boneskull that makes sense, thanks!

@juergba @nicojs I integrated your feedback 👷

@arvidOtt arvidOtt requested a review from juergba June 3, 2020 08:34
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

@boneskull
Copy link
Member

Wanted to understand what @nicojs is recommending here insofar as documentation

@nicojs
Copy link
Contributor

nicojs commented Jun 4, 2020

Sure, I'll write down my thoughts tomorrow.

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.

Sorry to come back to the sandbox, but the way it is now I see a problem with it. Sorry if I didn't comunicate myself clearly the first time.

test/unit/suite.spec.js Outdated Show resolved Hide resolved
@nicojs
Copy link
Contributor

nicojs commented Jun 5, 2020

Maybe something like this.

BREAKING CHANGE

Focussed suites (i.e. describe.only) are never allowed in conjunction with --forbid-only, even if it's filtered out with --grep. This behavior was already the case with focussed tests (i.e. it.only), but not for suites.

This is no longer OK to do:

// foobar.spec.js
describe.only('foo', () => {
});

describe('bar', () => {
});
npx mocha --forbid-only --grep bar *.spec.js

Work around

If you want to keep using this setup, you can work around this issue by moving focussed suites to different files and configure mocha to exclude them in your CI.

// only-foo.spec.js
describe.only('foo', () => {
});

// bar.spec.js
describe('bar', () => {
});
npx mocha --forbid-only --grep bar --exclude only-foo.spec.js *.spec.js

The same configuration could also be achieved with 2 configuration files.

// .mocharc.json
{
  "spec": "*.spec.js",
  "exclude": "only-foo.spec.js",
  "grep": "bar",
  "forbid-only": true
}

// .mocharc.only.js
const config = require('./.mocharc.json');
config['forbid-only'] = false;
config.spec = ['**.spec.js'];
module.exports = config;
npx mocha # normal run, without foo
npx mocha --config .mocharc.only.js # run with focussed foo

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.

Awesome! Thanks for the fast response. For me, this can land now 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: usability concerning user experience or interface semver-major implementation requires increase of "major" version number; "breaking changes"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants