lo1tuma/eslint-plugin-mocha

ESLint rules for mocha

eslint-plugin-mocha's Issues

handle-done-callback: Add checks how the callback is handled specifically (e.g. for simple to detect erroneous calls)

E.g. return done; should cause a warning while return otherImportantFunctionThatHandlesDone(done); should not.

Rule proposal: no-nested-describes

Some consider having nested describes a bad practice, and prefer splitting tests into several files. See avajs/ava#222 for a discussion for the introduction of nesting in AVA (which is currently not accepted, and probably never will).

I'm proposing this rule for people such as myself that like this train of thought but use Mocha in their projects.

Examples

Invalid

describe('foo', function() {
  describe('bar', function() {
    // ...
  });
});

Valid

describe('foo', function() {
  // ...
});

describe('bar', function() { // or even split it into several files
  // ...
});

Now that I think about it, another way to do this could be to only allow one describe per file, which would make the valid example invalid. This would probably be better as a different but complimentary rule (either allow one describe per file, or one describe per "level").

Rule proposal: Require a test to have an assertion

If a test has no assertions, it looks like it passed, when in fact nothing happened. It would be nice to have a rule that catches when a test does not have an assertion.

Bad

it('adds two numbers', () => {
   const numerator = 4;
   const denominator = 2;
   // oops, no assertion!
});

Good

it('adds two numbers', () => {
   const numerator = 4;
   const denominator = 2;
   expect(calc.divide(numerator, denominator)).to.equal(2);
});

By default, it could check for an identifier from one of the assertion libraries (assert, expect, should). Or a list of identifiers could be provided as an option.

eslint 3.0.0 upgrade

eslint has been upgraded to 3.0.0, throwing an UNMET PEER DEPENDENCY error when installing. Probably just a chore upgrade dependency, but might be worth a look at their breaking changes.

Rule proposal: consistent order of tests

The spec reporter from Mocha always outputs the test results of the current describe block before the results of subsequent describe blocks.

I would like to have a rule to reflect the same order in the code:

• describe
  • before / after hooks
  • it
  • it
  • describe

This would be an invalid order:

• describe
  • before / after hooks
  • describe
  • it

Maybe we should consider a separate rule that requires the position of before /after hooks on top of a describe block.

handle-done-callback: branch-tracking

Add branch-tracking to make sure that the done callback is called in every possible branch

Why `no-synchronous-tests` running inside of `before` and `after`?

Why is running done inside of after necessary? I dont think it is.

rule to enforce returning promise in async tests instead of done

don't know if it is possible to enforce usage of promise returns, probably not.
at least it should be possible to check that done argument is not used and a return something is used instead

Rule proposal: forbid top level tests

Tests should be always inside of a describe block.

Release 4.8.0?

4.7.0 has been out for a while, and contains nice things like autofix for no-mocha-arrows. I thought we could release 4.8.0 now.

I can (probably) do it myself, but I was wandering whether there was something missing before releasing.

Implement auto fix for no-exclusive-tests

Modularise core

I maintain eslint-plugin-jasmine and noticed some crossover between eslint-plugin-mocha and eslint-plugin-jasmine.

Jasmine and Mocha share the same general describe/it syntax and in cases where they differ, for example in exclusive (Mocha) or focused (Jasmine) tests, similar if not the same basic matchers apply.

Perhaps we could extract "core" functionality where it makes sense, a la standard/semistandard?

Export a recommended plugin configuration

Please export a recommended plugin configuration similar to how other plugins do that:

• https://github.com/alecxe/eslint-plugin-protractor/blob/master/index.js#L32
• https://github.com/tlvince/eslint-plugin-jasmine/blob/master/index.js#L16

Additionally, mention it in the documentation (sample).

Thank you!

Rule proposal: enforce it() description to start with "should"

Would it make sense to enforce having descriptive test names by warning if it() description does not start with "should". Example of violation:

it("I want to test something", function () {
  // ...
});

Valid usage:

it("should test something", function () {
  // ...
});

Thanks.

no-exclusive-tests: additionalTestFunctions should support methods

I'm using this plugin with selenium-webdriver tests that uses wrapped mocha functions as methods of test object:

