debugger: Fix inconsistent inspector output of `exec new Map()`#42423

cola119 · 2022-03-21T15:42:26Z

The output string of exec command is from RemoteObject[customInspectSymbol], but currently it doesn't support some object types such as Map and Set, which leads to invalid string representations (#42405).

This PR implemented string representations when RemoteObject.subtype is set and map. RemoteObject has their abbreviation as ObjectPreview in RemoteObject.preview, which means we can construct their string representation from ObjectPreview.

For example, new Set([{a: 1}, new Set([1]) ]) is a RemoteObject that has two ObjectPreviews ({a: 1} and new Set([1])).
String representation of ObjectPreview {a: 1} will be {a: 1} and new Set([1]) will be Set(1){... }. (Note that ObjectPreview treats an object nested in two or more layers as overflow, so overflowed object is converted to {... })

1d0da48 added test cases for object type RemoteObject
7820dd4 added ObjectPreview and PropertyPreview. And current string representation logics are ported to them.
5a4f767 fixed string representations of Map and Set.

hybrist

Thanks for fixing this! I like the new split here, just one nit/question.

lib/internal/debugger/inspect_repl.js

nodejs-github-bot · 2022-04-17T10:29:38Z

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

Trott · 2022-04-17T13:39:51Z

The way these three commits are organized, the first commit adds a test that fails. This will break git bisect. Can you either move that commit to the end, or else squash the three commits together?

cola119 · 2022-04-17T14:27:11Z

@Trott I squashed them into the one.

nodejs-github-bot · 2022-04-17T14:40:49Z

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

jasnell · 2022-04-20T20:54:05Z

test/parallel/test-debugger-object-type-remote-object.js

+throwerror;
+}
+
+cli.waitForInitialBreak()


oy... rather than a long chain of thens... can we convert this into an async function with awaits please?

@jasnell done 3f21ce5

cola119 · 2022-05-07T03:45:47Z

@Trott @jasnell @jkrems Sorry for bothering you. Could you land this when you are free. Thank you.

cola119 · 2022-05-08T00:58:18Z

I think debugger-* test should be in test/sequential/.

Trott · 2022-05-08T02:14:25Z

I think debugger-* test should be in test/sequential/.

Most of them are. Maybe this one should be too. The ones in parallel should either not open a port or open a system-provided arbitrary port.

nodejs-github-bot · 2022-05-08T14:19:11Z

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

nodejs-github-bot · 2022-05-08T16:40:19Z

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

nodejs-github-bot · 2022-05-08T19:03:52Z

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

nodejs-github-bot · 2022-05-08T19:51:39Z

Landed in f890ef5

PR-URL: #42423 Reviewed-By: Jan Krems <[email protected]> Reviewed-By: Rich Trott <[email protected]>

PR-URL: nodejs/node#42423 Reviewed-By: Jan Krems <[email protected]> Reviewed-By: Rich Trott <[email protected]>

nodejs-github-bot added debugger Issues and PRs related to the debugger subsystem. needs-ci PRs that need a full CI run. labels Mar 21, 2022

cola119 force-pushed the issue/42405 branch from 7e11029 to a69bc81Compare March 21, 2022 23:51

Trott requested a review from hybrist March 22, 2022 02:41

cola119 force-pushed the issue/42405 branch from a69bc81 to 5a4f767Compare March 22, 2022 09:48

hybrist approved these changes Mar 23, 2022
View reviewed changes

lib/internal/debugger/inspect_repl.jsShow resolvedHide resolved

cola119 changed the title ~~debugger: Fix Inconsistent inspector output of exec new Map()~~debugger: Fix inconsistent inspector output of exec new Map()Mar 23, 2022

meixg added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 17, 2022

github-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 17, 2022

cola119 force-pushed the issue/42405 branch from 5a4f767 to a52f32fCompare April 17, 2022 14:26

Trott force-pushed the issue/42405 branch from a52f32f to 82ab123Compare April 17, 2022 14:31

cola119 force-pushed the issue/42405 branch from 82ab123 to 2f64fccCompare April 17, 2022 14:31

Trott added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 17, 2022

Trott approved these changes Apr 17, 2022
View reviewed changes

github-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 17, 2022

github-actionsbot mentioned this pull request Apr 18, 2022
CI Reliability 2022-04-18 nodejs/reliability#255
Open
14 tasks

hybrist approved these changes Apr 18, 2022
View reviewed changes

This was referenced Apr 19, 2022
CI Reliability 2022-04-19 nodejs/reliability#256
Open
CI Reliability 2022-04-20 nodejs/reliability#257
Open

jasnell reviewed Apr 20, 2022
View reviewed changes

