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

test: improve error logging for inspector test #14508

Closed
wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Jul 27, 2017

If JSON.parse() fails, print a message showing the JSON that failed to
parse. This is to help with debugging a current test failure on CI.

Refs: #14507

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test inspector

@Trott Trott added inspector Issues and PRs related to the V8 inspector protocol test Issues and PRs related to the tests. labels Jul 27, 2017
@nodejs-github-bot nodejs-github-bot added dont-land-on-v4.x inspector Issues and PRs related to the V8 inspector protocol test Issues and PRs related to the tests. labels Jul 27, 2017
@Trott
Copy link
Member Author

Trott commented Jul 27, 2017

@@ -414,7 +414,10 @@ function error_test() {
expect: `${prompt_multiline}'foo \\n'\n${prompt_unix}` },
// Whitespace is not evaluated.
{ client: client_unix, send: ' \t \n',
expect: prompt_unix }
expect: prompt_unix },
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this related?

Copy link
Member Author

Choose a reason for hiding this comment

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

@refack No, it's not. That's an error on my part. Will remove it. It's from an unrelated PR I was testing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed. Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

I recognized the ... REPL test 😉

If JSON.parse() fails, print a message showing the JSON that failed to
parse. This is to help with debugging a current test failure on CI.

Refs: nodejs#14507
@Trott
Copy link
Member Author

Trott commented Jul 27, 2017

Rebased, resolved merge conflict, force pushed.

@Trott
Copy link
Member Author

Trott commented Jul 27, 2017

@Trott
Copy link
Member Author

Trott commented Jul 28, 2017

Since this test fails (per #14507) on CI pretty frequently, I'd like to fast-track this to get better information about how it's failing. Does that seem reasonable? @nodejs/testing

@cjihrig
Copy link
Contributor

cjihrig commented Jul 28, 2017

Seems reasonable to me.

Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

👍 for fast track

@Trott
Copy link
Member Author

Trott commented Jul 29, 2017

Landed in bfb4f42

@Trott Trott closed this Jul 29, 2017
Trott added a commit to Trott/io.js that referenced this pull request Jul 29, 2017
If JSON.parse() fails, print a message showing the JSON that failed to
parse. This is to help with debugging a current test failure on CI.

PR-URL: nodejs#14508
Ref: nodejs#14507
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
addaleax pushed a commit that referenced this pull request Jul 29, 2017
If JSON.parse() fails, print a message showing the JSON that failed to
parse. This is to help with debugging a current test failure on CI.

PR-URL: #14508
Ref: #14507
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@addaleax addaleax mentioned this pull request Aug 2, 2017
MylesBorins pushed a commit that referenced this pull request Aug 16, 2017
If JSON.parse() fails, print a message showing the JSON that failed to
parse. This is to help with debugging a current test failure on CI.

PR-URL: #14508
Ref: #14507
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Aug 16, 2017
MylesBorins pushed a commit that referenced this pull request Aug 16, 2017
If JSON.parse() fails, print a message showing the JSON that failed to
parse. This is to help with debugging a current test failure on CI.

PR-URL: #14508
Ref: #14507
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
MylesBorins pushed a commit that referenced this pull request Sep 3, 2017
If JSON.parse() fails, print a message showing the JSON that failed to
parse. This is to help with debugging a current test failure on CI.

PR-URL: #14508
Ref: #14507
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
MylesBorins pushed a commit that referenced this pull request Sep 5, 2017
If JSON.parse() fails, print a message showing the JSON that failed to
parse. This is to help with debugging a current test failure on CI.

PR-URL: #14508
Ref: #14507
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
inspector Issues and PRs related to the V8 inspector protocol test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants