#1289: Error will be displayed that only 1 array is allowed in batch #2585

vanshaj · 2022-07-04T12:09:53Z

Updated Batch method to contain array of goja.Value and if the size is greater than 1 then return and error. Else continue with the data.

… batch

CLAassistant · 2022-07-04T12:09:59Z

All committers have signed the CLA.

codebien

Hi @vanshaj, LGTM. Can you sign the CLA so we can accept your contribution, please?

js/modules/k6/http/request.go

olegbespalov · 2022-07-05T06:33:29Z

Hi @vanshaj !

Thanks for your contribution! It seems like the changes affected other tests. Could you please have a look?

Cheers!

…idation

vanshaj · 2022-07-05T08:15:06Z

Hi @vanshaj !

Thanks for your contribution! It seems like the changes affected other tests. Could you please have a look?

Cheers!

Sorry for the failures. I have added the validation for init context above my new validation.

Pushed the changes

Regards
Vanshaj

codecov-commenter · 2022-07-05T08:30:33Z

Codecov Report

Merging #2585 (0c8de0c) into master (7f823f0) will decrease coverage by 0.55%.
The diff coverage is 65.50%.

❗ Current head 0c8de0c differs from pull request most recent head 88b64f7. Consider uploading reports for the commit 88b64f7 to get more accurate results

@@            Coverage Diff             @@
##           master    #2585      +/-   ##
==========================================
- Coverage   75.59%   75.03%   -0.56%     
==========================================
  Files         202      203       +1     
  Lines       15992    16274     +282     
==========================================
+ Hits        12089    12212     +123     
- Misses       3151     3296     +145     
- Partials      752      766      +14

Flag	Coverage Δ
ubuntu	`75.03% <65.50%> (-0.36%)`	⬇️
windows	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
errext/exit_code.go	`100.00% <ø> (ø)`
js/common/interrupt_error.go	`100.00% <ø> (+18.18%)`	⬆️
js/compiler/compiler.go	`64.15% <ø> (ø)`
js/modules/k6/grpc/client.go	`78.39% <0.00%> (ø)`
js/timeout_error.go	`56.25% <ø> (ø)`
lib/consts/consts.go	`78.57% <ø> (ø)`
metrics/metric.go	`92.40% <0.00%> (+3.38%)`	⬆️
output/csv/time_format_gen.go	`20.00% <20.00%> (ø)`
metrics/thresholds.go	`88.54% <33.33%> (-0.18%)`	⬇️
...utils/httpmultibin/grpc_any_testing/any_test.pb.go	`37.97% <37.97%> (ø)`
... and 42 more

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 e7c2aee...88b64f7. Read the comment docs.

js/modules/k6/http/request_test.go

js/modules/k6/http/request.go

codebien · 2022-07-05T08:51:13Z

js/modules/k6/http/request.go

