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.

Sign up for GitHub

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

Jump to bottom

test: fill DOMException names #43615

Merged
merged 1 commit into from
Jul 2, 2022
Merged

test: fill DOMException names #43615

merged 1 commit into from
Jul 2, 2022

Conversation

LiviaMedeiros
Copy link
Contributor

DOMException.name is preferable over DOMException.code. Also this makes affected tests a bit more self-documented.

Note that instead of numbers we can use legacy constants exposed as DOMException properties.
And for testing compatibility with Web APIs we can explicitly check that error is not Node.js-specific by asserting its constructor.

  throws(
    () => AbortSignal.abort().throwIfAborted(),
    {
      constructor: DOMException,
      code: DOMException.ABORT_ERR,
      name: 'AbortError',
    }
  );

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Jun 29, 2022
@LiviaMedeiros LiviaMedeiros added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 29, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 29, 2022
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@aduh95 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 29, 2022
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/45048/

@LiviaMedeiros
test: fill DOMException names
2014608 
PR-URL: nodejs#43615
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
@LiviaMedeiros
Copy link
Contributor Author

Landed in 2014608

@LiviaMedeiros LiviaMedeiros merged commit 2014608 into nodejs:main Jul 2, 2022
@github-actions github-actions bot mentioned this pull request Jul 4, 2022
Open
23 tasks
targos pushed a commit that referenced this pull request Jul 12, 2022
@LiviaMedeiros @targos
test: fill DOMException names
043756d 
PR-URL: #43615
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
@targos targos mentioned this pull request Jul 12, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RaisinTen RaisinTen RaisinTen approved these changes

@aduh95 aduh95 aduh95 approved these changes

@lpinca lpinca lpinca approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. needs-ci PRs that need a full CI run. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@LiviaMedeiros @nodejs-github-bot @bnoordhuis @lpinca @aduh95 @RaisinTen @targos