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: validate no debug info for http2 #37447

Closed
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]

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

@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

@nodejs-github-bot
Copy link
Collaborator

@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
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
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 pushed a commit that referenced this pull request May 1, 2021
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants