[valid-title] Use type information to determine titles

[JoshuaKGoldberg]

I noticed that the valid-title rule doesn't use typed linting to determine whether a title is a string. That means dynamic nodes such as Identifiers will always be detected as non-strings:

for (const {name, input, expected} of cases) {
  it(name, () => expect(process(input)).toEqual(expected));
  // ~~~~ Title must be a string 
    
}

let nameBase = "My name: ";

const makeName = (suffix: string) => `first ${suffix}`;

describe(makeName("second"), () => { /* ... */ }
// ~~~~~ Title must be a string 

ACK that this has been brought up before (see next list). But it's really quite inconvenient to have to eslint-disable for every dynamic test name. Could an option similar to the Vitest ESLint plugin to allow using type checking on test names be marked as accepting PRs, please?

Relevant other issues:

• valid-type: use type checking to determine test name type? vitest-dev/eslint-plugin-vitest#251: equivalent one for Vitest
• [valid-title] Allow types that yield string for test titles #1234 (comment) & [valid-title] static class string name property not allowed #1390 (comment): past comments about not using typechecking

[G-Rath]

yup, happy to accept a PR

[G-Rath]

Ok so after working on this, I think it might be better to first step back and discuss more generally what the intent is behind this part of the rule - generally my view is that it's about making titles easy to predict which means using the result of a function call isn't a good choice, even if it does return a string.

I also think its ok for this to clash with "dynamic" tests because by definition I think they're suited to having this check disabled either at the file or line level - part of that is that personally I would want any dynamic tests to be cleared grouped rather than mixed among static tests which should making it less cumbersome to disable e.g. because you have a dedicated file or describe block that you can disable the rule for entirely.

If we pretend this was a brand new rule to not be blocked on breaking changes, I wonder how this would look (frankly, if we would have this option at all): maybe instead it should be something like noComplexTitles which checks for things like regexps, math expressions, and template literals that have a ton of interpolations?

[JoshuaKGoldberg]

generally my view is that it's about making titles easy to predict

Interesting! That's now how I read the rule. I read it as a pure validity rule: i.e. acting as a subset of a type-checker to make sure you don't accidentally pass a [] or some other silly value in there.

Maybe there are two areas of rule responsibility in play? One around validity, and one around complexity?

[G-Rath]

Interesting! That's now how I read the rule.

An important potential typo - I assume you mean "not" given your next sentence 😝

acting as a subset of a type-checker to make sure you don't accidentally pass a [] or some other silly value in there.

but isn't that the role of the type checker...? Granted this rule / option predates I think @typescript-eslint entirely but that makes this discussion even more relevant I think.

Here are the types for the two functions:

interface Describe {
  // eslint-disable-next-line @typescript-eslint/no-unsafe-function-type
  (name: number | string | Function | FunctionLike, fn: EmptyFunction): void;

  // ...
}

interface It {
  (name: string, fn?: ProvidesCallback, timeout?: number): void;

  // ...
}

So they already prevent a lot of silly values (when type checking is available) - what values can you still pass in that would classed as silly and worth a lint rule catching?

[JoshuaKGoldberg]

An important potential typo - I assume you mean "not" given your next sentence 😝

Haha, yes, thank you.

but isn't that the role of the type checker...?

Agreed! I'd say the "validity" part of the rule could probably just be disabled. Maybe there could be a TS-oriented preset for users, like a jest.configs.typed kind of deal?

[G-Rath]

I don't feel it's worth a whole new config for this given I don't think we have anything else that'd fit, so it would be the same work downstream to disable this particular option for the rule vs use the config.

But it sounds like we agree it's probably best to change the rule - I'm really leaning towards just removing this part entirely because the only two invalid things I can think of that we'd be able to catch is regexp literals and anonymous functions, both of which feel really unlikely for people to use by mistake.

If there was demand, we could then introduce a rule like prefer-simple-title which would require you pass in literals and similar stuff