platinumazure / eslint-plugin-qunit Goto Github PK

assert.throws has removed (block, string, string) signature in QUnit 2.0. Need a new rule to catch this and add to qunit/two configuration.

Probably worth adding to qunit/recommended configuration, since (block, regex, string) is more flexible anyway.

No need to flag (block, string) form as that has always been interpreted as an assertion message rather than an expected string, so its behavior is unchanged.

Marked as "breaking" since changes to shared configurations are very likely to be breaking changes.

It would be great to have something like https://github.com/lo1tuma/eslint-plugin-mocha/blob/master/docs/rules/no-identical-title.md but for QUnit instead.

This rule should be added to qunit/two config.

Examples that should warn:

test("test name", 0, function () { });

QUnit.test("test name", 0, function () { });

asyncTest("test name", 0, function () { });

QUnit.asyncTest("test name", 0, function () { });

Examples that should not warn:

// No expect is allowed by this rule (see "require-expect")
test("test name", function () { });
QUnit.test("test name", function () { });
asyncTest("test name", function () { });
QUnit.asyncTest("test name", function () { });

// Global expect() okay by this rule (see "no-global-expect")
test("test name", function () { expect(0); });
QUnit.test("test name", function () { expect(0); });
asyncTest("test name", function () { expect(0); });
QUnit.asyncTest("test name", function () { expect(0); });

// assert.expect preferred
test("test name", function (assert) { assert.expect(0); });
QUnit.test("test name", function (assert) { assert.expect(0); });
asyncTest("test name", function (assert) { assert.expect(0); });
QUnit.asyncTest("test name", function (assert) { assert.expect(0); });

Some people may not know to look in docs/lib/rules to discover which rules this plugin supports. It may also be worth calling out that the plugin is stable despite its 0.x version number.

The no-negated-ok rule should not warn on ok(!something). This is because there is no global notOk assertion.

There is also no point in warning for global notOk either, since that isn't a real assertion.

In summary, this rule should only really be enforced for MemberExpression assert.ok and assert.notOk.

As arrow functions become more commonly used, developers may use them to create QUnit test callbacks. This changes the expected test context to something else (probably to window). Some testing setups have specialized modules that use this inside beforeEach to manipulate the context, so changing it can be problematic.

Example that should warn:

test('description', (assert) => {
});

Example that is ok:

test('description', function(assert) {
});

I haven't thought too much about implementation yet, but it seems fairly simple to look for the test callback and then ensure that an arrowFunctionExpression is not being used.

It might also make sense to apply this rule to the beforeEach and afterEach callbacks in module setup.

The isEqual(node.callee) check should be accompanied by a check of the testStack. We should only be checking these calls if we are in a test anyway.

no-negated-ok should not warn for double negation (!!)

Ensure that expect is called when using assertions outside of top-level test callback

QUnit's assert.expect(...) helps developers create tests that correctly fail
when their expected number of assertions are not called. QUnit will throw an
error if no assertions are called by the time the test ends. This rule goes
further and ensures that expect is used anytime a developer uses are assertion
that is nested in a block or anytime a developer passes assert to another
function.

Rule Details

The following patterns are considered warnings:

test('name', function(assert) {
    assert.ok(true);
    while (false) {
        assert.ok(true);
    }
});

test('name', function(assert) {
    assert.ok(true);
    maybeCallback(function() {
        assert.ok(true);
    });
});

test('name', function(assert) {
    assert.ok(true);
    var callback = function() {
        assert.ok(true);
    }
    callback();
});

test('name', function(assert) {
    assert.ok(true);
    maybeCallback(function() {
        assert.expect(2); // Must be called in the top test function.
        assert.ok(true);
    });
});

test('name', function(assert) {
    assert.ok(true);
    myCustomAssertionWrapper(assert);
});

The following patterns are not considered warnings:

test('name', function(assert) {
    assert.ok(true);
});

test('name', function(assert) {
    assert.expect(2);

    if (true) {
        assert.ok(true);
        callMeBack(function() {
            assert.ok(true);
        });
    }
});

test('name', function(assert) {
    assert.expect(1);
    myCustomAssertionWrapper(assert);
});

When Not to Use It

1. If your tests have some logic that relies on an unpredictable number of
   assertions being called.
2. If you want to pass the assert context to functions without using expect.

Binary logical operators should never be needed in assertion arguments. Usually it should be possible to decompose the assertion into multiple assertions if needed.

Additionally, using logical-or in an assertion may be a sign that a test is non-deterministic.

Or possibly add a separate rule for that?

Rule "no-async-in-loops" does not respect other assert context identifiers besides "assert".

The following cases should be invalid but currently pass:

• test('Name', function (foo) { while (false) foo.async(); });
• test('Name', function (foo) { do foo.async(); while (false); });
• test('Name', function (foo) { for (;;) foo.async(); });
• test('Name', function (foo) { for (i in {}) foo.async(); });
• test('Name', function (foo) { for (i of {}) foo.async(); });

No major breaking changes occurred in [email protected], so we can relax the peerDependency specification.

Define what sorts of changes will trigger major, minor, and patch version updates according to semantic versioning.

The no-async-in-loops rule still uses "assert.async()" in its message, even if the assert variable has another name. The rule does catch other async calls in loops even with a different assert variable name, but the message should respect the assert variable name to be more useful.

Now that QUnit 1.20.0 is out and the QUnit.only is available, it would probably be a good idea to add an ESLint rule to forbid its use (at least on check-in).

Similar to no-qunit-stop but should also check that the call is inside a test or a module hook.

We should create a rule which detects commented out tests and requires the use of QUnit.skip instead.

A minor annoyance I noticed with require-expect using the "except-simple" option in is that it adds an error for every assert that is called without calling expect first. Shouldn't be too hard to report one error per test.

We should create a rule which ensures that equality checks are not performed as the assertion argument of assert.ok and assert.notOk.

Ideally, the rule would suggest a better assertion to use instead:

• assert.ok(a === b) would become assert.strictEqual(a, b)
• assert.ok(a == b) would become assert.equal(a, b)
• assert.ok(a !== b) would become assert.notStrictEqual(a, b)
• assert.ok(a != b) would become assert.notEqual(a, b)
• assert.notOk(a === b) would become assert.notStrictEqual(a, b)
• assert.notOk(a == b) would become assert.notEqual(a, b)
• assert.notOk(a !== b) would become assert.strictEqual(a, b)
• assert.notOk(a != b) would become assert.equal(a, b)

With ESLint >=3.18.0, AST selectors are now available for rules to use.

Lots of boilerplate could potentially be removed by using those selectors. However, this requires bumping the ESLint peerDependency to at least 3.18 and is thus a breaking change.

Need to add .travis.yml file for travis-ci builds. Will probably model off of eslint-plugin-backbone.

The README needs to be updated to show the shared configs defined in index.js.

Need to determine which version of ESLint to aim for (around 2.6 or 2.8?). Want to support both plugin shareable configuration and new-style rules (rules as objects rather than factory functions).

The purpose of this rule is to prevent unit tests from returning early (even conditionally) if there are still assertions further in the test code. If a test may need to return early, this is a sign that the test either is testing nondeterministically (in which case the test or code should be refactored to support deterministic testing), or that the entire test might need to be skipped in certain conditions.

The following example is a problem under this rule:

QUnit.test("test", function (assert) {
    if (true) {
        return;
    }
    assert.ok(false);
});

The following examples are not problems under this rule:

if (condition) {
    QUnit.test("test", function (assert) {
        assert.ok(false);
    });
}

QUnit[condition ? "test" : "skip"]("a test", function (assert) {
    assert.ok(false);
});

It would be good to have a rule that checks if the number of arguments to assertions is correct.

The rule could have an option { requireMessage: true } which would require an assertion message in all cases. If this option is not specified, then assertions without messages would be acceptable.

The following patterns would trigger a warning:

• ok(result, true, "This really should be equal() or similar");

The following patterns would trigger a warning with requireMessage: true on:

• ok(result);
• equal(a, b);
• throws(function () {}, TypeError);

Still need to flesh this out a bit.

The goal of this rule is to try to catch cases where there is potential for tests being dependent on each other (or on the order they are run) due to a shared dependence on mutable fixtures.

Example problem:

var myFixture = [];

QUnit.test("Mutates", function (assert) {
    myFixture.push("a value");
    assert.strictEqual(myFixture.length, 1);
});

QUnit.test("Reads", function (assert) {
    assert.strictEqual(myFixture.length, 0);   // this will fail
});

Ideally, this rule would use escope to look at variable references and would flag the definition node if the initial value is mutable (object or array) and the variable is accessed in at least one test. The lint message should suggest using a function to return the fixture to avoid shared state.

We should create a new rule which forbids assert.equal (preferring instead assert.strictEqual, assert.deepEqual, or assert.propEqual as needed).

Create 1.0.0-rc0 release candidate so that people can try out the new shareable configs more easily.

assert-args was found to be semi-unusable due to lots of assertions not having a string literal message. Goal is to add support for more constrained configuration later, but for now we need to fix the docs to acknowledge that any node type is accepted as a possible assertion message.

Depends on #43.

Rules "resolve-async" and "no-async-in-loops" currently use some similar functions to detect calls to stop(), start(), assert.async(), and test contexts. These functions should be grouped into their own file to maximize code reuse.

The goal of this issue is to avoid scenarios like this:

QUnit.test("a test", function (assert) {
    var result1 = doSomething();
    assert.ok(result1);

    var result2 = doSomething();
    assert.ok(result2);
});

This test should be split into multiple tests.

In order to allow re-synchronizing async code and clean up any objects that need to be cleaned up, the best approach is probably to warn only

1. when an assertion occurs,
2. after a non-assertion statement (i.e., possible action),
3. which is itself after an assertion statement.

This means one of two things:

1. The extra action can be moved above the assertions (i.e., as part of the unit test action); or
2. The extra action is another test and should be split into a test accordingly.

Internal issue for tracking the chore of transforming all rules into new-style rules (with meta property, etc.).

Documentation: https://github.com/platinumazure/eslint-plugin-qunit/blob/master/docs/rules/no-qunit-start-in-tests.md

There are many setTimeout invocations that have the wrong indentation.

The following should be invalid:

assert.strictEqual(1 + 1 === 2, true);

The following should be valid:

assert.strictEqual(1 + 1, 2);

The use of async controls in loops makes test code much harder to reason about and maintain. We should create a rule which will detect and report those uses.

Some valid test cases:

• test('name', function () { stop(); for (var i = 0; i < 2; ++i) {} start(); });

Some invalid test cases:

• test('name', function () { for (var i = 0; i < 2; ++i) { stop(); } start(2); });
• test('name', function () { for (var i in obj) { stop(); } });
• test('name', function () { for (var i of obj) { stop(); } });
• test('name', function () { var i = 2; while (i--) { stop(); } start(2); });
• test('name', function () { var i = 2; do { stop(); } while (i--); start(2); });

This rule should warn if any of the built-in two-argument assertions are invoked with arguments in the wrong order.

Will flag the following:

assert.equal("a literal", someVar);
assert.equal("a literal", someVar, "a message");
// etc. for strictEqual, deepEqual, propEqual, notEqual,
// notStrictEqual, notDeepEqual, notPropEqual

Will not flag the following:

assert.equal(someVar, "a literal");
assert.equal(someVar, "a literal", "a message");
assert.equal(expected, result);    // Args are in wrong order but we can't flag them :-(
assert.equal(expected, result, "a message");
// etc. for strictEqual, deepEqual, propEqual, notEqual,
// notStrictEqual, notDeepEqual, notPropEqual

This rule seems like a good best practice and should be added to qunit/recommended.

This is not a QUnit 2.0 compatibility rule, so no need to add it to qunit/two.

Enable these ESLint rules:

• no-var
• prefer-const
• prefer-arrow-callback Can add this later if desired
• arrow-body-style Can add this later if desired
• prefer-template
• no-const-assign
• template-curly-spacing

Currently we are just tracking the number of calls to QUnit.stop() and QUnit.start(), regardless of any semaphore count that might be passed in as an argument. We need to take the semaphore argument into account.

Some test cases which should be valid but currently aren't:

• test('name', function () { stop(2); start(); start(); });
• test('name', function () { stop(); stop(); start(2); });
• asyncTest('name', function () { stop(); start(2); });
• asyncTest('name', function () { stop(2); start(3); });

Problem is in line 80 of no-throws-string.

All you get is a 404 from GitHub.

Example:

QUnit.test("a test", function () {
    require(["some/path"], function (dep) {
        if (true) {
            return;
        }

        assert.ok(true);
    });
});

Seems we will need to find the function scope in which assertions are being invoked, and then start at that level for linting ReturnStatements.

Once #51 is implemented, the rule should be added to qunit/two configuration (as soon as we are ready for a major release).

This shareable config for QUnit 2.0 would contain all the rules created for issue #20.

Each does not do anything with its iteratee return value.

Assertions should not be run inside a conditional statement. Assertions run inside conditional statements imply either non-determinism, or else that the test needs to be broken out into multiple, simpler tests.

The following code is problematic under this rule:

if (foo) {
    assert.ok(true);
}

if (foo) {
    doSomething();
} else {
    assert.ok(true);
}

Also, a lot of the time, the condition in question could be asserted on instead: if (foo) becomes assert.ok(foo);.

Per the migration guide, this implies a few warnings (which may or may not be separate rules):

• Forbid use of module, test, and asyncTest globals
• Forbid use of QUnit.asyncTest
• Forbid global stop()/start()
• Require assert.async() instead of QUnit.stop()
• Forbid use of assertion globals
• Forbid use of global expect()
• Require beforeEach and afterEach module hooks instead of setup and teardown
• Forbid QUnit.push (use this.push in custom assertion)
• Forbid QUnit.init
• Forbid QUnit.reset
• Forbid assignments to QUnit.log and other logging callbacks
• Forbid QUnit.jsDump, favoring QUnit.dump instead

It is preferable to use .notOk(something) rather than .ok(!something) since it will improve the readability of the error message.

This rule would be especially valuable since .notOk() was only introduced in version 1.18.0. Projects that started using the framework prior to that would have had to use the negated .ok() value approach.