Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
fix: only update content-length header if one was present in spy #25920
fix: only update content-length header if one was present in spy #25920
Changes from 9 commits
fc7ac80
a8b0a31
04f03d6
27393b7
721b295
901a1ae
f37a85b
a787a52
c3803a9
2406103
8b702a3
f06c793
b05eda4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Should a test be added here instead of the unit test (or maybe in addition to)?
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.
@mschile as far as I have been able to tell, reproducing this from inside a Cypress test requires that there actually be an API upstream that behaves differently depending on the content-length header - there's no way to test it in isolation that I've found. That was part of why this was so hard for me to identify this as a root cause of an issue I was having in the first place - everything looks untouched from inside the test runner.
The way we originally found this issue was that healthy API endpoints would start replying 400 when spied on by Cypress, and the reason turned out to be that AWS ELB - at least as configured in our case - did not expect the content-length header on a GET request and would reject the requests. For the minimal reproduction linked in the issue, I made a simple HTML document that makes a single fetch call to an Express server that echoes back the request headers. You would basically need to move all that machinery into those tests - the only point at which you can see the issue is from the perspective of the Express server, not Cypress.
I do not believe there is any event you can listen to in Cypress that gets you at the request headers of a spied request at the relevant point of the lifecycle. I agree it would be better if we could integration test this, but I think it may be more trouble than it's worth unless there's a more pure way to see the modified headers without an external server.
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.
Thanks @Bourg, we do start an external server in the driver tests so we should be able to output the content-length header to verify it doesn't get inadvertently added.
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.
I'm going to take a look into this in a few 😃
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.
@mschile I added a simple test in b05eda4. What do you think?
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.
Looks good! You could even use the existing
dump-headers
request if you wanted. Something like: