platinumazure/eslint-plugin-qunit

New Rule: `no-assert-unexpected-type`

bmish opened this issue · 4 comments

bmish commented

I've put together a quick/rough list of what argument data type checks we could add to each qunit assertion. Feel free to comment with additional suggestions/corrections.

I do think we should have just a single rule to check the argument data types of all assertions, similar to how we have a single rule assert-args to verify the correct number of arguments for all assertions.

We would use getStaticValue to statically determine the type of arguments where possible. Types that can't be determined would be ignored.

  • deepEqual / notDeepEqual
  • equal / notEqual / strictEqual / notStrictEqual
    • suggest deepEqual and propEqual for objects
    • suggest deepEqual for arrays
    • nothing yet
  • false / true
    • disallow non-boolean values - assert.true(123);
  • ok / notOk
    • nothing yet
  • propEqual / notPropEqual
    • autofix/suggest strictEqual for literal values - assert.propEqual(foo, 123); - partially covered by qunit/require-object-in-propequal, this existing rule could be deprecated and replaced by the holistic rule
raycohen commented

It can be valid to assert strictEqual on arrays and objects. For example testing that a method you called returns the exact instance of an array that was passed into it.

assert.true and assert.false will fail with a non-boolean type, so does have a lint requirement add value there?

bmish commented

@raycohen makes sense on your first point, I'll remove that.

One of the goals of this rule would be to catch assertions that we know will fail due to the usage of incorrect types (more immediate feedback vs. running the tests), so I think it would be very valuable to catch that for the boolean assertions.

  • 👍1
mongoose700 commented

For assert.false/assert.true, it should make sure that there's a possibility that the value is false/true respectively, instead of making sure that it's always a boolean. assert.true should work for a true | undefined, or true | string, but not a false.

bmish commented

@mongoose700 that's true, although I wasn't originally planning to take TypeScript information into account in this rule. Considering TypeScript types when available is a good idea, although I want this rule to have basic capabilities with plain JS too. At least initially, I was just planning to use static analysis, like below:

const x = 123;
assert.true(x); // lint violation since we know it's not a boolean
assert.true('hello world'); // lint violation since we know it's not a boolean
assert.deepEqual(foo, 123); // lint violation since this assertion shouldn't be used with literals
assert.propEqual(foo, 123); // lint violation since this assertion shouldn't be used with literals