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

4 participants
@hudochenkov
Member

hudochenkov commented Nov 27, 2017

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.

@jeddy3

jeddy3 approved these changes Nov 27, 2017

LGTM

});
expect(result.warn.mock.calls[0]).toHaveLength(2);
expect(result.warn.mock.calls[0][0]).toEqual(

This comment has been minimized.

@CAYdenberg

CAYdenberg Nov 30, 2017

Contributor

Can you use toHaveBeenCalledTimes and toHaveBeenCalledWith? Just easier to read for someone unfamiliar with Jest.

This comment has been minimized.

@hudochenkov

hudochenkov Nov 30, 2017

Member

I used same matchers as other tests in this file for consistency.

This comment has been minimized.

@CAYdenberg

CAYdenberg Dec 1, 2017

Contributor

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

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.002%) to 95.674%
Details
@hudochenkov

This comment has been minimized.

Member

hudochenkov commented Dec 2, 2017

@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