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.

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

debugger: assert test before accessing this.binding #5145

Closed
wants to merge 1 commit into from

Conversation

princejwesley
Copy link
Contributor

assert this.binding before accessing it

@princejwesley
Copy link
Contributor Author

/cc @indutny

@MylesBorins
Copy link
Contributor

LGTM

1 similar comment
@indutny
Copy link
Member

indutny commented Feb 8, 2016

LGTM

@mscdex mscdex added the debugger label Feb 8, 2016
@thefourtheye
Copy link
Contributor

LGTM

@Trott
Copy link
Member

Trott commented Feb 9, 2016

Did you manage to trigger this issue (so that your code failed with cannot read property of undefined or something like that when it should have failed with the assertion error)? Or did you simply see the issue in the code without triggering it?

@princejwesley
Copy link
Contributor Author

@Trott I saw the issue in the code without triggering it.
Code before:
image

After:
image

@Trott
Copy link
Member

Trott commented Feb 9, 2016

LGTM

A test might be a good idea, or it might be overkill. Thoughts?

The test would be simple based on your code above:

'use strict';
require('../common');
const assert = require('assert');

assert.throws(() => { require('_debug_agent').start(); },
  assert.AssertionError);

@princejwesley
Copy link
Contributor Author

@Trott I just have rearranged the code (predicate assertion first).
I couldn't find tests for _debug_agent.js. shall I create test/debugger/test-debugger-agent.js ?

@Trott
Copy link
Member

Trott commented Feb 9, 2016

@princejwesley I would put it in test/parallel rather than test/debugger. test-debugger-agent.js is fine for a file name, I think.

@princejwesley
Copy link
Contributor Author

@Trott I have used test/parallel/test-debug-agent.js.

@Trott
Copy link
Member

Trott commented Feb 9, 2016

@Trott
Copy link
Member

Trott commented Feb 9, 2016

CI looks good! Thanks for doing this!

@jasnell
Copy link
Member

jasnell commented Feb 10, 2016

LGTM

jasnell pushed a commit that referenced this pull request Feb 10, 2016
PR-URL: #5145
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@jasnell
Copy link
Member

jasnell commented Feb 10, 2016

Landed in 826844e

@jasnell jasnell closed this Feb 10, 2016
rvagg pushed a commit that referenced this pull request Feb 15, 2016
PR-URL: #5145
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 1, 2016
PR-URL: #5145
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 1, 2016
PR-URL: #5145
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 2, 2016
PR-URL: #5145
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Rich Trott <[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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants