FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: validate no debug info for http2 by mhdawson · Pull Request #37447 · nodejs/node · GitHub

/ node Public

test: validate no debug info for http2 - #37447

Closed
mhdawson wants to merge 1 commit into
nodejs:masterfrom
mhdawson:noisyHttp
ConversationCommits (1)ChecksFiles changed
Closed

test: validate no debug info for http2#37447
mhdawson wants to merge 1 commit into
nodejs:masterfrom
mhdawson:noisyHttp

Conversation

mhdawson commented Feb 19, 2021

Copy link
Copy Markdown
Member

Refs: #31763

This test would have helped us catch the noisy output
from http2 earlier. Currently none of the tests
fail if there is unexpected debug output.

Signed-off-by: Michael Dawson mdawson@devrus.com

test: validate no debug info for http2
bbd62b4 
Refs: nodejs#31763

This test would have helped us catch the noisy output
from http2 earlier. Currently none of the tests
fail if there is unexpected debug output.

Signed-off-by: Michael Dawson <mdawson@devrus.com>
nodejs-github-bot added the test Issues and PRs related to the tests. label Feb 19, 2021

lpinca commented Feb 19, 2021

Copy link
Copy Markdown
Member

If --debug-nghttp2 is used the noisy output is expected, no?

mhdawson commented Feb 19, 2021

Copy link
Copy Markdown
Member Author

@lpinca I was on the fence about that but I don't think people will be planning to ship binaries with that noisy output enabled and having a test failure that helps avoid that is useful since we have already seen it happen.

In terms of impact to a regular test flow I'm surprised that tests did not already fail as there are tests in other areas that would with extra debug output. ie when you turn on a bunch of extra debug output I don't think there is an expectation that all of the tests will pass.

So the net is that I'm thinking its no impact to people using the tests and adds value by helping to avoid a situation we already saw happen.

lpinca commented Feb 20, 2021

Copy link
Copy Markdown
Member

Ok, it makes sense, thank you.

nodejs-github-bot commented Feb 23, 2021

Copy link
Copy Markdown
Collaborator

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

mhdawson commented Feb 23, 2021

Copy link
Copy Markdown
Member Author

CI is good, going to land

mhdawson commented Feb 23, 2021

Copy link
Copy Markdown
Member Author

We'll git-node land tells me the last CI failed, not sure why but will run another one.

mhdawson commented Feb 23, 2021

Copy link
Copy Markdown
Member Author

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

nodejs-github-bot commented Feb 23, 2021

Copy link
Copy Markdown
Collaborator

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

richardlau commented Feb 23, 2021

Copy link
Copy Markdown
Member

We'll git-node land tells me the last CI failed, not sure why but will run another one.

It's most likely the failing test-asan GitHub actions run (#37442) which was fixed by #37443.

mhdawson added a commit that referenced this pull request Feb 23, 2021
test: validate no debug info for http2
4eec919 
Refs: #31763

This test would have helped us catch the noisy output
from http2 earlier. Currently none of the tests
fail if there is unexpected debug output.

Signed-off-by: Michael Dawson <mdawson@devrus.com>

PR-URL: #37447
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Beth Griggs <bgriggs@redhat.com>

mhdawson commented Feb 23, 2021

Copy link
Copy Markdown
Member Author

Landed in 4eec919

mhdawson closed this Feb 23, 2021
targos pushed a commit that referenced this pull request Feb 28, 2021
test: validate no debug info for http2
dfa0440 
Refs: #31763

This test would have helped us catch the noisy output
from http2 earlier. Currently none of the tests
fail if there is unexpected debug output.

Signed-off-by: Michael Dawson <mdawson@devrus.com>

PR-URL: #37447
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Beth Griggs <bgriggs@redhat.com>
targos mentioned this pull request Mar 2, 2021
Merged
targos pushed a commit that referenced this pull request May 1, 2021
test: validate no debug info for http2
d168cde 
Refs: #31763

This test would have helped us catch the noisy output
from http2 earlier. Currently none of the tests
fail if there is unexpected debug output.

Signed-off-by: Michael Dawson <mdawson@devrus.com>

PR-URL: #37447
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Beth Griggs <bgriggs@redhat.com>
danielleadams mentioned this pull request May 3, 2021
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

lpinca lpinca approved these changes
BethGriggs BethGriggs approved these changes

Assignees

No one assigned

Labels

test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL