[ArjitGopher]

What?

This PR aims to solve the issue #2823

Why?

1. The changes made will warn the user if more than two arguments are passed in http.get and http.head method
2. A helper function is added.

[CLAassistant]

All committers have signed the CLA.

[joanlopez]

Suggested change

	validateArgCount := func(methodName string,args ...sobek.Value){
	if len(args)>2 {
	vu.State().Logger.Warningf("%s method has more than two arguments",methodName)
	}
	}
	validateArgCount := func(methodName string,args ...sobek.Value){
	if len(args)>2 {
	vu.State().Logger.Warnf("http.%s method has more than two arguments",methodName)
	}
	}

Small suggestion

[olegbespalov]

I do agree with @joanlopez that it's better to add http prefix.

Also, how about changing the message to something more actionable, like http.%s requires two arguments, but %d were provided. Please adjust the input.

[joanlopez]

Looks like the new code is not correctly formatted, @ArjitGopher could you run the linter and fix the related issues, please?

Beyond that, I only have a small suggestion because I think these functions are usually called like http.get(...), as suggested in the official docs, and in any case, it's always good idea to disambiguate.

Other than that, it looks good! 👍🏻
cc/ @olegbespalov

[olegbespalov]

Thanks for the contribution!

I do agree with what @joanlopez said, also have a minor suggestion for the actual message

[olegbespalov]

I do agree with @joanlopez that it's better to add http prefix.

Also, how about changing the message to something more actionable, like http.%s requires two arguments, but %d were provided. Please adjust the input.

[olegbespalov]

Hi @ArjitGopher. Out of curiosity, are you going to apply feedback, or you have no capacity of doing that?

[ArjitGopher]

Hi @olegbespalov I agree with the suggestions and will add them today, Thanks

[joanlopez]

Nit; I'd prefer if we can use the "actionable" version of the message suggested by @olegbespalov 👉🏻 here.

[joanlopez]

Suggested change

	// http.get(url, params) doesn't have a body argument, so we add undefined
	// as the third argument to http.request(method, url, body, params)
	// get method should not have more than two arguments
	validateArgCount("get", args...)
	args = append([]sobek.Value{sobek.Undefined()}, args...)
	return mi.defaultClient.Request(http.MethodGet, url, args...)
	// get method should not have more than two arguments
	validateArgCount("get", args...)
	// http.get(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([]sobek.Value{sobek.Undefined()}, args...)
	return mi.defaultClient.Request(http.MethodGet, url, args...)

Nit; I'd prefer if we can keep the order as suggested here, as I think the already existing comment is tied to the args slice we build below, so it kinda makes more sense to keep both together. Same for the "head" method.

Could you confirm it, please @olegbespalov?

[olegbespalov]

I've tested and, checked the UI and actually spotted a bug 😢

So my script was:

import http from "k6/http";

export default function () {
	http.get("https://test-api.k6.io/", {}, "lorem")
}

and it didn't produce any warning

When I re-looked into the code I found that validateArgCount doesn't get all arguments, it just gets all arguments, except url which is separated in the signatures.

So to make it work as expected, we need to adjust the logical expression inside validate, like len(args) > 1, but I'd also ask adjusting both comments (made inline suggestion)

[olegbespalov]

Suggested change

	// get method should not have more than two arguments
	// get method should not have more than one additional arguments

[olegbespalov]

As the team, we decided to close this PR for now. Happy to return to it if feedback will be addressed.