test.describe('Google Search', function() {
  var driver;

  test.before(function() {
    driver = new webdriver.Builder()
        .forBrowser('firefox')
        .build();
  });

  test.it('should append query to title', function() {
    driver.get('http://www.google.com');
    driver.findElement(By.name('q')).sendKeys('webdriver');
    driver.findElement(By.name('btnG')).click();
    driver.wait(until.titleIs('webdriver - Google Search'), 1000);
  });

  test.after(function() {
    driver.quit();
  });
});

Full example from selenium repo: https://github.com/SeleniumHQ/selenium/blob/master/javascript/node/selenium-webdriver/example/google_search_test.js

Setting additionalTestFunctions does not work:

  "settings": {
    "mocha/additionalTestFunctions": [
      "test.describe",
      "test.it"
    ]
  }

Is it possible to support this?
Thanks!

Error: Definition for rule 'mocha/valid-test-description' was not found

When I set rules to the following

"rules": {
    "mocha/valid-test-description": "error"
}

i get the error error: Definition for rule 'mocha/valid-test-description' was not found.

eslint: 3.0.0
eslint-plugin-mocha: 3.0.0

'no-sibling-hooks': false-positives inside functions

I have a rough example below:

describe('a thing', () => {

  before(() => {
    // do some global setup
  });

  function runSomeTests() {
    before(() => { // <-- Incorrectly triggers 'mocha/no-sibling-hooks'
      // stuff
    });
    it('should test a different thing', () => {
      // test
    });
  }

  function runSomeOtherTests() {
    before(() => { // <-- Incorrectly triggers 'mocha/no-sibling-hooks'
      // stuff
    });
    it('should test a thing', () => {
      // test
    });
  }

  context('one thing', () => {
    before(() => {
      // more setup
    });
    context('nested thing', () => {
      runSomeTests();
    });
    context('other nested thing', () => {
      runSomeOtherTests();
    });
  });

  context('another thing', () => {
    before(() => {
      // different setup
    });
    context('nested thing', () => {
      runSomeTests();
    });
    context('other nested thing', () => {
      runSomeOtherTests();
    });
  });
});

Rule proposal: Forbid tests with both a callback and a return statement

In Mocha v3, it is not allowed to return a Promise in tests while the test function takes a done callback function.

It would be nice to detect this, so that you have a tighter feedback loop when coding, instead of waiting for your test to run. It could also help with the migration to v3 (I did this yesterday in a relatively big project, and it was a bit time-consuming).

I'm not sure whether we should forbid (top-level) return statements altogether, or only when we can detect that a Promise is returned (with a .then call). I think the former would be fine, as there is little value in returning something in a test when you have a callback, and it would cover a lot more cases where we can't know for sure that a Promise is being returned.

Incorrect

it('should XYZ', function(done) {
  return foo()
    .then(bar)
    .nodeify(done);
});

Correct

it('should XYZ', function(done) {
  foo()
    .then(bar)
    .nodeify(done);
});

it('should XYZ', function() {
  return foo()
    .then(bar);
});

As for the rule name, I'm not too sure yet. no-return-and-callback? Or if we want to make sure the returned statement is a Promise, no-promise-and-callback?

Rule proposal: add "valid-suite-description" rule

Similar to the existing valid-test-description rule, but to match against the suite descriptions (describe or context or suite). Thanks.

is there a way to turn off auto fixing with --fix for no-skipped-tests

While trying auto fix we realised that mocha is converting all "xit", "it.skip" ... into "it" which is adding the test into scope. Ideal case for us would be if it just raises warning or shows error.

Is there any way we can turn off the autofix for specific rule ?

Don't autofix no-exclusive-tests

With autofix eslint is becoming a great tool to fix stylistic linting issues in code. I love having --fix on save in Atom's linter-eslint.

While I love no-exclusive-tests to make sure I don't commit .only (pre-commit hook). Having autofix removing .only on save is obviously undesired.

Right now the only solution for this is disabling the no-exclusive-tests rule. It would be great to have autofix be opt-in for this rule.

Switch to semantic releasing

Consider switching to semantic-release package publishing and a more strict commit message format.

This proved to work quite well and transparently for me at https://github.com/alecxe/eslint-plugin-protractor.

Thanks.

Rule proposal: no pending tests

Inside a large mocha test suite, it's quite difficult to hunt down pending specs of PRs past. It would be awesome to be able to mark them as warnings to pester each other to implement or delete them.

OK

