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

Fix incorrect font-family-no-missing-generic-family-keyword configuration #3038 #3039

Merged
merged 2 commits into from
Dec 2, 2017

Conversation

hudochenkov
Copy link
Member

Which issue, if any, is this issue related to?

#3038

Is there anything in the PR that needs further explanation?

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 first accept test, add severity secondary option, and run test. There is a little warning: most test cases use config: ["value"], while very few use config: [["value"]] (which is probably should be fixed). I can't think straight anymore today, so I'm not including this change for jest-setup.js in this PR.

Copy link
Member

@jeddy3 jeddy3 left a 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(
Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Contributor

@CAYdenberg CAYdenberg Dec 1, 2017

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 CAYdenberg merged commit f785cda into master Dec 2, 2017
@hudochenkov
Copy link
Member Author

@CAYdenberg when you merge PR, please, delete branch and update changelog.

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

Successfully merging this pull request may close these issues.

None yet

4 participants