-
Notifications
You must be signed in to change notification settings - Fork 30k
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
test: remove error allowance in debugger test #41640
Conversation
Thanks! cc @Trott (last blame on the file though I think for moving it?) and @nodejs/inspector (is this safe? I'm missing context, will also start a CI run) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM if CI is green. There's another one later in the file but no idea if it is similarly necessary or not.
This was moved from https://github.com/nodejs/node-inspect/blob/643179ef5e0452a52ceef2c5e7c8ca42ab00280f/test/cli/exceptions.test.js#L24-L25 which was authored by @jkrems so a review from them would be nice.
This may need some |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This too can be changed
node/test/sequential/test-debugger-exceptions.js
Lines 69 to 70 in 0e6db9c
// TODO: Remove FATAL ERROR once node doesn't show a FATAL ERROR anymore | |
.then(() => cli.waitFor(/disconnect|FATAL ERROR/)) |
Let's also make the commit message more useful. Maybe this?
|
46565c3
to
1b47ad8
Compare
All comments resolved. |
Remove allowance for FATAL ERROR. It is no longer needed.
1b47ad8
to
a0df5c5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RSLGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wish I had the foresight to add a specific issue reference to that TODO. IIRC it was something about different node versions behaving differently. So if the tests pass and aren't flaky, then . Thanks for cleaning this up!
Commit Queue failed- Loading data for nodejs/node/pull/41640 ✔ Done loading data for nodejs/node/pull/41640 ----------------------------------- PR info ------------------------------------ Title test: remove error allowance in debugger test (#41640) ⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch MrJithil:works-on-test-1 -> nodejs:master Labels test, inspector, needs-ci, dont-land-on-v12.x, dont-land-on-v14.x Commits 1 - test: remove error allowance in debugger test Committers 1 - MrJithil PR-URL: https://github.com/nodejs/node/pull/41640 Reviewed-By: Rich Trott Reviewed-By: Tobias Nießen Reviewed-By: Darshan Sen Reviewed-By: Luigi Pinca Reviewed-By: Jan Krems ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/41640 Reviewed-By: Rich Trott Reviewed-By: Tobias Nießen Reviewed-By: Darshan Sen Reviewed-By: Luigi Pinca Reviewed-By: Jan Krems -------------------------------------------------------------------------------- ℹ This PR was created on Sat, 22 Jan 2022 06:02:30 GMT ✔ Approvals: 5 ✔ - Rich Trott (@Trott) (TSC): https://github.com/nodejs/node/pull/41640#pullrequestreview-860211046 ✔ - Tobias Nießen (@tniessen) (TSC): https://github.com/nodejs/node/pull/41640#pullrequestreview-860211421 ✔ - Darshan Sen (@RaisinTen) (TSC): https://github.com/nodejs/node/pull/41640#pullrequestreview-860319062 ✔ - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/41640#pullrequestreview-861446096 ✔ - Jan Krems (@jkrems): https://github.com/nodejs/node/pull/41640#pullrequestreview-861580196 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2022-01-22T08:07:44Z: https://ci.nodejs.org/job/node-test-pull-request/42080/ ⚠ Commits were pushed after the last Full PR CI run: ⚠ - test: remove error allowance in debugger test - Querying data for job/node-test-pull-request/42080/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/1742665860 |
Landed in 938ab0e |
Congrats on your first pull request landing @MrJithil ! |
Remove allowance for FATAL ERROR. It is no longer needed. PR-URL: nodejs#41640 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Jan Krems <[email protected]>
Remove allowance for FATAL ERROR. It is no longer needed. PR-URL: #41640 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Jan Krems <[email protected]>
Remove allowance for FATAL ERROR. It is no longer needed. PR-URL: #41640 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Jan Krems <[email protected]>
Remove allowance for FATAL ERROR. It is no longer needed. PR-URL: #41640 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Jan Krems <[email protected]>
Remove allowance for FATAL ERROR. It is no longer needed. PR-URL: #41640 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Jan Krems <[email protected]>
Completed a TODO Task