-
-
Notifications
You must be signed in to change notification settings - Fork 145
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
Adding support for Jest asymmetric matchers and more #148
Conversation
…efined' also implementing consistent messages across react and react native as well as fixing skipped and broken react native tests
@@ -92,41 +97,31 @@ const getDeclaration = (rule, property) => | |||
const getDeclarations = (rules, property) => | |||
rules.map(rule => getDeclaration(rule, property)).filter(Boolean) | |||
|
|||
const normalizeModifierOption = modifier => | |||
Array.isArray(modifier) ? modifier.join('') : modifier | |||
/* eslint-disable prettier/prettier */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit
const normalizeOptions = ({ modifier, ...options }) => {
if (modifier) {
return {
...options,
modifier: Array.isArray(modifier) ? modifier.join('') : modifier,
}
}
return options
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy to implement following your suggestion, however I usually try to avoid "if statements" when they're not strictly required and in this case I did definitely prefer a simple ternary which also allowed to take advantage of ES6 arrow function implicit return as opposed to open a block.
WOW, @santino, the quality of this PR is impressive. I'm more than happy to merge this, I have only one nit so that we can remove the |
Thanks for your positive feedback on my PR. I can update the code; the problem in this case highlights a clash between In this case I just disabled the prettier rule which I thought it could be a decent compromise to more modern and concise code since prettier after all doesn't deal with any inconsistency that could possibly compromise code functionality (as eslint does) but is only there to enforce human readability. FIY I found an example here where actually even all eslint rules are disabled which encouraged me to go ahead disabling just the prettier rule within my changes. Last word is yours obviously, I'll implement the change straight away if you prefer to go with it. |
Thank you very much for providing more information, @santino. In general, I would 100% agree with your point of view on ternaries, while in this case, I felt more inclined to avoid the comment. The
This is not true :) One more time, thank you very much for making such a big improvement and props for the attention to details you put into this PR, I'm looking forward to collaborating again. |
This PR aims to address features requested in #146 for
.toHaveStyleRule
matcher.Changelog:
undefined
rulesNotes about the changes introduced:
A rule like this
border: 0.05em solid ${({transparent}) => transparent ? 'transparent' : 'black'};
can now be tested in isolation when passing thetransparent
prop like soexpect(<Component transparent />).toHaveStyleRule('border', expect.stringContaining('transparent'))
without testing the entire rule (which consist of border-width, border-style and border-color) nor the need to use a RegEx but rather taking advantage of known solutions provided by Jest matchers.undefined
is also useful when checking conditional styling based on props. For example this ruleopacity: ${({ disabled }) => disabled && '.65'};
can now be tested like thisexpect(<Component />).toHaveStyleRule('opacity', undefined)
whendisabled
prop is not passed; when passing the prop this can be simply tested withexpect(<Component disabled />).toHaveStyleRule('opacity', '.65)
.This sort of test was not possible before, something similar could have only be achieved with this code
expect(<Component disabled />).not.toHaveStyleRule('opacity', '.65)
which is error prone because it relies on a specific value for the rule, if a developer updates the value by mistake this "negative" assertion would always pass regardless of the presence of the conditional prop (in this casedisabled
).Thanks to the implementation of the above change
#1
testing for an undefined rule can now also be achieved with Jest asymmetric matchers like soexpect(component).not.toHaveStyleRule('opacity', expect.anything())
however this requires, again, a negative assertion and is definitely less intuitive and more articulated.received
vs.expected
value only with a strict equality comparison (===
) allowing, therefore, only a string comparison.Now since a new shared matching function has been introduced for both React and React Native (as per change
#4
) this will also support matching against a RegEx (e.g./^exp/
)received === expected
whilst React also had in additionexpected.test(received)
to support RegEx.These matching functions have been replaced with Jest matchers to remove custom implementation and to allow support for Jest asymmetric matchers and
undefined
as detailed in notes for changes#1
and#2
. The above to rules present in the React implementation are simply replaced by Jest'sexpect.stringMatching(expected)
which has built in support for both Strings and RegEx values.In addition to that this message will print the third argument,
options
, (when passed) to help the user identify possible issues with missing rules given a specific modifier or media applied.buildReturnMessage
, is defined in the utils to provide a message for "property not found" and "property mismatch" which are the two messages required by both React and React Native; to allow consistent messaging across the two implementations.xtest
. The main issue with these tests is that they were checking on properties such asbackground
but React Native rules are identified individually. For example when applying a rule likeborder: 1px solid black
this is translated into the following three different rulesborder-width: 1px
,border-style: solid
,border-color: black
; hence these rules must be tested individually.The "null" test in React Native (which was also skipped) has been removed because is not relevant since in React Native the passed component is an object with an expected
props
object property; hence passingnull
instead of an object component is not relevant.Similarly the "message when property not found" test has been fixed replacing the passed
null
with a valid styled React Native component.Finally the "message when value does not match" test has been fixed since the wrong assertion caused the snapshot to be based on the wrong message
background isn't in the style rules
instead ofExpected: background-color: red Received: background-color: orange