-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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: elevate common http sequences to common #32766
Conversation
There are several instances of data download sequences in test cases. Defined an abstraction for it in common module for easy consumption.
There are many test cases that will benefit from this, but I changed only one test case now, so that any feedback can be incroporated. |
@HarshithaKP Did you see #31968? That PR does something similar, and there is some relevant discussion in it. I would not land this unless the people who object to it are okay with this PR. Also, the particular example here doesn’t follow best practices – when a stream body is consumed by concatenating strings, you should always use |
Thanks @addaleax, I did not know about that PR.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we add this, it will need to be documented in test/common/README.md
.
receive()
is an insufficiently descriptive name for the function if we're going to add it to common
.
I don't want to seem like I'm moving the goalposts, so I'll add that I have other reasons I'm hesitant to land stuff like this. Those two things above are the ones I don't think anyone would disagree with. However, in general, I don't think we should be adding things to |
Closing based on feedback that add things to common only when the case is overwhelming. |
There are several instances of data download sequences in test
cases. Defined an abstraction for it in common module for easy
consumption.
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes