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

c-hyper: ignore case of hex numbers in tests #6987

Conversation

@divergentdave
Copy link
Contributor

@divergentdave divergentdave commented May 1, 2021

When hyper is used, it emits uppercase hexadecimal numbers for chunked encoding lengths. Without hyper, lowercase hexadecimal numbers are used. This change adds splitpart commands to tests where this is an issue, and normalizes the lengths to lower-case digits.

Nine test cases are affected here. 218 now passes when run with hyper, while 510, 645, 650, 654, 667, 668, 1073, and 1591 are only partially fixed, as they each later fail for other reasons (when run with hyper).

@bagder bagder added the tests label May 1, 2021
@bagder
Copy link
Member

@bagder bagder commented May 1, 2021

Thanks. I suppose this is the best way to fix this issue without any changing code. It's a little bit annoying I think as they regex replacements are very error-prone and break if we change content. Still, I can't think of a way to make the test script do this automatically in a reliable way...

Should we perhaps instead consider uppercase in libcurl too?

diff --git a/lib/transfer.c b/lib/transfer.c
index c31e22e00..2ed3bc35b 100644
--- a/lib/transfer.c
+++ b/lib/transfer.c
@@ -316,11 +316,11 @@ CURLcode Curl_fillreadbuffer(struct Curl_easy *data, size_t bytes,
 
     /* if we're not handling trailing data, proceed as usual */
     if(data->state.trailers_state != TRAILERS_SENDING) {
       char hexbuffer[11] = "";
       hexlen = msnprintf(hexbuffer, sizeof(hexbuffer),
-                         "%zx%s", nread, endofline_native);
+                         "%zX%s", nread, endofline_native);
 
       /* move buffer pointer */
       data->req.upload_fromhere -= hexlen;
       nread += hexlen;
 
@divergentdave
Copy link
Contributor Author

@divergentdave divergentdave commented May 1, 2021

Sure, that sounds good. I'll work up another branch with that change.

@divergentdave
Copy link
Contributor Author

@divergentdave divergentdave commented May 3, 2021

In the course of working on #6996, I ran across the %if/%else%endif directives. I'm going to re-do this branch using those pre-processing directives instead of regular expression substitutions for the sake of argument.

When hyper is used, it emits uppercase hexadecimal numbers for chunked
encoding lengths. Without hyper, lowercase hexadecimal numbers are used.
This change adds preprocessor statements to tests where this is an issue,
and adapts the fixtures to match.
@divergentdave divergentdave force-pushed the divergentdave:hyper-tests-case-normalization branch from 0b58b5e to 18217bb May 4, 2021
@bagder
Copy link
Member

@bagder bagder commented May 4, 2021

Thanks for doing this. Now we have two really good takes to compare and weigh against each other. Thanks for playing along with my curiosity here.

With your update here to instead use the %if approach, I think I actually lean towards this being the nicer take. Because of:

  1. it doesn't change any libcurl code and thus risk surprising some niche users who make wrong assumptions
  2. it doesn't introduce the assumption that hyper and libcurl work the same here, now and in the future

The downside is of course the added conditionals in the tests.

@bagder
Copy link
Member

@bagder bagder commented May 4, 2021

@divergentdave what do you think?

@divergentdave
Copy link
Contributor Author

@divergentdave divergentdave commented May 4, 2021

I agree, I prefer the preprocessor conditionals. Having the condition inline in the protocol section is clearer, and there's no risk of accidentally breaking tests or use cases with intolerant servers.

@bagder bagder closed this in 70cf50f May 4, 2021
@bagder
Copy link
Member

@bagder bagder commented May 4, 2021

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants