Skip to content

Conversation

bnoordhuis
Copy link
Member

@bnoordhuis bnoordhuis commented Apr 18, 2017

The file no longer works after the removal of the --debug/--debug-brk
switches in commit 47f8f74 ("src: remove support for --debug".)

This commit also removes several tests that still referenced the
old debugger but were either unit-testing its internals or passing
for the wrong reason (like expecting an operation to fail, which
it did because the debugger is gone.)

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

Overlooked in commit 47f8f74 ("src: remove support for --debug") from
earlier this month.
The file no longer works after the removal of the --debug/--debug-brk
switches in commit 47f8f74 ("src: remove support for --debug".)

This commit also removes several tests that still referenced the
old debugger but were either unit-testing its internals or passing
for the wrong reason (like expecting an operation to fail, which
it did because the debugger is gone.)
@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. c++ Issues and PRs that require attention from people who are familiar with C++. debugger lib / src Issues and PRs related to general changes in the lib or src directory. module Issues and PRs related to the module subsystem. labels Apr 18, 2017
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

Removing stuff is goodness.

@jasnell jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Apr 18, 2017
@jasnell
Copy link
Member

jasnell commented Apr 18, 2017

Defensively flagging as semver-major. If it needed be, please remove the flag.

@gibfahn
Copy link
Member

gibfahn commented Apr 18, 2017

There is a request to re-add --debug-brk in master and Node 8: #12364, which would presumably mean that we'd have to keep the --debug-brk bits.

@bnoordhuis
Copy link
Member Author

What I've removed is all dead code. I was aware of #12364 so I left in the --debug and --debug-brk handling logic in lib/internal/cluster.js.

Whoever is going to pick up that issue should look closely at that file and the NODE_DEBUG_ENABLED logic in particular.

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

LGTM 👋

@bnoordhuis bnoordhuis closed this Apr 21, 2017
bnoordhuis added a commit to bnoordhuis/io.js that referenced this pull request Apr 21, 2017
Overlooked in commit 47f8f74 ("src: remove support for --debug") from
earlier this month.

PR-URL: nodejs#12495
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
bnoordhuis added a commit to bnoordhuis/io.js that referenced this pull request Apr 21, 2017
The file no longer works after the removal of the --debug/--debug-brk
switches in commit 47f8f74 ("src: remove support for --debug".)

This commit also removes several tests that still referenced the
old debugger but were either unit-testing its internals or passing
for the wrong reason (like expecting an operation to fail, which
it did because the debugger is gone.)

PR-URL: nodejs#12495
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@bnoordhuis
Copy link
Member Author

For some reason the PR doesn't show up as merged but the relevant changes were landed in ee0620b...90476ac.

@jasnell jasnell mentioned this pull request May 11, 2017
@bnoordhuis bnoordhuis mentioned this pull request Jun 19, 2017
2 tasks
cjihrig added a commit to cjihrig/node that referenced this pull request Oct 1, 2018
internal/cluster/utils.js exported a handles object, which was
used in a test. That test, test-cluster-disconnect-handles.js,
was removed in nodejs#12495. This
commit moves the handles object to the only file in the codebase
that still uses it.

PR-URL: nodejs#23131
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Oct 1, 2018
internal/cluster/utils.js exported a handles object, which was
used in a test. That test, test-cluster-disconnect-handles.js,
was removed in #12495. This
commit moves the handles object to the only file in the codebase
that still uses it.

PR-URL: #23131
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Oct 3, 2018
internal/cluster/utils.js exported a handles object, which was
used in a test. That test, test-cluster-disconnect-handles.js,
was removed in #12495. This
commit moves the handles object to the only file in the codebase
that still uses it.

PR-URL: #23131
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. module Issues and PRs related to the module subsystem. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants