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

Add and configure eslint-plugin-mocha #521

Merged
merged 6 commits into from
Sep 29, 2018
Merged

Conversation

papandreou
Copy link
Member

The main use case for me is to prohibit it.only from being pushed accidentally.

I enabled the rules that seemed most useful and hopefully not too controversial, but there are more: https://github.com/lo1tuma/eslint-plugin-mocha/blob/master/docs/rules/README.md

@sunesimonsen
Copy link
Member

Why the arrow function hate 😊

@papandreou
Copy link
Member Author

Arrow functions are bad in mocha tests because they prevent you from accessing the test context via this. More background here: mochajs/mocha#2018

@papandreou
Copy link
Member Author

(however, since we also want the test suite to run in jest, which doesn't have the concept of a test context, we don't really rely on it anywhere).

@sunesimonsen
Copy link
Member

I don't think we want to use this in the tests are you mention.

Copy link
Member

@sunesimonsen sunesimonsen left a comment

Choose a reason for hiding this comment

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

I would like to remove the arrow functions constraint. But if you want to keep it I'm also okay with that.

@papandreou
Copy link
Member Author

I've wired up my brain so arrow functions with it and describe smell bad, because it rubs mocha the wrong way, but I could make my peace with that.

Inconsistency bothers me even more though -- so if we decide against including that rule, I think we should switch to arrow functions everywhere: #522

papandreou added a commit that referenced this pull request Sep 29, 2018
@papandreou papandreou force-pushed the feature/eslint-plugin-mocha branch from f8f9575 to f08e726 Compare September 29, 2018 11:28
@alexjeffburke
Copy link
Member

I'm sort of torn - I actually had no idea that there was a functionality penalty using arrow functions with mocha. If I may ask, what is the use case for the mocha context API?

I've always just been consistent with what each project currently does. In $work trees we do arrow functions for it/describe and I'd be surprised if many people reach for the feature.

In summary I defer to those with stronger feelings - I'm perfectly happy to keep the constraint and relax it later since we already uphold it or convert wholesale.

@papandreou
Copy link
Member Author

papandreou commented Sep 29, 2018

@alexjeffburke, the mocha test context can be use for building up state in beforeEach blocks, similar to using variables lexically scoped to eg. a describe block. That part is mostly a matter of taste.

Another use case is to install a test suite-wide feature or "plugin", for example something like https://www.npmjs.com/package/mocha-sinon, without adding a beforeEach that creates a new sinon sandbox to each test file. To some extent that's analogous to putting the sandbox in a global variable, so again a matter of taste and aesthetics.

... All of this doesn't matter so much anymore, though, as I already reverted the parts that prohibited arrow functions in describe and it. I've opened another PR so we can consistently use arrow functions instead. We aren't likely to start using the mocha context because jest doesn't support it.

Seems like everyone is on board with the other parts, so I'll merge :)

@papandreou papandreou merged commit a319eaf into master Sep 29, 2018
@alexjeffburke alexjeffburke deleted the feature/eslint-plugin-mocha branch December 15, 2019 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants