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

doc: clarify guide on testing internal errors #42813

Merged
merged 2 commits into from
Apr 27, 2022
Merged

doc: clarify guide on testing internal errors #42813

merged 2 commits into from
Apr 27, 2022

Conversation

LiviaMedeiros
Copy link
Contributor

Guide on writing tests seems to be contradicting with the purpose of internal/errors:

node/lib/internal/errors.js

Lines 7 to 11 in fe85cf7

// The whole point behind this internal module is to allow Node.js to no
// longer be forced to treat every error message change as a semver-major
// change. The NodeError classes here all expose a `code` property whose
// value statically and permanently identifies the error. While the error
// message may change, the code should not.

The code example might give several ideas on why .message is subject to change at any time.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/tsc

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Apr 21, 2022
@aduh95 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 21, 2022
mhdawson
mhdawson approved these changes Apr 21, 2022
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

cjihrig
cjihrig approved these changes Apr 21, 2022
View reviewed changes
doc/contributing/writing-tests.md Outdated Show resolved Hide resolved
@LiviaMedeiros @cjihrig
squash: add articles
24d9407 
Co-authored-by: Colin Ihrig <cjihrig@gmail.com>
@aduh95 aduh95 mentioned this pull request Apr 22, 2022
Merged
@LiviaMedeiros LiviaMedeiros changed the title doc: clarify guide on testing internal errors docs: clarify guide on testing internal errors Apr 26, 2022
@LiviaMedeiros LiviaMedeiros changed the title docs: clarify guide on testing internal errors doc: clarify guide on testing internal errors Apr 26, 2022
@aduh95 aduh95 added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Apr 26, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 26, 2022
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/42813
✔  Done loading data for nodejs/node/pull/42813
----------------------------------- PR info ------------------------------------
Title      doc: clarify guide on testing internal errors (#42813)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     LiviaMedeiros:guide-internal-errors -> nodejs:master
Labels     doc, author ready, commit-queue-squash
Commits    2
 - doc: clarify guide on testing internal errors
 - squash: add articles
Committers 2
 - LiviaMedeiros 
 - GitHub 
PR-URL: https://github.com/nodejs/node/pull/42813
Reviewed-By: Antoine du Hamel 
Reviewed-By: Darshan Sen 
Reviewed-By: Mohammed Keyvanzadeh 
Reviewed-By: Michael Dawson 
Reviewed-By: Colin Ihrig 
Reviewed-By: Akhil Marsonya 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/42813
Reviewed-By: Antoine du Hamel 
Reviewed-By: Darshan Sen 
Reviewed-By: Mohammed Keyvanzadeh 
Reviewed-By: Michael Dawson 
Reviewed-By: Colin Ihrig 
Reviewed-By: Akhil Marsonya 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last review:
   ⚠  - squash: add articles
   ℹ  This PR was created on Thu, 21 Apr 2022 11:07:52 GMT
   ✔  Approvals: 6
   ✔  - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/42813#pullrequestreview-948638848
   ✔  - Darshan Sen (@RaisinTen) (TSC): https://github.com/nodejs/node/pull/42813#pullrequestreview-948667994
   ✔  - Mohammed Keyvanzadeh (@VoltrexMaster): https://github.com/nodejs/node/pull/42813#pullrequestreview-948823902
   ✔  - Michael Dawson (@mhdawson) (TSC): https://github.com/nodejs/node/pull/42813#pullrequestreview-949283669
   ✔  - Colin Ihrig (@cjihrig) (TSC): https://github.com/nodejs/node/pull/42813#pullrequestreview-949293894
   ✔  - Akhil Marsonya (@marsonya): https://github.com/nodejs/node/pull/42813#pullrequestreview-949319752
   ✔  Last GitHub CI successful
   ℹ  Green GitHub CI is sufficient
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/2230022874

@nodejs-github-bot nodejs-github-bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Apr 26, 2022
@aduh95 aduh95 added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Apr 27, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 27, 2022
@nodejs-github-bot nodejs-github-bot merged commit fc485a9 into nodejs:master Apr 27, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in fc485a9

targos pushed a commit that referenced this pull request Apr 28, 2022
@LiviaMedeiros @targos
doc: clarify guide on testing internal errors
fe65996 
PR-URL: #42813
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Akhil Marsonya <akhil.marsonya27@gmail.com>
@targos targos mentioned this pull request May 2, 2022
Merged
juanarbol pushed a commit that referenced this pull request May 31, 2022
@LiviaMedeiros @juanarbol
doc: clarify guide on testing internal errors
20acb48 
PR-URL: #42813
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Akhil Marsonya <akhil.marsonya27@gmail.com>
@juanarbol juanarbol mentioned this pull request May 31, 2022
Closed
danielleadams pushed a commit that referenced this pull request Jun 27, 2022
@LiviaMedeiros @danielleadams
doc: clarify guide on testing internal errors
34daec2 
PR-URL: #42813
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Akhil Marsonya <akhil.marsonya27@gmail.com>
targos pushed a commit that referenced this pull request Jul 12, 2022
@LiviaMedeiros @targos
doc: clarify guide on testing internal errors
12eab82 
PR-URL: #42813
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Akhil Marsonya <akhil.marsonya27@gmail.com>
targos pushed a commit that referenced this pull request Jul 31, 2022
@LiviaMedeiros @targos
doc: clarify guide on testing internal errors
e45861f 
PR-URL: #42813
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Akhil Marsonya <akhil.marsonya27@gmail.com>
@targos targos mentioned this pull request Aug 3, 2022
Merged
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
@LiviaMedeiros @guangwong
doc: clarify guide on testing internal errors
5083f24 
PR-URL: nodejs/node#42813
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Akhil Marsonya <akhil.marsonya27@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@VoltrexKeyva VoltrexKeyva VoltrexKeyva approved these changes

@RaisinTen RaisinTen RaisinTen approved these changes

@marsonya marsonya marsonya approved these changes

@aduh95 aduh95 aduh95 approved these changes

@mhdawson mhdawson mhdawson approved these changes

@cjihrig cjihrig cjihrig 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. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. doc Issues and PRs related to the documentations.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@LiviaMedeiros @nodejs-github-bot @cjihrig @mhdawson @aduh95 @marsonya @RaisinTen @VoltrexKeyva