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.

Sign up for GitHub

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

Refactor HTTP.request() method #574

Closed
na-- opened this issue Apr 5, 2018 · 3 comments
Closed

Refactor HTTP.request() method #574

na-- opened this issue Apr 5, 2018 · 3 comments
Labels
refactor

Comments

@na--
Copy link
Member

na-- commented Apr 5, 2018

It's over 400 lines long... I think that this could be done in conjunction with #562 and/or something else that depends on modifications in request().
@na-- na-- added the refactor label Apr 5, 2018
@na--
Copy link
Member Author

na-- commented Apr 5, 2018

It may also be worth it to refactor some of the other functions with high cyclomatic complexity: https://goreportcard.com/report/github.com/loadimpact/k6#gocyclo

@luizbafilho luizbafilho mentioned this issue Apr 16, 2018
Closed
@na-- na-- mentioned this issue May 18, 2018
Closed
na-- added a commit that referenced this issue May 23, 2018
@na--
Fix multiple bugs in the HTTP request and batch handling
1314bb6 
Included changes:
- Fix #626 - both the original bug with binary HTTP request bodies and the other bug described further down into the issue (incorrectly setting the body of a batch request).
- Fix #598 - the batch data race was because functions in the goja runtime were called concurrently from the different goroutines spawned by the http.batch() call. The fix was to first sequentially parse all of the request options and then execute them concurrently.
- Fix an error where some system tags could be overwritten by user-supplied tags from the script.
- Hopefully slightly ameliorated the situation with #574. There's still a lot of work to be done there... Even though this commit somewhat separates the parsing and execution functions in the HTTP handling code, I don't think the code is much more readable and maintainable than before... I think that having a clear separation between the parsing and validation of JS invocations and the actual execution would help very much, but that would probably take a lot of time and effort to refactor...
@na-- na-- mentioned this issue May 23, 2018
Merged
@robingustafsson robingustafsson added this to the v1.0.0 milestone May 23, 2018
@na--
Copy link
Member Author

na-- commented Sep 3, 2018

Making a note here that we should probably handle the digest HTTP authentication like how we'll handle the NTLM one after this PR is merged, having it as a http.Client.Transport (i.e. RoundTripper) wrapper should make the code a lot more testable and cleaner in general...

@na-- na-- mentioned this issue Sep 13, 2018
Closed
@na--
Copy link
Member Author

na-- commented Oct 9, 2018

I consider this closed after #642 and #753... Any other issues (like the digest authentication refactoring) should be separate issues.

@na-- na-- closed this as completed Oct 9, 2018
@na-- na-- mentioned this issue Oct 9, 2018
Open
@na-- na-- removed this from the v1.0.0 milestone Jul 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
refactor
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@robingustafsson @na--