+	if len(reqsValues) == 0 {
+		return nil, fmt.Errorf("no argument was provided to http.batch()")
+	} else if len(reqsValues) > 1 { // Validation for http.batch(["https://validurl"],["https://validurl"])
+		return nil, fmt.Errorf("http.batch() must have only one single array as an argument")


Suggested change

return nil, fmt.Errorf("http.batch() must have only one single array as an argument")

return nil, fmt.Errorf("http.batch() must have only one single array as a root argument. Define the requests as nested items.")

@olegbespalov WDYT? Do you have better suggestions? I would be very clear here because I think it's easy to waste time making the error to forget the root array.

Suggested change

return nil, fmt.Errorf("http.batch() must have only one single array as an argument")

return nil, fmt.Errorf("http.batch() accepts only an object/array of requests.")

Maybe something like this?

@codebien @vanshaj WDYT?

Suggested change

return nil, fmt.Errorf("http.batch() must have only one single array as an argument")

return nil, fmt.Errorf("http.batch() accepts only an array or an object of requests.")

@olegbespalov your version sounds better, I would just avoid the slash.

Done updated to "http.batch() accepts only an array or an object of requests." in file and test case also .

…ted test name

codebien

LGTM, just some minors improvements

codebien · 2022-07-05T13:21:18Z

js/modules/k6/http/request.go

@@ -437,21 +437,25 @@ func (c *Client) prepareBatchObject(requests map[string]interface{}) (

 // Batch makes multiple simultaneous HTTP requests. The provideds reqsV should be an array of request
 // objects. Batch returns an array of responses and/or error
-func (c *Client) Batch(reqsV goja.Value) (interface{}, error) {
+func (c *Client) Batch(reqsValues ...goja.Value) (interface{}, error) {


Suggested change

func (c *Client) Batch(reqsValues ...goja.Value) (interface{}, error) {

func (c *Client) Batch(reqsV ...goja.Value) (interface{}, error) {

Removing the line after we can restore the name so we don't need to update the comment.

Thanks a lot for your valuable feedback. Implemented the change.

codebien · 2022-07-05T13:22:36Z

js/modules/k6/http/request.go

+		return nil, fmt.Errorf("http.batch() accepts only an array or an object of requests")
+	}
+
+	reqsV := reqsValues[0]


Suggested change

reqsV := reqsValues[0]

This change doesn't seem required, so we can avoid one more allocation, you can use directly reqsV[0].Export()

Thanks a lot for your valuable feedback. Implemented the change.

olegbespalov

LGTM! Thanks for your contribution! 👍

codebien

LGTM! Hey @vanshaj, we are going to merge this, thanks for your contribution. 👏 🎉

vanshaj · 2022-07-06T15:28:58Z

LGTM! Hey @vanshaj, we are going to merge this, thanks for your contribution. 👏 🎉

Thanks a lot, very glad for your review and valuable feedback. Will keep on contributing

grafana#1289: Error will be displayed that only 1 array is allowed in…

b4d104b

… batch

github-actions bot requested review from codebien and olegbespalov July 4, 2022 12:10

na-- added this to the v0.40.0 milestone Jul 4, 2022

na-- linked an issue Jul 4, 2022 that may be closed by this pull request

http.batch() should not accept more than one argument #1289

Closed

vanshaj added 2 commits July 4, 2022 18:12

grafana#1289: Removed unit test

09baf97

grafana#1289: Lint fix

15e6005

codebien requested changes Jul 4, 2022

View reviewed changes

js/modules/k6/http/request.go Outdated Show resolved Hide resolved

k6-1289: Added test case for the scenario

5ce2f64

vanshaj requested a review from codebien July 5, 2022 05:15

grafana#1289: Moved Init context validation before multiple array val…

9744915

…idation

codebien requested changes Jul 5, 2022

View reviewed changes

grafana#1289: Updated error message, removed unused comments and upda…

592e07b

…ted test name

vanshaj requested a review from codebien July 5, 2022 10:17

grafana#1289: Fixed lint issue of having punctuation at the end

ffe5fd1

codebien requested changes Jul 5, 2022

View reviewed changes

grafana#1289: Removed extra parameter as per feedback

44b3c82

vanshaj requested a review from codebien July 6, 2022 05:37

grafana#1289: Ran go fmt on request.go

88b64f7

olegbespalov approved these changes Jul 6, 2022

View reviewed changes

codebien mentioned this pull request Jul 6, 2022

Batch improvements grafana/k6-docs#720

Merged

codebien approved these changes Jul 6, 2022

View reviewed changes

codebien merged commit 8cf459f into grafana:master Jul 6, 2022

olegbespalov mentioned this pull request Aug 31, 2022

v0.40.0 release notes #2669

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#1289: Error will be displayed that only 1 array is allowed in batch #2585

#1289: Error will be displayed that only 1 array is allowed in batch #2585

vanshaj commented Jul 4, 2022

CLAassistant commented Jul 4, 2022 •

edited

Loading

codebien left a comment

olegbespalov commented Jul 5, 2022

vanshaj commented Jul 5, 2022

codecov-commenter commented Jul 5, 2022 •

edited

Loading

codebien Jul 5, 2022

olegbespalov Jul 5, 2022

codebien Jul 5, 2022

vanshaj Jul 5, 2022

codebien left a comment

codebien Jul 5, 2022

vanshaj Jul 6, 2022

codebien Jul 5, 2022

vanshaj Jul 6, 2022

olegbespalov left a comment

codebien left a comment

vanshaj commented Jul 6, 2022

	return nil, fmt.Errorf("http.batch() must have only one single array as an argument")
	return nil, fmt.Errorf("http.batch() must have only one single array as a root argument. Define the requests as nested items.")

	return nil, fmt.Errorf("http.batch() must have only one single array as an argument")
	return nil, fmt.Errorf("http.batch() accepts only an object/array of requests.")

	func (c *Client) Batch(reqsValues ...goja.Value) (interface{}, error) {
	func (c *Client) Batch(reqsV ...goja.Value) (interface{}, error) {

#1289: Error will be displayed that only 1 array is allowed in batch #2585

#1289: Error will be displayed that only 1 array is allowed in batch #2585

Conversation

vanshaj commented Jul 4, 2022

CLAassistant commented Jul 4, 2022 • edited Loading

codebien left a comment

Choose a reason for hiding this comment

olegbespalov commented Jul 5, 2022

vanshaj commented Jul 5, 2022

codecov-commenter commented Jul 5, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codebien left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olegbespalov left a comment

Choose a reason for hiding this comment

codebien left a comment

Choose a reason for hiding this comment

vanshaj commented Jul 6, 2022

CLAassistant commented Jul 4, 2022 •

edited

Loading

codecov-commenter commented Jul 5, 2022 •

edited

Loading