describe('some module', function() {
  it('implements a test', function() {
    expect(true).to.be.ok;
  });
});

Error

describe('some module', function() {
  it('does not yet implement a test');

  it.skip('implemented but skipped', function() {
    expect(false).to.be.ok;
  });
});

describe.skip('another module', function() {
  // ...
});

Warn if done callback is not called

This should be considered as a warning:

it('foo', function (done) {
    asyncFunction(function (err, result) {
        expect(err).to.not.exist;
    });
});

Rule proposal: avoid setup code outside of tests or hooks

This should be considered a warning:

describe('foo',  function ()  {
    var fixture = fs.readFileSync('foo.txt');
});

You should read the file in a before hook instead.

The problem with such code is that it will be executed during the process startup time. It will be even executed if you skip the test suite.

---

I'm not 100% sure how to achieve this. Disallowing every call expression would also disallow using forEach for parameterized tests.

Update README to include eslint environment

eslint does not use the plugin if mocha is not mentioned in the environment. Please update the readme to include that in config:

{
  "env": {
    "mocha": true
  },
  "plugins": [
    "mocha"
  ],
  "rules": {
    "mocha/no-exclusive-tests": "error"
  }
}

Similar issue occured in the eslint-plugin-jasmine and environment declaration fixed it.

Rule proposal: disallow before, beforeEach, after, afterEach if you have only one test

Peer of eslint@^2.0.0 required but none was installed

Noticing the following error in terminal, wondering if an update is needed for this plugin? Please advise. Thanks!

npm WARN [email protected] requires a peer of eslint@^2.0.0 but none was installed.

FWIW, I have also ensured that the most up-to-date versions of both packages are installed.

Warn if a test uses async code in a synchronous test

This should be considered as a warning:

it('foo', function () {
    anyAsyncFunction(function (error, result) {
        expect(result).to.be.ok;
    });
});

it('foo', function () {
    anyAsyncFunction()
        .then(function (result) {
            expect(result).to.be.ok;
        });
});

This should not be considered as a warning:

it('foo', function (done) {
    anyAsyncFunction(function (error, result) {
        expect(result).to.be.ok;
        done();
    });
});

it('foo', function (done) {
    anyAsyncFunction()
        .then(function (result) {
            expect(result).to.be.ok;
            done();
        });
});

it('foo', function () {
    return anyAsyncFunction()
        .then(function (result) {
            expect(result).to.be.ok;
        });
});

Rule proposal: forbid top level before / after hooks

Those should be only used in a describe block

Rule proposal: no-nested-its

Today I accidentally wrote some tests like this:

describe("a", function() {
  it("b", function() {
    it("c1", function() {
       expect(...);
    });

    it("c2", function() {
       expect(...);
    });
  });
});

instead of like this:

describe("a", function() {
  describe("b", function() { // <-----------
    it("c1", function() {
       expect(...);
    });

    it("c2", function() {
       expect(...);
    });
  });
});

This caused mocha to not run tests "c1" or "c2" at all, since they were accidentally declared inside an it block instead of a describe block.

I only noticed because my code coverage tool said it was missing certain execution branches that I could have sworn I had written tests for. A linter setting to point out this subtle mistake would be helpful IMO.

Rule proposal: no same level suites and tests

Bad

describe('root', function() {
    it('a', function() {});
    it('b', function() {});
    describe('bar', function() {
        it('x', function() {});
        it('y', function() {});
    });
});

Good

describe('root', function() {
    describe('foo', function() {
        it('a', function() {});
        it('b', function() {});
    });

    describe('bar', function() {
        it('x', function() {});
        it('y', function() {});
    });
});

no-hooks: provide option to choose which hooks to allow

It would be great if the no-hooks rule accepted options specifying which hooks to allow. Specifically I'd like to be able to allow all hooks except before.

Would you accept a PR for this functionality?

`no-synchronous-tests` rule not exposed

It seems that the new rule wasn't added to the index.js file.

no-synchronous-tests fails with function in concise form returning property

These cases pass:

it('this returns a promise', () => {
    return obj.promise();
});

it('this returns a promise', () => {
    return obj.promise;
});

it('this returns a promise', () =>
    obj.promise()
);

But this case fails no-synchronous-tests

it('this returns a promise', () =>
    obj.promise
);

However, there is no way for eslint to know if obj.promise is a promise or not.