This was referenced Apr 21, 2022
CI Reliability 2022-04-21 nodejs/reliability#258
Open
CI Reliability 2022-04-22 nodejs/reliability#259
Open

cola119 force-pushed the issue/42405 branch from 2f64fcc to 3f21ce5Compare April 22, 2022 11:29

cola119 requested a review from jasnell April 22, 2022 11:30

Trott added the request-ci Add this label to start a Jenkins CI on a PR. label May 7, 2022

cola119 force-pushed the issue/42405 branch from 3f21ce5 to d11e707Compare May 8, 2022 00:50

cola119 requested a review from Trott May 8, 2022 04:57

Trott approved these changes May 8, 2022
View reviewed changes

Trott added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels May 8, 2022

github-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 8, 2022

Trott added the commit-queue Add this label to land a pull request using GitHub Actions. label May 8, 2022

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 8, 2022

nodejs-github-bot merged commit f890ef5 into nodejs:masterMay 8, 2022

RafaelGSS pushed a commit that referenced this pull request May 10, 2022
debugger: fix inconsistent inspector output of exec new Map()
946f57c
PR-URL: #42423 Reviewed-By: Jan Krems <[email protected]> Reviewed-By: Rich Trott <[email protected]>

RafaelGSS mentioned this pull request May 10, 2022
v18.2.0 proposal #43036
Merged

github-actionsbot mentioned this pull request May 12, 2022
CI Reliability 2022-05-12 nodejs/reliability#273
Open
30 tasks

joyeecheung mentioned this pull request May 12, 2022
Investigate flaky test-http-server-request-timeout-keepalive #42741
Open

This was referenced May 12, 2022
CI Reliability 2022-05-12 nodejs/reliability#275
Closed
CI Reliability 2022-05-13 nodejs/reliability#276
Open
CI Reliability 2022-05-14 nodejs/reliability#277
Open
CI Reliability 2022-05-15 nodejs/reliability#278
Open

juanarbol pushed a commit that referenced this pull request May 31, 2022
debugger: fix inconsistent inspector output of exec new Map()
1247809
PR-URL: #42423 Reviewed-By: Jan Krems <[email protected]> Reviewed-By: Rich Trott <[email protected]>

juanarbol mentioned this pull request May 31, 2022
2022-05-XX, Version 16.16.0 'Gallium' (LTS) #43268
Closed

danielleadams pushed a commit that referenced this pull request Jun 27, 2022
debugger: fix inconsistent inspector output of exec new Map()
7872da6
PR-URL: #42423 Reviewed-By: Jan Krems <[email protected]> Reviewed-By: Rich Trott <[email protected]>

targos pushed a commit that referenced this pull request Jul 12, 2022
debugger: fix inconsistent inspector output of exec new Map()
44c17f2
PR-URL: #42423 Reviewed-By: Jan Krems <[email protected]> Reviewed-By: Rich Trott <[email protected]>

targos pushed a commit that referenced this pull request Jul 31, 2022
debugger: fix inconsistent inspector output of exec new Map()
f75a5a9
PR-URL: #42423 Reviewed-By: Jan Krems <[email protected]> Reviewed-By: Rich Trott <[email protected]>

targos mentioned this pull request Aug 3, 2022
v16.17.0 release proposal #44098
Merged

guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
debugger: fix inconsistent inspector output of exec new Map()
f68e526
PR-URL: nodejs/node#42423 Reviewed-By: Jan Krems <[email protected]> Reviewed-By: Rich Trott <[email protected]>

Uh oh!

debugger: Fix inconsistent inspector output of exec new Map()#42423

debugger: Fix inconsistent inspector output of exec new Map()#42423

Uh oh!

Conversation

cola119 commented Mar 21, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hybrist left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nodejs-github-bot commented Apr 17, 2022

Uh oh!

Trott commented Apr 17, 2022

Uh oh!

cola119 commented Apr 17, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Apr 17, 2022

Uh oh!

jasnellApr 20, 2022

Choose a reason for hiding this comment

Uh oh!

cola119Apr 22, 2022

Choose a reason for hiding this comment

Uh oh!

cola119 commented May 7, 2022

Uh oh!

cola119 commented May 8, 2022

Uh oh!

Trott commented May 8, 2022

Uh oh!

nodejs-github-bot commented May 8, 2022

Uh oh!

nodejs-github-bot commented May 8, 2022

Uh oh!

nodejs-github-bot commented May 8, 2022

Uh oh!

nodejs-github-bot commented May 8, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

debugger: Fix inconsistent inspector output of `exec new Map()`#42423

debugger: Fix inconsistent inspector output of `exec new Map()`#42423

cola119 commented Mar 21, 2022•
edited
Loading

cola119 commented Apr 17, 2022•
edited
Loading