FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: remove error allowance in debugger test by MrJithil · Pull Request #41640 · nodejs/node · GitHub

/ node Public

test: remove error allowance in debugger test - #41640

Merged
nodejs-github-bot merged 1 commit into
nodejs:masterfrom
MrJithil:works-on-test-1
Jan 25, 2022
ConversationCommits (1)ChecksFiles changed
Merged

test: remove error allowance in debugger test #41640
nodejs-github-bot merged 1 commit into
nodejs:masterfrom
MrJithil:works-on-test-1

Conversation

MrJithil commented Jan 22, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

Completed a TODO Task

nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Jan 22, 2022

benjamingr commented Jan 22, 2022

Copy link
Copy Markdown
Member

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)

benjamingr added inspector Issues and PRs related to the V8 inspector protocol request-ci Add this label to start a Jenkins CI on a PR. labels Jan 22, 2022
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 22, 2022

nodejs-github-bot commented Jan 22, 2022

Copy link
Copy Markdown
Collaborator

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

Trott approved these changes Jan 22, 2022
View reviewed changes

Trott left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Trott requested a review from hybrist January 22, 2022 14:32

Trott commented Jan 22, 2022

Copy link
Copy Markdown
Member

This may need some dont-backport labels. Not sure.

RaisinTen approved these changes Jan 23, 2022
View reviewed changes

RaisinTen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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/))

Trott commented Jan 23, 2022

Copy link
Copy Markdown
Member

Let's also make the commit message more useful. Maybe this?

test: remove error allowance in debugger test

Remove allowance for FATAL ERROR. It is no longer needed.

MrJithil force-pushed the works-on-test-1 branch 2 times, most recently from 46565c3 to 1b47ad8 Compare January 24, 2022 11:25

MrJithil commented Jan 24, 2022

Copy link
Copy Markdown
Member Author

All comments resolved.

RaisinTen reviewed Jan 24, 2022
View reviewed changes
Comment thread test/sequential/test-debugger-exceptions.js Outdated
Trott changed the title test: completed the TODO test: remove error allowance in debugger test Jan 24, 2022
test: remove error allowance in debugger test
a0df5c5 
Remove allowance for FATAL ERROR. It is no longer needed.
lpinca approved these changes Jan 24, 2022
View reviewed changes

lpinca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

RSLGTM

hybrist approved these changes Jan 24, 2022
View reviewed changes

hybrist left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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!

benjamingr added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 24, 2022
nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Jan 24, 2022

nodejs-github-bot commented Jan 24, 2022

Copy link
Copy Markdown
Collaborator
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/.ncu
https://github.com/nodejs/node/actions/runs/1742665860

benjamingr added request-ci Add this label to start a Jenkins CI on a PR. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Jan 24, 2022
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 24, 2022

nodejs-github-bot commented Jan 24, 2022

Copy link
Copy Markdown
Collaborator

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

Trott added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 25, 2022
nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 25, 2022
nodejs-github-bot merged commit 938ab0e into nodejs:master Jan 25, 2022

nodejs-github-bot commented Jan 25, 2022

Copy link
Copy Markdown
Collaborator

Landed in 938ab0e

benjamingr commented Jan 25, 2022

Copy link
Copy Markdown
Member

Congrats on your first pull request landing @MrJithil !

MrJithil deleted the works-on-test-1 branch January 27, 2022 05:36
ruyadorno pushed a commit that referenced this pull request Feb 8, 2022
test: remove error allowance in debugger test
30df37f 
Remove allowance for FATAL ERROR. It is no longer needed.

PR-URL: #41640
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
ruyadorno mentioned this pull request Feb 8, 2022
Merged
danielleadams pushed a commit that referenced this pull request Mar 2, 2022
test: remove error allowance in debugger test
86dbb0e 
Remove allowance for FATAL ERROR. It is no longer needed.

PR-URL: #41640
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
danielleadams mentioned this pull request Mar 3, 2022
Merged
danielleadams pushed a commit that referenced this pull request Mar 3, 2022
test: remove error allowance in debugger test
f7c1576 
Remove allowance for FATAL ERROR. It is no longer needed.

PR-URL: #41640
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
danielleadams pushed a commit that referenced this pull request Mar 14, 2022
test: remove error allowance in debugger test
2b87f97 
Remove allowance for FATAL ERROR. It is no longer needed.

PR-URL: #41640
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

RaisinTen RaisinTen approved these changes
hybrist hybrist approved these changes
Trott Trott approved these changes
lpinca lpinca approved these changes
tniessen tniessen approved these changes

Assignees

No one assigned

Labels

inspector Issues and PRs related to the V8 inspector protocol 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.

8 participants

Back | FazBrowse Home | New Git URL