eslint-plugin-ava icon indicating copy to clipboard operation
eslint-plugin-ava copied to clipboard

Ensure argument types in the `assertion-arguments` rule

Open sindresorhus opened this issue 8 years ago • 7 comments

https://github.com/sindresorhus/eslint-plugin-ava/pull/95#issuecomment-216173852

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.

sindresorhus avatar May 02 '16 09:05 sindresorhus

#86 will help with this.

If someone is putting a non-string inline, it's usually an expression, like prefix + ' some title', which you can be confident always evaluates to a string without evaluating what prefix is.

That's not to say we couldn't also pursue the more complex evaluation, though I'm not sure you would need to bother with CodePath analysis. Just iterating every assignment for a particular scope should get you there. It's really unlikely that you will see prefix = "foo" and prefix = 3 in the same scope. If it does comes to that, I say just give up and accept that it's probably a string by the time it reaches the assertion call instead of using CodePath analysis. CodePath analysis is hard enough to use when you're looking for something really specific (like t.end). Tracking types through the code-path would be hard, and would provide really minimal improvements over other methods.

jamestalmage avatar May 03 '16 05:05 jamestalmage

@jamestalmage Good points. I agree.

In addition, one mistake I've done multiple times is the order of arguments in t.regex(). I've sometimes put the regex first and not last. Would be nice if the editor warned me about that.

sindresorhus avatar May 03 '16 09:05 sindresorhus

I currently face the use case where i want to test for an explicit call of one callback like this:

test.cb('test callbacks', () => {
  run({
    a: () => t.end(true),
    b: () => t.end(false)
  });
});

The linter of course fails, isn't this a intended use case of t.end?

Is this issue still active and what approach should be taken to fix it? ☕️

florianb avatar Jan 07 '17 13:01 florianb

t.end() does not take arguments. If with t.end(false) you meant to fail the test, I'd suggest using t.fail() or throwing something instead.

This issue is still active, nobody started working on it. If you feel like working on it, please do :) I think that a good first step for this rule would be to ignore arguments that are too complex to identify their type (Identifiers, function calls, expressions, etc...) and only report things whose type you know for sure (functions, strings, regexes, numbers, etc.)

jfmengels avatar Jan 07 '17 13:01 jfmengels

Interesting, thanks @jfmengels. I didn't get t.fail() work within a test.cb():

$ xo && ava

  test.js:17:1
  ✖  17:1  Callback test was not ended. Make sure to explicitly end the test with t.end().  ava/test-ended

I guess i will take an approach to improve this rule. :)

florianb avatar Jan 07 '17 13:01 florianb

I don't see how this discussion is relevant to this issue. Use Gitter or open a new issue ;)

sindresorhus avatar Jan 07 '17 14:01 sindresorhus

Aww - sorry for that: #164

florianb avatar Jan 09 '17 09:01 florianb