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 Jest setup/teardown for test server awaiting SIGTERM #2880

Merged
merged 3 commits into from
Sep 27, 2022

Conversation

colinrotherham
Copy link
Contributor

@colinrotherham colinrotherham commented Sep 26, 2022

This PR fixes an issue with Jest in watch mode:

npx jest --watch src/govuk/components/button/button.test.js

When watching JavaScript component tests project files the array returned via getServers() from jest-dev-server appears to list processes that have terminated (or are currently awaiting termination).

We should check for signalCode: 'SIGTERM' processes before assuming the test site is running

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2880 September 26, 2022 15:58 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2880 September 26, 2022 17:12 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2880 September 26, 2022 18:57 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2880 September 26, 2022 21:11 Inactive
Copy link
Contributor Author

@colinrotherham colinrotherham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Morning @romaricpascal this PR is now ready for review again

@@ -1,6 +1,6 @@
module.exports = {
browserContext: 'incognito',
dumpio: true,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've taken this out as it's supposed to be in launch so wasn't working. Can be useful for debugging but adds a lot of noise

// Server already running? Wait for exit
if (servers.some(({ signalCode }) => signalCode === 'SIGTERM')) {
await serverStop()
timeout = 0
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need to wait for the port to open when we know it's just closed

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's probably worth putting a reminder in the code for the future

Suggested change
timeout = 0
// We don't need to wait for the port to open when we know it's just closed
timeout = 0

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've put the comments together like this:

await serverStop() // Wait for server to stop
timeout = 0 // No need to wait for start

await serverStart() // Wait for web server
await setup() // Open browser
await setup(jestConfig) // Open browser
Copy link
Contributor Author

@colinrotherham colinrotherham Sep 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This enables Jest to open multiple Chrome pages, one per worker

@@ -59,8 +59,8 @@ describe('/components/button', () => {
*
* Examples don't do this and we need it to have something to submit
*/
async function trackClicks () {
page.evaluate(() => {
function trackClicks (page) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With (potentially) multiple pages or tabs open at once, we need to pass in which one we use

Copy link
Member

@romaricpascal romaricpascal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds great that both the watch and test stepping on each other's tabs will be fixed by that 😄

// Server already running? Wait for exit
if (servers.some(({ signalCode }) => signalCode === 'SIGTERM')) {
await serverStop()
timeout = 0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's probably worth putting a reminder in the code for the future

Suggested change
timeout = 0
// We don't need to wait for the port to open when we know it's just closed
timeout = 0

jest-puppeteer.config.js Show resolved Hide resolved
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2880 September 27, 2022 09:07 Inactive
@colinrotherham colinrotherham merged commit f6b8e8a into main Sep 27, 2022
@colinrotherham colinrotherham deleted the fix-jest-watch branch September 27, 2022 11:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

Successfully merging this pull request may close these issues.

3 participants