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

Ensure tests fail for browser exceptions #2933

Merged
merged 4 commits into from
Oct 24, 2022
Merged

Conversation

colinrotherham
Copy link
Contributor

@colinrotherham colinrotherham commented Oct 20, 2022

This PR ensures all browser tests (Puppeteer, jsdom) fail for unhandled browser exceptions

It does a few separate things:

  1. Adds Jest custom environments for Puppeteer, jsdom
  2. Adds process.emit('error', error) to ensure browser exceptions fail tests
  3. Prevents Jest Puppeteer incorrectly emitting uncaughtException
  4. Sets the unit test default environment to jsdom (not Node.js)

Update: The missing piece of the puzzle was (strangely) removing the error.stack property before calling process.emit(). This ensures the error is logged in our test output, rather than blank lines

Possibly related to jestjs/jest#6281

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2933 October 20, 2022 14:22 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2933 October 21, 2022 17:33 Inactive
@colinrotherham colinrotherham changed the base branch from main to package-updates October 21, 2022 17:42
@colinrotherham colinrotherham requested a review from a team as a code owner October 21, 2022 17:42
@36degrees 36degrees self-requested a review October 24, 2022 09:05
Copy link
Contributor

@36degrees 36degrees left a comment

Choose a reason for hiding this comment

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

Generally looks really good – have tested by recreating the issue we had in September (trying to access $module.dataset when $module was undefined) and we now get actually useful messages surfaced in the test output 🙌🏻

A minor nitpick and a suggestion for your consideration, but neither are blocking so happy to approve.

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2933 October 24, 2022 13:22 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2933 October 24, 2022 13:55 Inactive
Base automatically changed from package-updates to main October 24, 2022 16:43
Only three unit test files remain which still need a jsdom environment override:

```
src/govuk/common.unit.test.mjs
src/govuk/components/character-count/character-count.unit.test.mjs
src/govuk/i18n.unit.test.mjs
```
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2933 October 24, 2022 16:46 Inactive
@colinrotherham
Copy link
Contributor Author

Rebased since #2937 merged and all passing still

I'll get this merged as it holds up #2901 and #2902

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

3 participants