Replacing Mocha with Jest #640
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
govuk-design-system-ci
temporarily deployed
to
govuk-prototype-kit-pr-640
aliuk2012
force-pushed
the
add-jest-tests
branch
from
1e6d8c3
to
d35733a
Compare
govuk-design-system-ci
temporarily deployed
to
govuk-prototype-kit-pr-640
kr8n3r
reviewed
Nov 19, 2018
govuk-design-system-ci
temporarily deployed
to
govuk-prototype-kit-pr-640
36degrees
reviewed
Nov 19, 2018
There was a problem hiding this comment.
I believe
govuk-prototype-kit/.travis.yml
Lines 6 to 7 in 48ad1c3
__tests__/spec/sanity-checks.js
Outdated
|
|
||
| test('should send with a well formed response for the index page', async () => { | ||
| const response = await request(app).get('/') | ||
| expect(response.statusCode).toBe(200) |
There was a problem hiding this comment.
Can we aim for one expectation per test?
There was a problem hiding this comment.
Good shout, this has now been addressed 👍
NickColley
approved these changes
Nov 19, 2018
govuk-design-system-ci
temporarily deployed
to
govuk-prototype-kit-pr-640
hannalaakso
force-pushed
the
add-jest-tests
branch
from
73b34ca
to
584c511
Compare
govuk-design-system-ci
temporarily deployed
to
govuk-prototype-kit-pr-640
aliuk2012
force-pushed
the
add-jest-tests
branch
from
584c511
to
b5fbb5c
Compare
govuk-design-system-ci
temporarily deployed
to
govuk-prototype-kit-pr-640
govuk-design-system-ci
temporarily deployed
to
govuk-prototype-kit-pr-640
After running jest-codemon
I was able to get rid of the gulp task and just call it straight from `npm test`.
This listener which looks out for file changes using BrowserSync doesn't need to be part of `server.js` as it's only needed for npm start. We added a new file called `listen-on-port.js` - this needs to be separate from `server.js` for Jest to work correctly. More detail here: http://www.albertgao.xyz/2017/05/24/how-to-test-expressjs-with-jest-and-supertest/#2-Separate-your-app-and-sever
hannalaakso
force-pushed
the
add-jest-tests
branch
from
523ca90
to
0ccbc4a
Compare
govuk-design-system-ci
temporarily deployed
to
govuk-prototype-kit-pr-640
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Co-authored with @hannalaakso
We wanted to ensure that testing frameworks were consistent between the various repos that we supported so this removes Mocha and moved the tests over to Jest.
We experienced a lot of issues replacing mocha because server.js had a lot of complicated code and it wasn’t clear what was happening. This commit starts to address this issue by splitting the server and the app into separate files (server.js and app.js) As mentioned in this blog http://www.albertgao.xyz/2017/05/24/how-to-test-expressjs-with-jest-and-supertest/. There is no need to add listeners to the app for testing and seemed to be a best practice.
Another issue we had, At one point the tests were passing but Jest was not exiting and instead threw this error and kept running until we manually killed the process.
The issue here was that we had nunjucks (middleware) configured incorrectly. It was setup to continuously watch for file changes in the view (even in production). We resolved this by adding some logic which dynamically created an object for nunchucks configuration. This object conditionally set the watch flag to true depending on whether it was running in development.
While cleaning up mocha we were able to get rid of the gulp task and just call it straight from
npm test.Edit: We added a new file called
listen-on-port.jswhich adds a listener for the server - this needs to be separate fromserver.jsfor Jest to work correctly. We now haveserver.js,start.jsandlisten-on-port.js- we can improve the naming once we do more major refactoring of the code, we didn't want to rename existing files as this could be considered a breaking change.Note: it's probably helpful to review this commit by commit.
To do:
https://trello.com/c/OYcXcTFd/1559-rewrite-existing-smoke-tests-using-jest-pair