Hey guys,

eslint-plugin-ava has had plenty of new rules (including all the ones listed in the beginning) since its last publication on npm. Should we publish it now?
Also, I was wondering whether we should release it as 1.0.0. The project is unlikely to have a breaking change soon, so I'd say it's better to move it to a "normal" semver version before it gets used a lot.

What do you guys think?

Rule proposal: Enforce that AVA gets imported with test as a variable name.

Bad

var foo = require('ava');
let foo = require('ava');
const foo = require('ava');
import foo from 'ava';

Good

var test = require('ava');
let test = require('ava');
const test = require('ava');
import test from 'ava';

var test = require('foo');
import test from 'foo';

I think this should be a recommended rule at error level, especially considering that all rules assume that AVA is named test. What do you think?

Rule proposal: Enforce that the test function has either no parameters, or only one parameter named t.

Bad

test(foo => { // Incorrect name
  // ...
});

test((foo) => { // Incorrect name
  // ...
});

test((t, bar) => { // too many arguments
  // ...
});

test((bar, t) => { // too many arguments
  // ...
});

Good

test(t => {
  // ...
});

test(foo); // As I don't think we can infer the arguments of foo, or even if it is a function

test.todo("foo");

I think this should be a recommended rule at error level, especially considering that all rules assume that t is indeed called t. What do you think?

If you use max-assertions, it warns for every additional assertion. I think it should just warn for the first assertion in excess of the max (at most once per test). Perhaps the message could specifically say how far over the limit.

Require or disallow the message parameter being used for assertions.
With options for always require or to never allow.
Default to always.

Option Always

Good

test(t => {
    t.pass('is passing');
    t.fail('is failing');
    t.ok('unicorn', 'is unicorn');
    t.notOk('unicorn', 'is not unicorn');
    t.true(true, 'is true');
    t.false(false, 'is false');
    t.is('same', 'same', 'is same');
    t.not('not', 'same', 'is not same');
    t.deepEqual({}, {}, 'is same object'); 
    t.notDeepEqual({}, {a: true}, 'is name same object'); 
    t.throws(Promise.reject(), 'is exception');
    t.notThrows(Promise.resolve(), 'is not exception');
    t.regex(a, /a/, 'is matching pattern');
    t.ifError(new Error(), 'is error');
});

Bad

test(t => {
    t.pass();
    t.fail();
    t.ok('unicorn');
    t.notOk('unicorn');
    t.true(true);
    t.false(false);
    t.is('same', 'same');
    t.not('not', 'same');
    t.deepEqual({}, {}); 
    t.notDeepEqual({}, {a: true}); 
    t.throws(Promise.reject());
    t.notThrows(Promise.resolve());
    t.regex(a, /a/);
    t.ifError(new Error());
});

Option Never

Good

test(t => {
    t.pass();
    t.fail();
    t.ok('unicorn');
    t.notOk('unicorn');
    t.true(true);
    t.false(false);
    t.is('same', 'same');
    t.not('not', 'same');
    t.deepEqual({}, {}); 
    t.notDeepEqual({}, {a: true}); 
    t.throws(Promise.reject());
    t.notThrows(Promise.resolve());
    t.regex(a, /a/);
    t.ifError(new Error());
});

Bad

test(t => {
    t.pass('is passing');
    t.fail('is failing');
    t.ok('unicorn', 'is unicorn');
    t.notOk('unicorn', 'is not unicorn');
    t.true(true, 'is true');
    t.false(false, 'is false');
    t.is('same', 'same', 'is same');
    t.not('not', 'same', 'is not same');
    t.deepEqual({}, {}, 'is same object'); 
    t.notDeepEqual({}, {a: true}, 'is name same object'); 
    t.throws(Promise.reject(), 'is exception');
    t.notThrows(Promise.resolve(), 'is not exception');
    t.regex(a, /a/, 'is matching pattern');
    t.ifError(new Error(), 'is error');
});

I'm just tying out a test like this:

;[
  ['BIT_STRING', BIT_STRING],
  ['CHARACTER_STRING', CHARACTER_STRING],
  ['OCTET_STRING', OCTET_STRING]
].forEach(([title, type]) => {
  test(`decodes ${title}`, t => {
    const expected = new Buffer('foo')
    t.same(decode(type, 3, expected, 0), expected)
  })
})

;[
  ['ENUMERATED', ENUMERATED],
  ['INTEGER', INTEGER]
].forEach(([title, type]) => {
  test(`decodes ${title}`, t => {
    t.is(decode(type, 1, new Buffer([42]), 0), 42)
    const expected = new Buffer(7)
    t.same(decode(type, 7, expected, 0), expected)
  })
})

