Default rules enabled / extends

[rugk]

I have not much knowledge of EsLint, but it is annoying to go through all rules and find out, which to use.

Could not you – just like plain EsLint – somehow also use a thing/things like "extends": "eslint:recommended", so one can get a set of rules that are almost always good for mocha tests?

I think there are many rules that should be enabled "by default"/"by extends".

[lo1tuma]

Although this is not documented a recommended config already exist. When you have this plugin installed you should be able to use it via "extends": "plugin:mocha/recommended".

[rugk]

Okay, then it needs to be documented. And it also needs to be documented, which rules are covered by that extend.

[jzaefferer]

Looks like the preset only enables a single rule: https://github.com/lo1tuma/eslint-plugin-mocha/blob/master/index.js#L28

Is that correct? If so, what's the reason?

It seems like this could cover most of the provided rules instead.

[basickarl]

I find it weird also that the recommended refers to one ruleset.

[Lakitna]

I would very much like a proper recommended ruleset. So let me kick off the discussion.

How about the following

// Will always result in a failed test, throw error to prevent this
'mocha/handle-done-callback': 'error',

// For low level tests it makes a lot of sense to enforce a single top level suite. For higher
// level tests you'll sometimes want more that one top level suite, but not often. Enforcing
// at 1 seems best for recommended
'mocha/max-top-level-suites': ['error', {limit: 1}],

// Exclusive tests (`.only`) in CI should be caught with `--forbid-only`. While working on
// tests a warning will suffice.
'mocha/no-exclusive-tests': 'warn',

// A test is always run in context, not adding a suite is a bad practice on the maintenance
// side of things.
'mocha/no-global-tests': 'error',

// Hooks (`.before` etc) are a very powerful part of Mocha and should not be disallowed.
// Disallowing this is very opinionated. 
'mocha/no-hooks': 'off',

// This can be a sign of bad test design. It can also be a valid separation of concerns imo.
// Am I opinionated?
'mocha/no-hooks-for-single-case': 'warn',

// You probably made a copypasta oopsy
'mocha/no-identical-title': 'error',

// `it(() => {})` should never be used because you loose `this`. A common trap that can be
// prevented with this rule
'mocha/no-mocha-arrows': 'error',

// Nested tests don't behave like you expect. You probably wanted to make a suite
'mocha/no-nested-tests': 'error',

// `it('is pending');`
// Not sure about this one. Might have to make this an error?
'mocha/no-pending-tests': 'warn',

// According to the docs, it causes a failure in Mocha. Prevent that with this rule
'mocha/no-return-and-callback': 'error',

// You should do this kind of thing in a hook.
'mocha/no-setup-in-describe': 'error',

// Generally confusing and unnecessary. The few times where you do want this you can
// skip the rule with an annotation. 
'mocha/no-sibling-hooks': 'error',

// Skipping tests can be a bad thing you should not forget about. I'm not sure about this
// one but I'd say you should catch this with `--forbid-pending` when you want to fail on
// this.
'mocha/no-skipped-tests': 'warn',

// Opinionated
'mocha/no-synchronous-tests': 'off',

// Hooks should always be part of a suite
'mocha/no-top-level-hooks': 'error',

// Opinionated
'mocha/prefer-arrow-callback': 'off',

// Opinionated and dependent on the way the project chooses to structure things
'mocha/valid-suite-description': 'off',

// Opinionated and dependent on the way the project chooses to structure things
'mocha/valid-test-description': 'off',

// Results in unexpected behaviour
'mocha/no-async-describe': 'error',

[lo1tuma]

@Lakitna Thanks for the proposal, I pretty much agree with your reasonings. The only thing I’m not sure about is enforcing mocha/no-top-level-hooks. I often see top-level hooks in integration or e2e test suites, where a "setup.js" file is loaded by mocha and a global beforeEach does something like connecting to a database or configuring selenium. On the other hand I also think there are better ways to do those kind of setups.

[Lakitna]

It's true that Mocha has no problems when you use this pattern as the hooks will be added to the root suite (the everything root). At the same time, I think it's a weird pattern as every step should explicitly be a part of a suite. It feels like the ones using this pattern are often indecisive and a rule like this can trigger them to be more aware of what they're doing.

There is also the question of what kinds of tests we want to target here. I think it should be an error in unit tests but, as @lo1tuma points out, it might be ok in e2e.

Maybe even create multiple rulesets? mocha/unit, mocha/integration and mocha/e2e?

Also, one thing I was thinking about and want to note, but can't test right now, is how dynamic tests are treated by these rules. Think something like

describe('Dynamic tests', function() {
  ['a', 'b', 'c'].forEach((testCase) => {
    it(`is test case ${testCase}`);
  });
});

['a', 'b', 'c'].forEach((testCase) => {
  describe(`Dynamic tests for ${testCase}`, function() {
    it(`is a test`);
  });
});

This is officially supported and can be very useful in high-level tests

[lo1tuma]

Personally I’ve stopped using hooks. Even in integration tests, there are almost always cases where a test case don’t require a database connection. I just use setup functions and dependency injection, for example:

it('should work', async function () {
    return withDatabase(async (database) => {
        const myModel = new Model(database);
        assert.deepStrictEqual(await myModel.getData(), { the: 'data' });
    });
});

This makes it also easy to understand what is going on. If there would be a global hook in a different file it would be not obvious for the reader to notice that.

So I would say let’s start with a strict ruleset and see how many users complain, we can always adjust the ruleset in future versions.

Maybe even create multiple rulesets? mocha/unit, mocha/integration and mocha/e2e?

Yes, that might be a viable option for the future. But let’s wait and see if there is an actual need for it.

Also, one thing I was thinking about and want to note, but can't test right now, is how dynamic tests are treated by these rules

That depends on the rule. For example the no-global-tests rule documents this kind of problem and wouldn’t detect anything.

[Lakitna]

Personally I’ve stopped using hooks.

Did you stop with all hooks or are you still using them in nested describes?

I don't think we should enforce that, I'm just curious :)

This makes it also easy to understand what is going on. If there would be a global hook in a different file it would be not obvious for the reader to notice that.

I agree. Therefore I think we should promote good practice and make no-top-level-hooks an error, even if in some cases it does make sense. At least it will force the user to think about this. The docs for the rule also talk about the confusion you're talking about. When issues are opened because people don't understand, adding a concrete example to the docs should do the trick of what you're describing.

For dynamic tests, I'm mainly concerned about no-setup-in-describe. For example in the following situation.

describe('top level suite', function() {
    ['a', 'b', 'c'].forEach((testCase) => {
        describe(`Dynamic tests for ${testCase}`, function() {
            it(`is a test`);
        });
    });
});

I can test these kinds of thing when I get home.

[lo1tuma]

Did you stop with all hooks or are you still using them in nested describes?

I completely stopped using any kind of hooks. I just use plain functions instead.

Therefore I think we should promote good practice and make no-top-level-hooks an error,

Agreed 👍

For dynamic tests, I'm mainly concerned about no-setup-in-describe

Good point, actually the documentation already points out that this rule should be disabled if you use dynamically generated tests. So I think until one comes up with a good idea how to improve the rule in that regard we shouldn’t enable it in the recommend ruleset.

[Lakitna]

So I think until one comes up with a good idea how to improve the rule in that regard we shouldn’t enable it in the recommend ruleset.

How about we make it a warning. That way the user will know that it is a questionable thing to do but can ignore it when using dynamic tests.

[lo1tuma]

That sounds reasonable.