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

http2: updates test coverage for Http2CodecImplTest.CheckHeaderValueValidation #37966

Merged

Conversation

birenroy
Copy link
Contributor

@birenroy birenroy commented Jan 10, 2025

The only case where codec behavior differs is \0, so that case is still skipped.

UHV + oghttp2 is also skipped, as otherwise the compile-time-options build fails.

Commit Message:
Additional Description:
Risk Level: none, test only
Testing: ran codec_impl_test locally
Docs Changes:
Release Notes:
Platform Specific Features:

Copy link

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #37966 was opened by birenroy.

see: more, trace.

Signed-off-by: Biren Roy <[email protected]>
@birenroy birenroy marked this pull request as ready for review January 10, 2025 21:51
@birenroy
Copy link
Contributor Author

/assign @adisuissa
/assign @RyanTheOptimist

Copy link
Contributor

@RyanTheOptimist RyanTheOptimist left a comment

Choose a reason for hiding this comment

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

OH, nice!

@birenroy birenroy marked this pull request as draft January 10, 2025 23:05
@birenroy
Copy link
Contributor Author

Looks like the compile-time-options build needs more work. I'll ping when I need another review.

@RyanTheOptimist
Copy link
Contributor

@birenroy
Copy link
Contributor Author

Is this failure real? https://github.com/envoyproxy/envoy/actions/runs/12717776264/job/35454951834

Looks like UHV + oghttp2 fails for some reason. I'll leave the test exclusion in place for that combination.

@birenroy birenroy marked this pull request as ready for review January 11, 2025 00:20
TestRequestHeaderMapImpl request_headers;
HttpTestUtility::addDefaultHeaders(request_headers);
header_value[2] = static_cast<char>(i);

SCOPED_TRACE(absl::StrCat("header value: [", absl::CEscape(header_value), "]"));
Copy link
Contributor

Choose a reason for hiding this comment

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

Just checking that this addition is intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only effect is to print the header value to logs iff a test expectation fails. I found this helpful when looking at the UHV + oghttp2 case. If you think it impedes readability, I can remove it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nah, totally fine to have it. Wasn't sure if it was useful or just a one-off. Thanks for confirming!

@RyanTheOptimist RyanTheOptimist merged commit 1ea7314 into envoyproxy:main Jan 11, 2025
24 checks passed
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.

3 participants