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

Proposal for handling properties/css rules with no value #205

Closed
wants to merge 1 commit into from

Conversation

dumbNickname
Copy link

Make it possible to use not.toHaveStyleRule when rule defined for higher resolutions.

…se not.toHaveStyleRule when rule defined for higher resolutions.
width: 50%;
}
`
notToHaveStyleRule(<Button />, 'width')
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MicheleBertoli I mentioned those changes in our previous discussion, as promised I deliver those as a separate PR. Changes were made due to the 'notToHaveStyleRule' check marked with this comment. It is a quick fix and probably there is a better way to do it. If you share any ideas I can try to fix in better way. In my old test suits such tests worked well and a scenario when you check lack of some rule seems to be useful when doing mobile first design. Anyway, I would like to get your opinion on it.

@MicheleBertoli
Copy link
Member

Thank you very much @dumbNickname for opening a separate PR.

In #148 we implemented the current behaviour, and the reason was to be able to test this:
opacity: ${({ disabled }) => disabled && '.65'};
In the following way:
expect(<Component />).toHaveStyleRule('opacity', undefined)

If you want to negate the expectation, you can use the following statement:
expect(<Button />).not.toHaveStyleRule('width', expect.anything())
(which I agree it's not super intuitive)

However, I would love to hear more about your use-case, and see why you can't do this:
expect(<Button />).toHaveStyleRule('width', undefined)
(which I believe it's more explicit)

I would also love @santino (the author of the original PR) to chime in here.

@santino
Copy link
Contributor

santino commented Nov 4, 2018

@dumbNickname if I understand correctly you are proposing to handle something like this:
expect(<Component />).not.toHaveStyleRule('opacity')

Although I'm not sure on the use case since the same result is achievable with expect(<Component />).toHaveStyleRule('opacity', undefined), as already reminded by @MicheleBertoli; I do understand that given the name of toHaveStyleRule it sounds tempting/natural to prepend the .not modifier to test against a not defined rule without necessarily specifying a value.

However releasing your proposal would mean releasing a breaking change (because of the removed undefined matcher) which would then require a major bump and migration steps for existing users, so I'm afraid I can't agree with this approach.

In addition to that the signature of toHaveStyleRule expects a property and a value and so being able to explicitly pass undefined as a value is still beneficial and works naturally with the serialiser which will indeed return undefined for “undefined” rules, so I wouldn't recommend removing the undefined matcher.

Having said that I thought about what you want to achieve and if you look into how matcherTest util currently works this is essentially a scenario already catered for.
In fact given this test expect(<Component />).not.toHaveStyleRule('opacity') (where Component doesn't have any opacity rule) matcherTest would already evaluate the following expect(undefined).toEqual(undefined) which correctly returns true.

Right now the test simply fails because in this case we're using a negated .not modifier which means we need to flip our assertion as per jest docs.

I have now opened #206 to introduce the behaviour you're after without any breaking change.

Hope this help.

@dumbNickname
Copy link
Author

dumbNickname commented Nov 4, 2018

@santino Thanks for your detailed answer. As I wrote it was a quick fix and I expected that there has to be a better solution. I made this PR just to show the problem, did not really expect that this will be the final solution - as you wrote it would cause a breaking change.

I have never reached the part of jest docs that you have mentioned, great to know that! I think this was the missing part for me, next time will try to prepare myself better ;). I could not wrap my head around how to change the matcher behavior to support not modifier.

Just a short comment on .toHaveStyleRule(‘opacity’, undefined) vs .not modifier - imho tests that can be expressed with .not modifier look far better, it feels natural to use, expectation is nice to read and follows general jest matchers approach etc. Here thanks again for implementing it instead of just sharing your comments

I took a look at your solution - looks exactly as what I needed. When it gets merged I will double check if it covers all of my project cases.

@MicheleBertoli Please reject my PR ;)

Thank you both for your help.

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.

None yet

3 participants