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

Adding support for Jest asymmetric matchers and more #148

Merged
merged 2 commits into from
Jun 24, 2018
Merged

Adding support for Jest asymmetric matchers and more #148

merged 2 commits into from
Jun 24, 2018

Conversation

santino
Copy link
Contributor

@santino santino commented Jun 21, 2018

This PR aims to address features requested in #146 for .toHaveStyleRule matcher.

Changelog:

  1. Added support to match against Jest asymmetric matchers
  2. Added support to match against undefined rules
  3. Added support for RegEx matching to React Native
  4. Shared matching function across React and React Native
  5. Introduced a "no style rules" found message that prints options object when received (React)
  6. Introduced consistent "property not found" and "property mismatch" messages across React and React Native
  7. Fixed all React Native skipped tests plus broken "message when value does not match" test

Tests for both React and React Native have been added to cover the new functionalities introduced.

Notes about the changes introduced:

  1. Jest asymmetric matchers are powerful and useful when testing conditional styling based on props.
    A rule like this border: 0.05em solid ${({transparent}) => transparent ? 'transparent' : 'black'};
 can now be tested in isolation when passing the transparent prop like so expect(<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.
  2. Testing undefined is also useful when checking conditional styling based on props. For example this rule opacity: ${({ disabled }) => disabled && '.65'}; can now be tested like this expect(<Component />).toHaveStyleRule('opacity', undefined) when disabled prop is not passed; when passing the prop this can be simply tested with expect(<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 case disabled).
    Thanks to the implementation of the above change #1 testing for an undefined rule can now also be achieved with Jest asymmetric matchers like so expect(component).not.toHaveStyleRule('opacity', expect.anything()) however this requires, again, a negative assertion and is definitely less intuitive and more articulated.
  3. React Native was validating the 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/)
  4. Matching in React Native was performed with received === expected whilst React also had in addition expected.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's expect.stringMatching(expected) which has built in support for both Strings and RegEx values.
  5. When an empty set of rules was retrieved a "property not found" message was returned to the user. This has now been replaced with a dedicated "no rules found" message to provide additional clue that the problem is not with a specific property but with the entire styles supposedly applied to the component.
    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.
  6. A new function, 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.
  7. A number of tests in React Native were skipped using xtest. The main issue with these tests is that they were checking on properties such as background but React Native rules are identified individually. For example when applying a rule like border: 1px solid black this is translated into the following three different rules border-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 passing null 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 of Expected: background-color: red Received: background-color: orange

…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 */
Copy link
Member

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
}

Copy link
Contributor Author

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.

@MicheleBertoli
Copy link
Member

WOW, @santino, the quality of this PR is impressive.
Thank you very much for adding the new functionality, taking care of all the related changes (e.g. React Native), and cover everything with tests.

I'm more than happy to merge this, I have only one nit so that we can remove the eslint-disable comment. Would you mind updating the code?

@santino
Copy link
Contributor Author

santino commented Jun 24, 2018

Thanks for your positive feedback on my PR.

I can update the code; the problem in this case highlights a clash between prettier and eslint, as prettier fix adds indentation to prettify the ternary but then eslint requires to remove the extra indentation.
As I explained in my reply to your comment on the code I think that ES6 arrow function benefit to have implicit return is a winner against opening a block and so is the ternary approach against the "if statement".
However your suggestion is definitely valid and possibly more declarative.

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.

@MicheleBertoli
Copy link
Member

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.
Anyway, the fact that prettier clashes with eslint makes me think we could use a better approach, and I'll investigate it soon.

The eslint-disable was added in a "rush" when a newly released version of SC broke this library, and we never fixed it (classic!).

Last word is yours obviously

This is not true :)
I implemented the initial concept and I'm now maintaining the repo, but every contributor has a word and since you felt this was the right thing to do, I'm going to me merge it as it is.

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.

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

Successfully merging this pull request may close these issues.

2 participants