The chai plugin chai-as-promised uses this syntax for promise testing:

expect(promise).to.be.rejected

.rejected is a promise and should pass this rule.

Rule proposal : Disallow arrow functions as parameters of mocha functions

It's discouraged: http://mochajs.org/#arrow-functions

Rule proposal: no skipped tests

It would be nice to have a rule that can warn/fail when skipped tests are found.

There's an issue discussing adding something like this to mocha itself, but I would personally prefer to have it in eslint.

I can work on a PR for this if you're open to adding the feature.

Feature Request: allow users to provide variables to protect from `.only`

I'm using ember-cli-mocha which provides some functions that build on top of describe, such as describeModule. I would love to be able to provide an array of additional things to disallow .only from being called.

Support `specify` alias to `it`

We use that pretty extensively in jsdom, but it looks like the no-exclusive-tests rule only supports it

Add package.json

Add a package.json which contains all project information and dependencies.

Add support for mocha's tdd style

Currently, the plugin only supports mocha's bdd style with describe and it. It could easily also support mocha's tdd style, by just enhancing the checks for suite and test as well.

Could you perhaps support that, too, please?

Crash in no-identical-titles: "Cannot read property 'testTitles' of undefined"

Cannot read property 'testTitles' of undefined
TypeError: Cannot read property 'testTitles' of undefined
at EventEmitter.CallExpression (/Users/wstamper/github/BBUI/SetupAssistantUI/node_modules/eslint-plugin-mocha/lib/rules/no-identical-title.js:51:54)
at emitOne (events.js:101:20)
at EventEmitter.emit (events.js:188:7)
at NodeEventGenerator.enterNode (/Users/wstamper/github/BBUI/SetupAssistantUI/node_modules/eslint/lib/util/node-event-generator.js:40:22)
at CodePathAnalyzer.enterNode (/Users/wstamper/github/BBUI/SetupAssistantUI/node_modules/eslint/lib/code-path-analysis/code-path-analyzer.js:608:23)
at CommentEventGenerator.enterNode (/Users/wstamper/github/BBUI/SetupAssistantUI/node_modules/eslint/lib/util/comment-event-generator.js:97:23)
at Controller.enter (/Users/wstamper/github/BBUI/SetupAssistantUI/node_modules/eslint/lib/eslint.js:915:36)
at Controller.__execute (/Users/wstamper/github/BBUI/SetupAssistantUI/node_modules/estraverse/estraverse.js:397:31)
at Controller.traverse (/Users/wstamper/github/BBUI/SetupAssistantUI/node_modules/estraverse/estraverse.js:501:28)
at Controller.Traverser.controller.traverse (/Users/wstamper/github/BBUI/SetupAssistantUI/node_modules/eslint/lib/util/traverser.js:36:33)

Rules proposal: one describe per file

The rationale and the use case we have is described here Enforce one describe per file.

To summarize - the rule should produce a warning if there is more than one top-level describe per file.

Probably, we have too narrow of a use case - feel free to close if you feel there is no need for this kind of rule. Or, may be we can just turn it off by default.

Thanks!

Rule proposal: avoid before / after hooks

Using before or after hooks is almost always a sign that you use shared state between multiple tests. This could lead to unwanted side-effects, e.g. some tests only pass/fail if the tests are executed in a specific order.

Rename no-calls-to-mochas-only-function rule

Rename the no-calls-to-mochas-only-function rule to a shorter name.

According to the mocha documentation .only-tests are called "exclusive tests". I suggest to call the rule no-exclusive-tests.

Forbid duplicate test names

Drop node 0.10 and iojs support

With the next major version we should consider to stop supporting node 0.10 and iojs officially.

handle-done-callback: allow passing done as callback

this should not cause an error:

it('should not cause an error', function(done) {
  var obj = {someFunc: done};
  somethingThatCallsSomeFuncOnObj(obj);
});

should it?

add a preset with all rules enabled

Rule proposal: No multiple hooks on the same level

In the codebase of one of my projects, I often see multiple hooks of the same type being defined under the same describe level:

describe('foo', function() {
  before(function() {
    // set up something
  });
  before(function() {
    // set up something else
  });

  // tests
});

This can be confusing when one hook is before the tests and the other after the tests (think after/afterEach). Anyway, this is IMO not a good pattern as you can and should do everything in one hook.