fix(rules): only report errors for prefer-to-have-value on valid DTL queries #122
Conversation
juzerzarif
force-pushed
the
fix-117/toHaveValue
branch
from
ea9b886 to
5676c99
Compare
Codecov Report
@@ Coverage Diff @@
## master #122 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 14 15 +1
Lines 337 358 +21
Branches 52 59 +7
=========================================
+ Hits 337 358 +21
Continue to review full report at Codecov.
|
src/rules/prefer-to-have-value.js
Outdated
| return [ | ||
| fixer.removeRange([ | ||
| node.callee.object.arguments[0].object.range[1], | ||
| node.callee.object.arguments[0].property.range[0] - 1, |
There was a problem hiding this comment.
what is the -1 for? generally hardcoded values in fixers can be problematic, unless you're inserting an arg (which i'd love to find a better way).
There was a problem hiding this comment.
If the object is wrapped in parentheses (like an inline await statement expect((await screen.findByRole('textbox')).value)), removing from the end of its range to the end of the property's range also gets rid of the closing wrapping parenthesis expect((await screen.findByRole('textbox')). It does feel kinda icky to use magical numbers but I couldn't find another way to get around that than to just assume that the . preceding value will be just one over in the vast majority of cases. Definitely open to other suggestions though.
There was a problem hiding this comment.
@juzerzarif, i did some digging and i think I found a winner! you can use context.getSourceCode().getTokenBefore(node) or getTokenAfter(). I discovered this by changing .value to be . value which broke both the test for this use case as well as the ones i was using in to-have-class. By switching to these functions they're now completely immune to spacing issues. :) I hope you don't mind I just pushed it to your branch along w/ some other changes just extracting some variables out for readability.
|
|
||
| const errors = [{ messageId: "use-to-have-value" }]; | ||
| ruleTester.run("prefer-empty", rule, { |
There was a problem hiding this comment.
wow thanks, that's what I get for copy pasta. :)
| valid: [ | ||
| `expect(element).toHaveValue('foo')`, | ||
| `expect(element.value).toBeGreaterThan(2);`, | ||
| `expect(element.value).toBeLessThan(2);`, | ||
|
|
||
| `const element = document.getElementById('asdfasf'); |
There was a problem hiding this comment.
for now this is fine, but it would be nice to at some point support document queries as well as RTL. :)
There was a problem hiding this comment.
Yeah, my initial idea was to create a list of all the DOM api query functions but then it occurred to me that you can also get a reference to a dom element via a lot of other keys like first/lastElementChild, children etc. so I figured probably best to leave that to be implemented as its own piece
src/utils.js
Outdated
| : expression.type === "AwaitExpression" | ||
| ? getInnerNodeFrom(expression.argument) | ||
| : expression; | ||
| } |
There was a problem hiding this comment.
this seems different than from what was in prefer-in-document:
function getAssignmentFrom(expression) {
return expression.type === "TSAsExpression"
? getAssignmentFrom(expression.expression)
: expression.type === "AwaitExpression"
? getAssignmentFrom(expression.argument)
: expression.callee
? expression.callee
: expression;
}
is expression.callee no longer needed?
There was a problem hiding this comment.
I had to make it return a slightly broader object because I needed access to the whole CallExpression in prefer-to-have-values to get access to its arguments as well as the callee. I've tweaked its consumption in prefer-in-document to maintain passivity here e77f6b9#diff-6eab9c599f2637bd770a8d963e5c787cfd559888fe0c7b71fbab69d5d2bfff4bR160
src/utils.js
Outdated
| @@ -0,0 +1,46 @@ | |||
| /** | |||
There was a problem hiding this comment.
i'm a bit of a stickler, but utils has always bothered me. :) Can we rename this to assignment-ast.js to denote that its purpose is for finding assignment values. :)
There was a problem hiding this comment.
Makes sense to me, renamed here 94aad94
|
Nice work @juzerzarif, a couple minor things then I think we're good to merge. @all-contributors please add @juzerzarif for code, tests, bugs. |
|
I've put up a pull request to add @juzerzarif! 🎉 |
benmonro
changed the title
|
🎉 This PR is included in version 3.6.3 🎉 The release is available on: Your semantic-release bot 📦🚀 |
What:
prefer-to-have-valuesonly reports an error forexpect(<something>.value).toEqual('foo')pattern if<something>is the result of a valid DTL query. Only auto fix forbyRole('textbox')andbyRole('dropdown')queries, suggest fix for rest.Why:
So
prefer-to-have-valuesdoesn't report an error when the value property of non DOM element objects are queried in an assertion. Auto fix update to only auto fix for recommended queries.Fixes #117
How:
Extracted existing logic for determining whether the value being queried in the assertion is a DOM element's value from the
prefer-in-documentrule.Checklist: