Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[valid-describe] Support functions as 'name' argument #431

Closed
ulrichb opened this issue Oct 16, 2019 · 8 comments
Closed

[valid-describe] Support functions as 'name' argument #431

ulrichb opened this issue Oct 16, 2019 · 8 comments

Comments

@ulrichb
Copy link

ulrichb commented Oct 16, 2019

Since Jest 22 describe() also allows function references as "name" (instead of a string).

See jestjs/jest#5154.

See also the TS typing: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/f79c08624659aea629bf0d1dc2f30a15e2c68383/types/jest/index.d.ts#L390

This is not supported by valid-describe:

function calc() { /* ... */ }

describe(calc, () => {
    it("adds up two values", () => {
        // ...
    });
});

=> Emits "error First argument must be name jest/valid-describe".

@G-Rath
Copy link
Collaborator

G-Rath commented Oct 16, 2019

Thanks for reporting this.

On the surface it seems easy, but it's actually an interesting problem, b/c it's possibly a lot of work to determine what's valid in that situation.

I feel that this is the point where you'd be best to disable valid-describe, as the problem is one that requires static analysis (i.e TypeScript) rather than a linter, since you can pass pretty much anything.

I'm not going to close this issue, as I think it's worth exploring what we can do, but my initial view is it's a possible can of worms.

Here's a bit of a brain dump of my thinking:

describe can take a string or a function/class.

The following are all valid:

function fn1() {};
const fn2 = () => {};
const fn3 = function() {};
const o = {
  fn4: () => {},
  fn5: function fn5 () {
  }
}

class MyClass {
  myField = () => {};

  myFunction() {
  }
}

// I've probably forgotten a few more

fn1 is easy, so we can ignore that. But ones like fn2 & fn3 are what makes it very interesting - effectively, that means supporting variables, at which point we either have to say a "valid describe" is one that is passed either a string or an identifier (at which point, valid-describe is far less useful, as it'll only error on things like number & object literals), or we track references to identifiers and attempt to resolve the values that they hold when they're used (meaning we're doing the job of a static analyzer, which would only work well for TS code - JS would be very hit and miss).

This would be very complex and out of scope of an eslint plugin very quickly. It's also why I see it as a can of worms: it's very easy to "fix" the first one, the second and third are somewhat easy, but then it's reasonable to ask for support for the rest.


The above is a bit of a brain dump, so it's possible I'm overthinking or have missed something, but for now, I don't know if there's a nice way we can solve this w/o crippling the rule or trying to write a mini-TS.

@garyking
Copy link
Contributor

Sometimes I'll use some weird things to name describe blocks, like regular expressions. But I'll always run String() on them first, so that they become a string. This rule throws an error on that. Same for JSON.stringify(), I imagine. Would be nice if it didn't.

@G-Rath
Copy link
Collaborator

G-Rath commented Oct 16, 2019

But I'll always run String() on them first, so that they become a string. This rule throws an error on that. Same for JSON.stringify(), I imagine. Would be nice if it didn't.

That's the can of worms I was thinking of :)

What you're asking is for us to resolve the type of the parameter at runtime, which is static analysis, and best left to the likes of TypeScript.

Again, not closing this just yet b/c I'm going to think on it for a bit for if there are any easy wins, and to get more opinions, ideas, input, etc, but I can say that whatever we come up w/, ultimately TypeScript will do it better in this case :)

@garyking
Copy link
Contributor

I would just say to give us an option on the rule, to disable checking the name field, since this rule also does other checks as well.

@ulrichb
Copy link
Author

ulrichb commented Oct 17, 2019

+1 for a "validate name argument" option

@jeysal
Copy link
Member

jeysal commented Oct 17, 2019

But I'll always run String() on them first, so that they become a string. This rule throws an error on that.

Maybe this helps:

test(`${fn}`)

We could ofc also consider specifically allowing String(whatever)

@G-Rath
Copy link
Collaborator

G-Rath commented Oct 26, 2019

FYI I'm going to be moving this into valid-title, just b/c of code structure.

From everything I can tell, only describe supports names that are not string, so valid-title will handle checking the title, but have an ignoreTypeOfDescribeName option that should solve this :)

@github-actions
Copy link

🎉 This issue has been resolved in version 23.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants