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.

Sign up for GitHub

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

Jump to bottom

test: validate no debug info for http2 #37447

Closed
wants to merge 1 commit into from
Closed

test: validate no debug info for http2 #37447

wants to merge 1 commit into from

Conversation

mhdawson
Copy link
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 [email protected]

@mhdawson
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 <[email protected]>
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Feb 19, 2021
@lpinca
Copy link
Member

lpinca commented Feb 19, 2021

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

@mhdawson
Copy link
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
Copy link
Member

lpinca commented Feb 20, 2021

Ok, it makes sense, thank you.

@nodejs-github-bot
Copy link
Collaborator

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

@mhdawson
Copy link
Member Author

CI is good, going to land

@mhdawson
Copy link
Member Author

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

@mhdawson
Copy link
Member Author

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

@nodejs-github-bot
Copy link
Collaborator

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

@richardlau
Copy link
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
@mhdawson
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 <[email protected]>

PR-URL: #37447
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
@mhdawson
Copy link
Member Author

Landed in 4eec919

@mhdawson mhdawson closed this Feb 23, 2021
targos pushed a commit that referenced this pull request Feb 28, 2021
@mhdawson @targos
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 <[email protected]>

PR-URL: #37447
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
@targos targos mentioned this pull request Mar 2, 2021
Merged
targos pushed a commit that referenced this pull request May 1, 2021
@mhdawson @targos
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 <[email protected]>

PR-URL: #37447
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
@danielleadams danielleadams mentioned this pull request May 3, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BethGriggs BethGriggs BethGriggs approved these changes

@lpinca lpinca lpinca 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
@mhdawson @lpinca @nodejs-github-bot @richardlau @BethGriggs