The no-identical-title complains about a duplicate title in decodes ${title}. It should probably recognize the title is a template string and is most likely not a duplicate.

As far as i can tell, we have three types of changes in this repo:

• Bug fixes
• Rule additions/changes for stuff existing in the current version of AVA
• Rule additions/changes for stuff that is yet to land in AVA

For instance #71 should probably not be released before the next release of AVA that includes it (be it 0.14.1 or 0.15.0). Rules prohibiting t.same and t.ok (had we had them before the 0.14 release) also should not be published before 0.14 lands on npm.

Should we have two separate branches? One for stuff that can be published right away, and one for stuff that is "on hold"? We can also put merges on hold, but that can be painful for contributors, especially if there are rebase/fixes needed a month after the PR was made.

eslint version: 2.9.0
eslint plugin ava: 2.3.0

assertion-arguments rule warns when t.throws is passed three parameters.
According to AVA docs t.throws is valid with one, two, or three parameters.

.eslintrc.yml

env:
    node: true
    es6: true
plugins:
    - ava
rules:
    ava/assertion-arguments: error

example code

const test = require('ava');

test('reduced test case', (t) => {
    // valid test
    // passes lint check
    t.throws(
        () => {
            throw new Error('example');
        }
    );

    // valid test
    // passes lint check
    t.throws(
        () => {
            throw new Error('example');
        },
        'example'
    );

    // valid test
    // fails lint check
    t.throws(
        () => {
            throw new Error('example');
        },
        'example',
        'some message'
    );
});

I've seen a lot of incorrect use of the throws assertion in other test runner and even done the mistake myself sometimes. Now I'm beginning to see it with AVA too, so would be nice to be preemptive about it.

People don't realize they need to wrap their call in a function, so many end up doing t.throws(foo()) instead of t.throws(() => foo());. It's an easy mistake to make.

Happened yesterday: avajs/ava#739

We should disallow doing t.throws(foo()) and t.notThrows(foo()).

I think we can easily add auto-fixing to this by just wrapping the call in an arrow function like so t.throws(() => foo());.

See: sindresorhus/eslint-plugin-unicorn#19

This will be possible when ESLint 2.0.0 is released.

See: eslint/eslint#3659

Proposal: Create a rule that disallows certain combination of modifiers, such as

test.only.skip(...)
test.before.only(...)
test.before.skip(...) // ?
test.before.beforeEach()
test.before.after()
test.before.serial()
// others?

Value: Earlier feedback when linting in the editor

Let me know if you think that this is an unnecessary rule or not (it probably is, or very close to).

#95 (comment)

Mainly for the message argument in assertions and plan count in t.plan().

Would be cool to use code paths for this in case the message or plan count is not inline. It's usually always defined in the same file though.

Does it really make sense to make it error by default in the recommended preset? Usually, I skip tests that doesn't work for some reason and I don't have time to fix right now. It's weird having to also add a ESLint ignore comment.

I propose we make it a warning.

Rule proposal: Ensure that no assertions are skipped.
Similar to the no-skip-test proposal, but for assertions. They are especially "dangerous" as they do not appear in the end output.

Fail

const test = require('ava');

test('some title', (t) => {
    t.skip.is(1, 1);
});

Pass

const test = require('ava');

test('some title', (t) => {
    t.is(1, 1);
});

I already wrote a rule for this, so I'll submit it if this seems like a good idea to you (after some of the other PRs I proposed get merged)

Require tests to have a title. Should have an option to enable an exception for when there is only one test.

avajs/ava@e9c6cc2
avajs/ava@a7f50eb

Is there a reason plan is not one of the whitelisted methods?

Discovered via

Unknown assertion method `plan`  ava/use-t-well

Followup to #73

We have this code construct everywhere:

CallExpression: function (node) {
  // only check if we are within a test file, and if this CallExpression is within a test.
  if (!ava.isTestFile || !ava.currentTestNode) {
    return;
  }
}

I propose we add some sugar to ava.merge() that allows something like this:

'CallExpression:inTest': function (node) {

},
'VariableDeclaration:inTestFile': function (node) {

},
'CallExpression:inTest:inTestFile': function (node) {

},

While we are at it, we should define one (specifically for CallExpression), that checks the callee is a MemberExpression of the form t.XXX:

'AssertionCallExpression:inTest': function (node) {

}

It's easy to do make a test skipped with test.skip() and then forget about it. It's visible in the results, but still easily missed. Some projects/companies might want to prevent such tests from entering the CI.

We should recommend using test.skip() instead.

This:

// test('foo', => {
//  t.pass();
// });

Should instead be:

test.skip('foo', => {
    t.pass();
});

Currently the default is always, but I honestly think it would be better to have if-multiple as a default.

https://github.com/sindresorhus/eslint-plugin-ava/blob/master/docs/rules/test-title.md

There's really no point in forcing naming a test if there's only one, and if users really want this they can just change the default.

Thoughts?

A lot of the rules have hardcoded t as the assertion library name, meaning two things:

• If the dev uses another name for t, such as assert, rules dealing with it will not lint anything
• If the dev uses another name for t, and uses t for something else in the test function, the linter may go haywire.

I've seen a related issue in AVA's issues. I think we should either of the following:

• Add a note in the README saying that t should be used, and why, including the fact that this plugin depends on it heavily, and the reasons that make t great when used in AVA (power-assert) (cc @twada). The description of the use-t rule should be updated too.
• Make the rules t-agnostic. Most of the work would probably be done in create-ava-rule and then simple updates in the rest of the rules + tests covering.

It could be nice that you share a default eslint-config-ava that define a default configuration for the eslint-plugin-ava rules. It will allow us to use the eslint extends feature: extends: ['airbnb', 'ava'] and avoid us to have a full block of configurations that is complicated to keep up to date (especially to add new ava rules):

{
    // ...
    'rules': {
        // ...
        'ava/assertion-message': ['error', 'always'],
        'ava/max-asserts': 'off',
        'ava/no-cb-test': 'warn',
        'ava/no-identical-title': 'error',
        'ava/no-ignored-test-files': 'error',
        'ava/no-invalid-end': 'error',
        'ava/no-only-test': 'error',
        'ava/no-skip-assert': 'error',
        'ava/no-skip-test': 'error',
        'ava/no-statement-after-end': 'error',
        'ava/no-todo-test': 'warn',
        'ava/no-unknown-modifiers': 'error',
        'ava/prefer-power-assert': 'off',
        'ava/test-ended': 'error',
        'ava/test-title': ['error', 'always'],
        'ava/use-t-well': 'error',
        'ava/use-t': 'error',
        'ava/use-test': 'error',
        'ava/use-true-false': 'error'
    }
}

