Skip to content

Conversation

@MrJithil
Copy link
Member

@MrJithilMrJithil commented Jan 22, 2022

Completed a TODO Task

@nodejs-github-botnodejs-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
Copy link
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)

@benjamingrbenjamingr 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-actionsgithub-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 22, 2022
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@TrottTrott left a 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.

@TrottTrott requested a review from hybristJanuary 22, 2022 14:32
@Trott
Copy link
Member

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

Copy link
Member

@RaisinTenRaisinTen left a 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

// TODO: Remove FATAL ERROR once node doesn't show a FATAL ERROR anymore
.then(()=>cli.waitFor(/disconnect|FATALERROR/))

@Trott
Copy link
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. 

@MrJithilMrJithilforce-pushed the works-on-test-1 branch 2 times, most recently from 46565c3 to 1b47ad8CompareJanuary 24, 2022 11:25
@MrJithil
Copy link
MemberAuthor

All comments resolved.

@TrottTrott changed the title test: completed the TODOtest: remove error allowance in debugger test Jan 24, 2022
Remove allowance for FATAL ERROR. It is no longer needed.
Copy link
Member

@lpincalpinca left a comment

Choose a reason for hiding this comment

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

RSLGTM

Copy link
Contributor

@hybristhybrist left a 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 :shipit:. Thanks for cleaning this up!

@benjamingrbenjamingr added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 24, 2022
@nodejs-github-botnodejs-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
Copy link
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

@benjamingrbenjamingr 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-actionsgithub-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 24, 2022
@nodejs-github-bot
Copy link
Collaborator

@TrottTrott added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 25, 2022
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 25, 2022
@nodejs-github-botnodejs-github-bot merged commit 938ab0e into nodejs:masterJan 25, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 938ab0e

@benjamingr
Copy link
Member

Congrats on your first pull request landing @MrJithil !

@MrJithilMrJithil deleted the works-on-test-1 branch January 27, 2022 05:36
Linkgoron pushed a commit to Linkgoron/node that referenced this pull request Jan 31, 2022
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]>
ruyadorno pushed a commit that referenced this pull request Feb 8, 2022
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]>
@ruyadornoruyadorno mentioned this pull request Feb 8, 2022
danielleadams pushed a commit that referenced this pull request Mar 2, 2022
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]>
danielleadams pushed a commit that referenced this pull request Mar 3, 2022
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]>
danielleadams pushed a commit that referenced this pull request Mar 14, 2022
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]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

inspectorIssues and PRs related to the V8 inspector protocolneeds-ciPRs that need a full CI run.testIssues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@MrJithil@benjamingr@nodejs-github-bot@Trott@hybrist@lpinca@tniessen@RaisinTen