-
Notifications
You must be signed in to change notification settings - Fork 325
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
Add tests for the govuk-font
mixin
#2474
Conversation
|
||
const results = await renderSass({ data: sass, ...sassConfig }) | ||
|
||
expect(results.css.toString().trim()).toBe(outdent` |
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.
This is basically a 'snapshot test', but I couldn't think of a better way of doing it… this does mean that whenever anything changes in the govuk-font mixin we'll need to update this test accordingly. It's not ideal, but I think that's OK?
expect(results.css.toString()).toContain('font-size: 12px') | ||
expect(results.css.toString()).not.toContain('font-size: 14px') |
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.
This feels a little clunky, but because $size: 14
also outputs font-size: 12px
(for mobile) it'd pass anyway… suggestions welcome.
expect(css).toContain('font-feature-settings: "tnum" 1;') | ||
expect(css).toContain(outdent` | ||
${outdent} | ||
@supports (font-variant-numeric: tabular-nums) { | ||
.foo { | ||
font-feature-settings: normal; | ||
font-variant-numeric: tabular-nums; } }`) |
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.
Thought it was worth testing for both of these, but not worth writing individual tests for them.
expect(results.css.toString()).toContain('font-weight: 700') | ||
}) | ||
|
||
it('does not output font-weight if $weight: false', async () => { |
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.
@36degrees do you think it's worth having a test for an unrecognised $weight?
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.
Hmm, good question… at the minute it'll basically ignore $weight if it's not
regularor
bold. What would the assertion be for that test? Just check that the output does not contain
font-weight`?
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 think so, yep. If that's the behaviour we want to keep!
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.
86eee67
to
5e72b3f
Compare
I noticed when looking into #2456 that we had no test coverage for the
govuk-font
mixin. Thegovuk-font
mixin does quite a lot of things – most of it handing off to the other mixins in this file – but does have responsibility for setting up things like tabular numbers.Add tests to try and cover all of the execution paths within the mixin.