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

Always set the correct content-length for requests #1106

Merged
merged 2 commits into from
Aug 8, 2019

Conversation

na--
Copy link
Member

@na-- na-- commented Aug 7, 2019

This would restore the behavior pre-#989, so that any content-length header values manually specified by users would be overwritten by k6. Only, this time it won't be done silently, a warning would be shown every time there was a mismatch.

The HAR converter is also modified to not include the content-length header in the generated sources. While that header isn't going to be a problem if its values matches the actual body content length, some HAR files are broken and include the header without any body data, and other times users may modify the body but forget the header. So, since k6 would automatically send it, we shouldn't include it in the script.

This will close #1094

This would restore the behavior pre-#989, so that any content-length header values manually specified by users would be overwritten by k6. Only, this time it won't be done silently, a warning would be shown every time there was a mismatch.

The HAR converter is also modified to not include the content-length header in the generated sources. While that header isn't going to be a problem if its values matches the actual body content length, some HAR files are broken and include the header without any body data, and other times users may modify the body but forget the header. So, since k6 would automatically send it, we shouldn't include it in the script.
@na-- na-- requested a review from mstoykov August 7, 2019 15:01
Copy link
Contributor

@mstoykov mstoykov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! While I would like a the test to utilize the logrus hook I am fine with merging it this way.

@codecov-io
Copy link

codecov-io commented Aug 7, 2019

Codecov Report

Merging #1106 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1106      +/-   ##
==========================================
+ Coverage   73.26%   73.26%   +<.01%     
==========================================
  Files         141      141              
  Lines       10251    10252       +1     
==========================================
+ Hits         7510     7511       +1     
  Misses       2300     2300              
  Partials      441      441
Impacted Files Coverage Δ
lib/netext/httpext/request.go 93.3% <100%> (+0.02%) ⬆️
converter/har/converter.go 64.36% <100%> (ø) ⬆️
lib/netext/httpext/tracer.go 96.52% <0%> (-1.74%) ⬇️
core/engine.go 93.92% <0%> (+0.93%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8e93258...7c32cc5. Read the comment docs.

@na-- na-- changed the title [WIP] Always set the correct content-length for requests Always set the correct content-length for requests Aug 8, 2019
@na--
Copy link
Member Author

na-- commented Aug 8, 2019

There was a minor but annoying mistake in one of the warning messages, so I'm glad I did the tests... Will merge when the build passes, which now shouldn't require 10 rebuilds 🎉

@na-- na-- merged commit 350729e into master Aug 8, 2019
@na-- na-- deleted the fix-content-length-mismatch-error branch August 8, 2019 07:17
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.

Wrong content-length header behaves differently for HTTP 1 and 2 requests
3 participants