With a default ``eslint-config-ava` all we have to do is to (maybe) tweak one or two rules to match our requirements.

It's now moot with the introduction of the assertion-arguments rule.

What's the best-practise for deprecating rules with ESLint?

As discussed in #90 (comment) and in #90 (comment).

Assertions like this evaluate to true or false:

t.truthy(array.indexOf(value) !== -1);

We should require you to write them like:

t.true(array.indexOf(value) !== -1);

Rule proposal: Check the good use of t.plan

• Check that t.plan(n) is used with n existing being one integer parameter
• Check that the number of assertions equals n
• Ignore this rule when n is not a Literal number (impossible to know how many assertions there are)

Use case: When linting the code in the editor. Not very useful otherwise.

Name TBD

Bad

test(t => {
  t.plan();
  t.is(1, 1);
});

test(t => {
  t.plan(2); // Incorrect number of assertions
  t.is(1, 1);
});

test(t => {
  t.plan('2'); // Not an integer
  t.is(1, 1);
});

test(t => {
  t.plan(2.5); // Not an integer
  t.is(1, 1);
});

test(t => {
  t.plan(1); // Incorrect number of assertions in all cases
  if (SOME_VAL === 1) {
    t.is(1, 1);
  }
});

Good

test(t => {
  t.plan(1);
  t.is(1, 1);
});

test(t => {
  t.plan(1);
  if (SOME_VAL === 1) {
    t.is(1, 1);
  } else {
    t.is(2, 2);
  }
});

test(t => {
  t.plan(SOME_OTHER_VAL); // Ignore this case, as it's not possible (usually?) to find out
  t.is(1, 1);
});

Since we're trying out a lot of crazy ideas for AVA, I had this idea the other day that we can help users not do silly mistakes by having some AVA specific linter rules. I've put together this prototype as a proof of concept.

@vdemedes @jamestalmage What do you think about the idea?

If you agree with the idea, what rules should we include? What issues are users likely to run into?

• no-cb-test
• no-skip-test

It's not yet in AVA, but opening this issue so I don't forget.

avajs/ava#565

http://eslint.org/docs/user-guide/migrating-to-2.0.0

Disallow tests with identical titles as it makes it hard to differentiate.

Might be better off in AVA itself: avajs/ava#517

Limit the amount of assertions per test to enforce people to split up large tests into smaller ones. Should accept a number of assertions.

We have test-title with the if-multiple setting for that.

Currently we create two violations for this one mistake.

Rule proposal: Enforce the use of t.plan(n) in tests.

Bad

test(t => {
  const bar = foo();
  t.is(bar.a, 1);
  t.is(bar.a, 2);
});

Good

test(t => {
  t.plan(2);

  const bar = foo();
  t.is(bar.a, 1);
  t.is(bar.a, 2);
});

The following results in a violation, but it's a common pattern, so I think we should support it.

import test from 'ava';

test.cb(t => {
    setTimeout(t.end, 10);
});

Same as #3, but for test.only().

Prevent the use of test.cb(). Some projects/companies might want to prevent the use of callback tests in favor of only async functions or returning promises.

When developping and running the tests, I often get the following:

As you can see and imagine, this error message is really not helpful, and I often have to uncomment all the tests to find out which one fails. I thought this was the normal behavior of RuleTester, but then I started contributing to https://github.com/benmosher/eslint-plugin-import, where the errors are a bit clearer

I have the same problem with an other eslint plugin repo of mine, so it would be pretty neat if this could be improved. Do you guys know of ways to improve it?

I'm getting a lot of errors like this on master, since the merge of #18.

9. prefer-power-assert › same(foo, bar) is allowed
  Should have no errors but had 1: [ { fatal: true,
    severity: 2,
    message: 'Parsing error: Illegal import declaration',
    line: 1,
    column: 2 } ]
  AssertionError: Should have no errors but had 1: [ { fatal: true,
    column: 2 } ]
        testValidTemplate (/home/jeroen/dev/fork/eslint-plugin-ava/node_modules/eslint/lib/testers/rule-tester.js:324:20)
        Function.<anonymous> (/home/jeroen/dev/fork/eslint-plugin-ava/node_modules/eslint/lib/testers/rule-tester.js:408:21)
        Function.RuleTester.it (/home/jeroen/dev/fork/eslint-plugin-ava/node_modules/eslint/lib/testers/rule-tester.js:193:19)
        /home/jeroen/dev/fork/eslint-plugin-ava/node_modules/eslint/lib/testers/rule-tester.js:407:28
        Function.<anonymous> (/home/jeroen/dev/fork/eslint-plugin-ava/node_modules/eslint/lib/testers/rule-tester.js:406:24)
        Function.RuleTester.describe (/home/jeroen/dev/fork/eslint-plugin-ava/node_modules/eslint/lib/testers/rule-tester.js:189:19)
        Object.RuleTester.run (/home/jeroen/dev/fork/eslint-plugin-ava/node_modules/eslint/lib/testers/rule-tester.js:405:20)
        Test.fn (prefer-power-assert.js:52:14)
        handleMessage (child_process.js:324:10)
        Pipe.channel.onread (child_process.js:352:11)

@wyze Do you happen to have any ideas regarding this?

 Reproducing

• Simply run npm test (Node v5.7.0)

Only allow use of the assertions that have no power-assert alternative:

• t.ok() (You can do most things with this one)
• t.throws()
• t.notThrows(),
• t.same()
• t.notSame()

Useful for people wanting to fully embrace the power of power-assert.

Rule proposal: Enforce that t.end() is not called in a non-test.cb() test function.

In a rule check that we do not create a test files (a file containing import test from 'ava';?) in a path that is not in an ignored path like fixtures or helpers.

• test/units/foo/myTestFile.js should not trigger an error
• test/units/foo/helpers/myHelper.js should not trigger an error
• test/units/foo/helpers/myTestFile.js should trigger an error

See: #78 (comment)

• We'll need to compile a list of builtin methods that return booleans. Help wanted for that.

Disallow having a function (or simply more than 1 argument) in a test.todo

See avajs/ava#697

I've been told that not giving a title to a t.before() (and friends) triggers the ̀test-title` rule, and I wondered what you guys thought about this. Should we disable this by default for those, and allow them to be enabled with an option?
I don't really see the point of having a title for a before/after, but some might find it useful. Anyway, I think it should be disabled by default at the very least.

hi
I'm trying to use this plugin for eslint my test files but getting error:
Parsing error: Unexpected token t
My test is using async:

test('resolve issues', async t => {
 ...
}

My eslint config is:

{
  "extends": "plugin:ava/recommended",
  "plugins": [
    "ava"
  ]
}

I suppose it will overwirte parser options so that it will use babel-eslint-parser for tests.
Could you suggest what should I do to run eslint over my tests?
thx!

Rule proposal: Enforce that no statements happen after a t.end() is called.

Might be a bit hard to handle if t.end() is called conditionnally, but it's worth a try.

Would be useful to support autofixing for some of the rules. Not all are possible.

http://eslint.org/docs/developer-guide/working-with-rules#contextreport

• no-skip-test (just remove the skip modifier)
• no-only-test (just remove the only modifier)
• no-identical-title (append #1, #2, etc, to the title)
• no-skip-assert (just remove the skip modifier)

Can't think of any way to reliably fix the other rules, but input welcome!

Some of the things in #10 could be useful for other rules and should be extracted into util.js.