Replace Mocha/Chai with Jest, re-enable task tests, add back-link component #455
Conversation
Since the task tests are meant to be run only as part of the build steps, this required some reconfiguring of the build scripts. This will allow us to run jest conditionally after builds as longside tests that run in master in CI.
govuk-design-system-ci
temporarily deployed
to
govuk-frontend-review-pr-455
govuk-design-system-ci
temporarily deployed
to
govuk-frontend-review-pr-455
NickColley
changed the title
govuk-design-system-ci
temporarily deployed
to
govuk-frontend-review-pr-455
govuk-design-system-ci
temporarily deployed
to
govuk-frontend-review-pr-455
lib/jest-helpers.js
Outdated
| lstripBlocks: true | ||
| }) | ||
|
|
||
| function render (name, params) { |
There was a problem hiding this comment.
Would be great to add some comments in this file. For example, from the point of view of someone new to JS testing who's writing their first test for something they're contributing, I think it'd be useful to understand what Cheerio is and what it is doing, and how you use the return value when it returns two things { output, $ }.
I'd also suggest maybe using componentName and arguments as the parameters. I think we're starting to standardise on arguments for macros, but we should maybe document our terminology somewhere.
There was a problem hiding this comment.
componentName sounds good but isn't params what we're using everywhere? https://github.com/alphagov/govuk-frontend/search?utf8=%E2%9C%93&q=params&type=
There was a problem hiding this comment.
…yep.
Good point, well made.
lib/jest-helpers.js
Outdated
| return { output, $ } | ||
| } | ||
|
|
||
| const yaml = require('js-yaml') |
There was a problem hiding this comment.
Is this conventional? I'd expect to see all includes at the top of the file.
There was a problem hiding this comment.
I was grouping with where they're used, agree lets move these 👍
lib/jest-helpers.js
Outdated
| const fs = require('fs') | ||
|
|
||
| function getExamples (name) { | ||
| const file = fs.readFileSync( |
There was a problem hiding this comment.
What happens if you try to get examples for a non-existent component? Is the error message clear? If not, can we add error handling to this function.
There was a problem hiding this comment.
● Test suite failed to run
ENOENT: no such file or directory, open 'src/components/back-links/back-links.yaml'
Seems pretty clear to me, let me know what you think.
lib/jest-helpers.js
Outdated
| }) | ||
|
|
||
| function render (name, params) { | ||
| const output = nunjucks.render(name + '/template.njk', { params }) |
There was a problem hiding this comment.
What happens if you try to render a non-existent component? Is the error message clear? If not, can we add error handling to this function.
There was a problem hiding this comment.
Will check this, good shout.
There was a problem hiding this comment.
FAIL src/components/back-link/template.test.js
back-link component
✕ renders the default with an anchor, href and text correctly (5ms)
✓ renders classes correctly (12ms)
✓ renders custom text correctly (3ms)
✓ renders html correctly (1ms)
✓ renders attributes correctly (3ms)
○ skipped 1 test
● back-link component › renders the default with an anchor, href and text correctly
template not found: back-links/template.njk
| }) | ||
|
|
||
| const $component = $('.govuk-c-back-link') | ||
| expect($component.text()).toEqual('Home') |
There was a problem hiding this comment.
Can we have a test that ensures HTML is correctly escaped when passed as text?
| }).toThrow() | ||
| }) | ||
|
|
||
| it('renders the default with an anchor, href and text correctly', () => { |
There was a problem hiding this comment.
the default what? "renders the default example"?
|
|
||
| const { render, getExamples } = require('../../../lib/jest-helpers') | ||
|
|
||
| const COMPONENT_NAME = 'back-link' |
There was a problem hiding this comment.
Not 100% sure this is worth the comprehension cost - seems like an extra connection for contributors to make?
There was a problem hiding this comment.
Worth noting, we also specify the class name in every test and it feels like if we continue down this path we would make that a constant too…?
e.g.
const { $ } = render(COMPONENT_NAME, examples.default)
const $component = $(COMPONENT_CLASS)
which I think could be a bit intimidating for new contributors.
There was a problem hiding this comment.
Agree let's remove this constant
govuk-design-system-ci
temporarily deployed
to
govuk-frontend-review-pr-455
See jestjs/jest#1456 for discussion around this problem
govuk-design-system-ci
temporarily deployed
to
govuk-frontend-review-pr-455
| const cheerio = require('cheerio') | ||
| const yaml = require('js-yaml') | ||
|
|
||
| const COMPONENT_DIR = 'src/components' |
There was a problem hiding this comment.
Missed this one, would've been good to use config/paths.json so we have all the paths in one place.
|
|
||
| const $component = $('.govuk-c-back-link') | ||
| expect($component.attr('data-test')).toEqual('attribute') | ||
| expect($component.attr('aria-label')).toEqual('Back to home') |
There was a problem hiding this comment.
If there is a way to get the entire list of attributes, you could combine that with a jest snapshot to very easily test the whole list without writing too many bulky test assertions. :)
This is very cool! 🌟
There was a problem hiding this comment.
At the moment the logic is pretty basic so I think it's covered by these assertions, definitely sounds like a good idea otherwise though.
This pull request:
Note: this approach avoids gulp to chain the commands, this is to avoid unnecessary abstraction which might be controversial main con is npm scripts is verbose. Comments welcome.
With special thanks to @htmlandbacon and @tyom for sharing their approaches.
As part of https://trello.com/c/UHYdyb4w/628-add-jest-testing-suite-to-govuk-frontend