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

Restrict @requiresLength request tests to clients #1471

Closed
wants to merge 1 commit into from
Closed

Restrict @requiresLength request tests to clients #1471

wants to merge 1 commit into from

Conversation

jjant
Copy link
Contributor

@jjant jjant commented Oct 27, 2022

Issue #1086

Description of changes:
As discussed in the linked issue, these tests only apply to clients.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

As discussed in #1086, these tests only apply to clients.
@mtdowling
Copy link
Member

IIRC, we want the server to validate that there's a content-length I thought, which means these wouldn't be client only.

@jjant
Copy link
Contributor Author

jjant commented Oct 28, 2022

@mtdowling We do want to validate that, but that should be a httpMalformedRequestTests suite.
The request in these tests don't actually carry the Content-Length header, so we can't really test anything on the server.

@cmoher cmoher requested review from AndrewFossAWS and a team December 14, 2022 14:52
@JordonPhillips
Copy link
Contributor

From the server perspective this tests that whatever you're doing under the hood with content length doesn't fail on a valid length. It can also test that you've exposed it somewhere. In the typescript ssdk, for example, this could be exposed in the context object they pass along. Or it could be a property of whatever you wrap the http request body with.

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.

4 participants