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

Revert k6/http.head to not taking body as second argument #2402

Merged
merged 5 commits into from
Mar 1, 2022
Merged

Revert k6/http.head to not taking body as second argument #2402

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1ea597f
Revert k6/http.head to not taking body as second argument
mstoykov Feb 28, 2022
d8ab553
Test http.head signature
mstoykov Feb 28, 2022
f7d3eca
k6/http: test Params argument placement for all method helpers
mstoykov Feb 28, 2022
d23fcb0
Add Known bugs to v0.36.0 release notes
mstoykov Feb 28, 2022
872de68
Add comment about the undefined body hack
mstoykov Feb 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion js/modules/k6/http/http.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,10 @@ func (r *RootModule) NewModuleInstance(vu modules.VU) modules.Instance {
args = append([]goja.Value{goja.Undefined()}, args...) // sigh
return mi.defaultClient.Request(http.MethodGet, url, args...)
})
mustExport("head", mi.defaultClient.getMethodClosure(http.MethodHead))
mustExport("head", func(url goja.Value, args ...goja.Value) (*Response, error) {
args = append([]goja.Value{goja.Undefined()}, args...) // sigh
Copy link
Contributor

@codebien codebien Feb 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
args = append([]goja.Value{goja.Undefined()}, args...) // sigh
// It avoid creating dedicated logic reusing the common Request method
// setting the body of the request to null.
args = append([]goja.Value{goja.Undefined()}, args...)

Sorry for seeing it just now, but if we want a comment there then I think we should use the opportunity for explaining why we are doing it.

Copy link
Member

Choose a reason for hiding this comment

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

hmm something like this will be better, I think:

Suggested change
args = append([]goja.Value{goja.Undefined()}, args...) // sigh
// http.head(url, params) doesn't have a body argument, so we add undefined
// as the third argument to http.request(method, url, body, params)
args = append([]goja.Value{goja.Undefined()}, args...) // sigh

Copy link
Member

Choose a reason for hiding this comment

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

+1 to this. I would rephrase it:

It avoids having to create some dedicated logic to reuse the common Request method

return mi.defaultClient.Request(http.MethodHead, url, args...)
})
mustExport("post", mi.defaultClient.getMethodClosure(http.MethodPost))
mustExport("put", mi.defaultClient.getMethodClosure(http.MethodPut))
mustExport("patch", mi.defaultClient.getMethodClosure(http.MethodPatch))
Expand Down
23 changes: 14 additions & 9 deletions js/modules/k6/http/request_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1113,12 +1113,13 @@ func TestRequestAndBatch(t *testing.T) {

t.Run("GET", func(t *testing.T) {
_, err := rt.RunString(sr(`
var res = http.get("HTTPBIN_URL/get?a=1&b=2");
var res = http.get("HTTPBIN_URL/get?a=1&b=2", {headers: {"X-We-Want-This": "value"}});
if (res.status != 200) { throw new Error("wrong status: " + res.status); }
if (res.json().args.a != "1") { throw new Error("wrong ?a: " + res.json().args.a); }
if (res.json().args.b != "2") { throw new Error("wrong ?b: " + res.json().args.b); }
if (res.request.headers["X-We-Want-This"] != "value") { throw new Error("Missing or invalid X-We-Want-This header!"); }
`))
assert.NoError(t, err)
require.NoError(t, err)
assertRequestMetricsEmitted(t, stats.GetBufferedSamples(samples), "GET", sr("HTTPBIN_URL/get?a=1&b=2"), "", 200, "")

t.Run("Tagged", func(t *testing.T) {
Expand All @@ -1136,22 +1137,24 @@ func TestRequestAndBatch(t *testing.T) {
})
t.Run("HEAD", func(t *testing.T) {
_, err := rt.RunString(sr(`
var res = http.head("HTTPBIN_URL/get?a=1&b=2");
var res = http.head("HTTPBIN_URL/get?a=1&b=2", {headers: {"X-We-Want-This": "value"}});
if (res.status != 200) { throw new Error("wrong status: " + res.status); }
if (res.body.length != 0) { throw new Error("HEAD responses shouldn't have a body"); }
if (!res.headers["Content-Length"]) { throw new Error("Missing or invalid Content-Length header!"); }
if (res.request.headers["X-We-Want-This"] != "value") { throw new Error("Missing or invalid X-We-Want-This header!"); }
`))
assert.NoError(t, err)
assertRequestMetricsEmitted(t, stats.GetBufferedSamples(samples), "HEAD", sr("HTTPBIN_URL/get?a=1&b=2"), "", 200, "")
})

t.Run("OPTIONS", func(t *testing.T) {
_, err := rt.RunString(sr(`
var res = http.options("HTTPBIN_URL/?a=1&b=2");
var res = http.options("HTTPBIN_URL/?a=1&b=2", null, {headers: {"X-We-Want-This": "value"}});
if (res.status != 200) { throw new Error("wrong status: " + res.status); }
if (!res.headers["Access-Control-Allow-Methods"]) { throw new Error("Missing Access-Control-Allow-Methods header!"); }
if (res.request.headers["X-We-Want-This"] != "value") { throw new Error("Missing or invalid X-We-Want-This header!"); }
`))
assert.NoError(t, err)
require.NoError(t, err)
assertRequestMetricsEmitted(t, stats.GetBufferedSamples(samples), "OPTIONS", sr("HTTPBIN_URL/?a=1&b=2"), "", 200, "")
})

Expand All @@ -1161,11 +1164,12 @@ func TestRequestAndBatch(t *testing.T) {
// https://tools.ietf.org/html/rfc7231#section-4.3.5
t.Run("DELETE", func(t *testing.T) {
_, err := rt.RunString(sr(`
var res = http.del("HTTPBIN_URL/delete?test=mest");
var res = http.del("HTTPBIN_URL/delete?test=mest", null, {headers: {"X-We-Want-This": "value"}});
if (res.status != 200) { throw new Error("wrong status: " + res.status); }
if (res.json().args.test != "mest") { throw new Error("wrong args: " + JSON.stringify(res.json().args)); }
if (res.request.headers["X-We-Want-This"] != "value") { throw new Error("Missing or invalid X-We-Want-This header!"); }
`))
assert.NoError(t, err)
require.NoError(t, err)
assertRequestMetricsEmitted(t, stats.GetBufferedSamples(samples), "DELETE", sr("HTTPBIN_URL/delete?test=mest"), "", 200, "")
})

Expand All @@ -1177,12 +1181,13 @@ func TestRequestAndBatch(t *testing.T) {
for method, fn := range postMethods {
t.Run(method, func(t *testing.T) {
_, err := rt.RunString(fmt.Sprintf(sr(`
var res = http.%s("HTTPBIN_URL/%s", "data");
var res = http.%s("HTTPBIN_URL/%s", "data", {headers: {"X-We-Want-This": "value"}});
if (res.status != 200) { throw new Error("wrong status: " + res.status); }
if (res.json().data != "data") { throw new Error("wrong data: " + res.json().data); }
if (res.json().headers["Content-Type"]) { throw new Error("content type set: " + res.json().headers["Content-Type"]); }
if (res.request.headers["X-We-Want-This"] != "value") { throw new Error("Missing or invalid X-We-Want-This header!"); }
`), fn, strings.ToLower(method)))
assert.NoError(t, err)
require.NoError(t, err)
assertRequestMetricsEmitted(t, stats.GetBufferedSamples(samples), method, sr("HTTPBIN_URL/")+strings.ToLower(method), "", 200, "")

t.Run("object", func(t *testing.T) {
Expand Down
5 changes: 5 additions & 0 deletions release notes/v0.36.0.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,3 +131,8 @@ export default () => {
- [#2304](https://github.com/grafana/k6/pull/2304) prepared the removal of external dependencies from k6's JSONAPI compliant REST API, and deprecated the `api.v1`'s `client.Call` method in favor of its newer `client.CallAPI` counterpart. It allows us to both reduce our reliance on external dependencies and improve its maintainability.
- We have updated our [Goja](https://github.com/dop251/goja) dependency, our JS interpreter, to its latest available version. Unfortunately, some of the new features are not always usable, yet. Namely, Goja now supports the [optional chaining](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Optional_chaining) syntax, but the Babel version we use presently does not. Which means that if Babel needs to be used, optional chaining can't be. See [#2317](https://github.com/grafana/k6/pull/2317) and [#2238](https://github.com/grafana/k6/pull/2238).
- Thanks to @knittl, [#2312](https://github.com/grafana/k6/pull/2312) upgraded [loadimpact/k6](https://hub.docker.com/r/loadimpact/k6) docker image base to Alpine *3.15*.


# Known Bugs

- [#2226](https://github.com/grafana/k6/pull/2226) introduced an unintended breaking change to `http.head()`. The signature in k6 v0.35.0 was `http.head(url, [params])` and was inadvertently changed to `http.head(url, [body], [params])` in v0.36.0. That change will be reverted in k6 v0.37.0, but until then, we suggest users use the stable `http.request('HEAD', url, null, params)` API for HTTP HEAD requests that need to specify custom [parameters](https://k6.io/docs/javascript-api/k6-http/params). Thanks, @[grantyoung](https://github.com/grantyoung), for reporting the problem ([#2401](https://github.com/grafana/k6/issues/2401))!