prefer-to-have-value having unintended effects

[juzerzarif]

• eslint-plugin-jest-dom version: 3.5.0
• node version: 10.22.0
• npm version: 6.14.6

Relevant code or config

const gen = someGeneratorFunction();
expect(gen.next().value).toEqual(someYieldedValue);

What you did: ^

What happened: Prefer .toHaveValue() over other attribute checks

Problem description:
The new rule prefer-to-have-value is causing unintended warnings when checking the value key on objects that are not DOM elements.

Suggested solution:
I'm not sure if there's a graceful solution that can allow for triggering warnings on element.value but not on obj.value (let me know if I'm wrong!). But I think a reasonable middle ground would be to allow a configuration option to only trigger a warning on expect(element).toHaveAttribute('value', 'someValue') so I don't have to individually turn off the rule everywhere I'm testing generators.

[benmonro]

@juzerzarif thank you for reporting this. that was quick, just published that rule last night. :). we have some options we can look at. one is to use suggest instead of a fixer for objects like that. would love to welcome a PR if you want to take a stab at it.

[juzerzarif]

@benmonro That sounds like a good idea, what do you think of providing an option to pick between the rule providing a suggestion, fix, or just being off for obj.value since a suggestion would still cause a linting error. I can look into an implementation after work today

[benmonro]

@juzerzarif that might work, another thought would be to check the assignment/value of the argument to make sure it's coming from a dom node. we did this in prefer-in-document: https://github.com/testing-library/eslint-plugin-jest-dom/blob/master/src/rules/prefer-in-document.js#L104-L126

perhaps we could just extract that out and use that?

[juzerzarif]

@benmonro That would be the best of both worlds, sounds like the way to go! I can mess around with it and see about a PR this weekend

[benmonro]

@juzerzarif another use case just occurred to me. this is totally optional, but would be wonderful if you want to implement it.

basically, toHaveValue should really only be used with <input>, <select> and <textarea> elements, so basically my thought is this (in pseudo code)

context.report({...

fix(fixer) {
  if(query == `getByRole("textbox") || query == `getByRole("dropdown")`) 
     return [...all the fixes]
}

[joeycozza]

Did you mean to mention me on this? or was it just a misclick?

[benmonro]

doh, yes it was a mis-click sorry

[juzerzarif]

@benmonro Are you saying it should still report an error if it is a different query than a byRole('textbox') or a byRole('dropdown') but just not provide an autofix?

[benmonro]

Yeah that's what I'm thinking

[juzerzarif]

But since we're only reporting an error when the object being queried came from a testing library query, wouldn't the fix that the user would have to implement be pretty much the same as the fix that the autofix would provide? Like if you had queries similar to the ones in the docs https://github.com/testing-library/jest-dom#tohavevalue, wouldn't we want to autofix those too?

[benmonro]

well, the problem is that the answer to your question is "maybe." it kinda depends on the element. You could do a suggest for those, but I think the fixer should only change code we can be certain won't have bad consequences. hopefully i'm making sense. :)

[juzerzarif]

Ah, yeah that makes sense to me. I think a suggestion for the other cases is a good alternative

[github-actions]

🎉 This issue has been resolved in version 3.6.3 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