-
-
Notifications
You must be signed in to change notification settings - Fork 929
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
Fix incorrect font-family-no-missing-generic-family-keyword configuration #3038 #3039
Conversation
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.
LGTM
}); | ||
|
||
expect(result.warn.mock.calls[0]).toHaveLength(2); | ||
expect(result.warn.mock.calls[0][0]).toEqual( |
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.
Can you use toHaveBeenCalledTimes
and toHaveBeenCalledWith
? Just easier to read for someone unfamiliar with Jest.
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 used same matchers as other tests in this file for consistency.
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.
Weird, I thought I changed all of those when we removed Sinon. Oh well, point taken.
@CAYdenberg when you merge PR, please, delete branch and update changelog. |
#3038
I've added a test to
validateOptions
, but it doesn't catch an error. It will only show an error if test with configuration like in #3038 run.I think we can add a new type of test in
jest-setup.js
. Take firstaccept
test, addseverity
secondary option, and run test. There is a little warning: most test cases useconfig: ["value"]
, while very few useconfig: [["value"]]
(which is probably should be fixed). I can't think straight anymore today, so I'm not including this change forjest-setup.js
in this PR.