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

Jump to bottom

hyper tests are broken since ca15b7512e #10009

Closed
bagder opened this issue Dec 1, 2022 · 9 comments
Closed

hyper tests are broken since ca15b7512e #10009

bagder opened this issue Dec 1, 2022 · 9 comments
Assignees
@bagder
Labels
Hyper tests

Comments

@bagder
Copy link
Member

bagder commented Dec 1, 2022

I did this

Build curl with hyper

cd tests
./runtests.pl 1

Problem

I think the new crlf="yes" support from commit ca15b75 should only be applied to the section where it is used, and not on the entire test case like it is now. Test 2500 only uses it for <data> so only that content should get crlf replaced.
@bagder
Copy link
Member Author

bagder commented Dec 1, 2022

The conversion could then be done explicitly without caring if it looks like HTTP or not.

@bagder bagder assigned bagder and unassigned icing Dec 1, 2022
bagder added a commit that referenced this issue Dec 1, 2022
@bagder
runtests: do CRLF replacements per section only
fb8bd79 
The `crlf="yes"` attribute and "hyper mode" are now only applied on a
subset of dedicated sections: data, datacheck, stdout and protocol.

Updated test 2500 accordingly.

Also made test1 use crlf="yes" for <protocol>, mostly because it is
often used as a template test case. Going forward, using this attribute
we should be able to write test cases using linefeeds only and avoid
mixed line ending encodings.

Fixes #10009
@bagder bagder mentioned this issue Dec 1, 2022
Closed
@bagder
Copy link
Member Author

bagder commented Dec 1, 2022

The conversion could then be done explicitly without caring if it looks like HTTP or not.

Eh, no. Because then we also add CRLF to the body in <data> etc and that changes the size of it...

@icing
Copy link
Contributor

icing commented Dec 1, 2022

The conversion could then be done explicitly without caring if it looks like HTTP or not.

Eh, no. Because then we also add CRLF to the body in <data> etc and that changes the size of it...

I remember myself walking down that line of thinking. That is why I sticked to the HTTP pattern that was in place for hyper. The trouble we have now is that it also replaces \r\n with \r\r\n, it seems.

@bagder
Copy link
Member Author

bagder commented Dec 1, 2022

I'm trying out a few things in #10010. I want to be able to mark individual sections as crlf="yes" so that we long-term can move away from using CRLF lines within the source test files .

@icing
Copy link
Contributor

icing commented Dec 1, 2022

Another idea would be to have some sort of <reply><http-header>... section where crlf is always normalized.

@bagder
Copy link
Member Author

bagder commented Dec 1, 2022

Or just a <crlf>...</crlf> within <data> etc

@bagder
Copy link
Member Author

bagder commented Dec 1, 2022

A challenge is to introduce such a new/better/different system in a way that can be gradually transitioned into, without us having to update 800+ test cases in a single blow.

@icing
Copy link
Contributor

icing commented Dec 1, 2022

I was thinking of gradually changing test cases to use the new section. The old way of setting the server response could be left unchanged. But that is just a thought, you know best.

@bagder
Copy link
Member Author

bagder commented Dec 1, 2022

If I can get this working with my current simpler approach, I think I'll stick to this for now.

bagder added a commit that referenced this issue Dec 1, 2022
@bagder
runtests: do CRLF replacements per section only
2304f3a 
The `crlf="yes"` attribute and "hyper mode" are now only applied on a
subset of dedicated sections: data, datacheck, stdout and protocol.

Updated test 2500 accordingly.

Also made test1 use crlf="yes" for <protocol>, mostly because it is
often used as a template test case. Going forward, using this attribute
we should be able to write test cases using linefeeds only and avoid
mixed line ending encodings.

Fixes #10009
@bagder bagder closed this as completed in 2f34a73 Dec 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@bagder bagder

Labels
Hyper tests
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

runtests: do CRLF replacements per section only curl/curl
2 participants
@icing @bagder