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

Extend info about group with an antipattern section #210

Merged
merged 4 commits into from
Feb 5, 2021

Conversation

ppcano
Copy link
Collaborator

@ppcano ppcano commented Feb 3, 2021

Discourage the usage of group in some cases and provide alternatives.

@ppcano ppcano requested a review from nicolek6 February 3, 2021 08:08
@github-actions
Copy link
Contributor

github-actions bot commented Feb 3, 2021

There's a version of the docs published here:

https://mdr-ci.staging.k6.io/docs/refs/pull/210/merge

It will be deleted automatically in 30 days.

Comment on lines 27 to 34
group('add several products to the shopping cart', function () {
// ...
});
group('go to login and authenticate', function () {
// ...
});
group('checkout process', function () {
// ...
Copy link
Contributor

Choose a reason for hiding this comment

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

I am for at least one example with nested groups, but it should probably be not an anti-pattern one ;)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I intentionally removed it to simplify the example.

Not saying that is the best option.

It is mentioned in the description.

Copy link

Choose a reason for hiding this comment

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

Should the content of the groups be more granular? Those groups could involve many actions/requests that aren't necessarily similar, so I'm not sure why someone would group them together.

"Add several products to the shopping cart" could be interpreted to mean going to many pages, such as individual product pages, in addition to the POST for adding to the cart. "Go to login and authenticate" could mean both the login page and the actual submission of credentials. "Checkout process" could be several pages including the checkout button, a call to an external payment system, and filling out address details.

Personally, I would probably use a group for all the resources on a single page. I would not combine requests from more than one page or action in a group. Then, if I needed a higher-level group, instead of nesting groups I would use a function.

Copy link

@nicolek6 nicolek6 left a comment

Choose a reason for hiding this comment

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

I agree that it doesn't make sense to have a group for just one request. But I think we should recommend using groups for multiple requests that are still a result of only one page/action. For those spanning more than one action, I think we should recommend functions rather than groups.

Comment on lines 27 to 34
group('add several products to the shopping cart', function () {
// ...
});
group('go to login and authenticate', function () {
// ...
});
group('checkout process', function () {
// ...
Copy link

Choose a reason for hiding this comment

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

Should the content of the groups be more granular? Those groups could involve many actions/requests that aren't necessarily similar, so I'm not sure why someone would group them together.

"Add several products to the shopping cart" could be interpreted to mean going to many pages, such as individual product pages, in addition to the POST for adding to the cart. "Go to login and authenticate" could mean both the login page and the actual submission of credentials. "Checkout process" could be several pages including the checkout button, a call to an external payment system, and filling out address details.

Personally, I would probably use a group for all the resources on a single page. I would not combine requests from more than one page or action in a group. Then, if I needed a higher-level group, instead of nesting groups I would use a function.

@ppcano
Copy link
Collaborator Author

ppcano commented Feb 5, 2021

@nicolek6 Please, let me know if the latest changes address your points. You can read it https://mdr-ci.staging.k6.io/docs/refs/pull/210/merge/using-k6/tags-and-groups

@simskij
Copy link
Contributor

simskij commented Feb 5, 2021

Nothing more from me. I'll leave the approval to @mstoykov or @nicolek6 as they both have unresolved discussions in their reviews. 👍🏼

Copy link

@nicolek6 nicolek6 left a comment

Choose a reason for hiding this comment

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

I really like the new changes, Pepe. I think it reads much better!

@ppcano ppcano merged commit 00df5fb into master Feb 5, 2021
@simskij simskij deleted the groups-antipattern branch February 5, 2021 09:40
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.

5 participants