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

debugger: fix TypeError of list command in inspector repl #42776

Merged
merged 5 commits into from
May 22, 2022
Merged

debugger: fix TypeError of list command in inspector repl #42776

merged 5 commits into from
May 22, 2022

Conversation

cola119
Copy link
Member

@cola119 cola119 commented Apr 18, 2022 •
edited by aduh95
Loading

Currently list command throws TypeError after the inspector is disconnected.
This PR fixed to throw ERR_DEBUGGER_ERROR instead of TypeError.

Fixes: #42775

@nodejs-github-bot nodejs-github-bot added debugger Issues and PRs related to the debugger subsystem. needs-ci PRs that need a full CI run. labels Apr 18, 2022
@cola119 cola119 requested a review from aduh95 April 20, 2022 01:46
@meixg
Copy link
Member

meixg commented Apr 20, 2022

There is a ci failure that seems related to the change : https://github.com/nodejs/node/runs/6070093179?check_suite_focus=true

@cola119 cola119 force-pushed the fix-cannot-read-props-of-null-error branch from ae6defd to cd64887 Compare April 20, 2022 03:16
@cola119
Copy link
Member Author

cola119 commented Apr 20, 2022

@meixg Thank you. I fixed. (but test/parallel/test-cluster-primary-kill.js failed 🥲

@meixg meixg added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 20, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 20, 2022
@nodejs-github-bot
Copy link
Collaborator

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

@aduh95
Copy link
Contributor

aduh95 commented Apr 20, 2022 •
edited
Loading

Based on the CI failures, it looks like this is quite flaky on some configurations 🤔 Any idea what might be causing this?

@cola119
Copy link
Member Author

cola119 commented Apr 20, 2022

@aduh95 It seems to be fixed by 2206bd0

jasnell
jasnell reviewed Apr 20, 2022
View reviewed changes
test/sequential/test-debugger-list.js Outdated

const cli = startCLI([fixtures.path('debugger/three-lines.js')]);

cli.waitForInitialBreak()
Copy link
Member

Choose a reason for hiding this comment

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

Can this be refactored into an async function with awaits rather than a long then chain?

Copy link
Member Author

Choose a reason for hiding this comment

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

@jasnell done fea4682

@github-actions github-actions bot mentioned this pull request Apr 21, 2022
Open
22 tasks
@cola119 cola119 requested a review from jasnell April 21, 2022 02:58
@aduh95 aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 21, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 21, 2022
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@aduh95
Copy link
Contributor

aduh95 commented Apr 21, 2022

There seems to be some related CI failures

node:internal/process/promises:288
            triggerUncaughtException(err, true /* fromPromise */);
            ^

AssertionError [ERR_ASSERTION]: The input did not match the regular expression /Uncaught Error \[ERR_DEBUGGER_ERROR\]: Requires execution to be paused/. Input:

'debug> '

    at /home/iojs/build/workspace/node-test-commit-linux/test/sequential/test-debugger-list.js:26:10
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5) {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: 'debug> ',
  expected: /Uncaught Error \[ERR_DEBUGGER_ERROR\]: Requires execution to be paused/,
  operator: 'match'
}

Node.js v19.0.0-pre

@github-actions github-actions bot mentioned this pull request Apr 22, 2022
Open
19 tasks
@cola119 cola119 force-pushed the fix-cannot-read-props-of-null-error branch from fea4682 to 88f48e0 Compare April 22, 2022 03:59
@aduh95 aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 22, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 22, 2022
@nodejs-github-bot
Copy link
Collaborator

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

This was referenced May 20, 2022
Open
Open
Open
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@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. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels May 22, 2022
@aduh95 aduh95 merged commit cbb92b0 into nodejs:master May 22, 2022
@aduh95
Copy link
Contributor

aduh95 commented May 22, 2022

Landed in cbb92b0

This was referenced May 23, 2022
Open
Open
Open
Open
Open
Open
bengl pushed a commit that referenced this pull request May 30, 2022
@cola119 @bengl
debugger: throw a more useful error when the frame is missing
c76caad 
PR-URL: #42776
Fixes: #42775
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@bengl bengl mentioned this pull request May 31, 2022
Merged
juanarbol pushed a commit that referenced this pull request May 31, 2022
@cola119 @juanarbol
debugger: throw a more useful error when the frame is missing
1702a80 
PR-URL: #42776
Fixes: #42775
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@juanarbol juanarbol mentioned this pull request May 31, 2022
Closed
@danielleadams
Copy link
Contributor

@cola119 This breaks the test suite when trying to land in v16.x. Do you mind opening up a backport PR to v16.x-staging? (docs) Thank you.

cola119 added a commit to cola119/node that referenced this pull request Jun 27, 2022
@cola119
debugger: throw a more useful error when the frame is missing
56b136f 
PR-URL: nodejs#42776
Fixes: nodejs#42775
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@cola119 cola119 mentioned this pull request Jun 27, 2022
Closed
cola119 added a commit to cola119/node that referenced this pull request Jun 28, 2022
@cola119
debugger: throw a more useful error when the frame is missing
04b2f33 
PR-URL: nodejs#42776
Fixes: nodejs#42775
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Jul 18, 2022
@cola119 @targos
debugger: throw a more useful error when the frame is missing
98a5670 
PR-URL: #42776
Fixes: #42775
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Jul 31, 2022
@cola119 @targos
debugger: throw a more useful error when the frame is missing
f1c12aa 
PR-URL: #42776
Fixes: #42775
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@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
@cola119 @guangwong
debugger: throw a more useful error when the frame is missing
db4ef5c 
PR-URL: nodejs/node#42776
Fixes: nodejs/node#42775
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@austinkelleher austinkelleher austinkelleher left review comments

@jasnell jasnell jasnell approved these changes

@aduh95 aduh95 aduh95 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. debugger Issues and PRs related to the debugger subsystem. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Debugger's list command throws TypeError: Cannot read properties of null (reading 'list')
9 participants
@cola119 @meixg @nodejs-github-bot @aduh95 @danielleadams @jasnell @austinkelleher @watilde @targos