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

Alternative grouping #1858

Merged
merged 8 commits into from Aug 21, 2021
Merged

Alternative grouping #1858

merged 8 commits into from Aug 21, 2021

Conversation

Serhiy1
Copy link

@Serhiy1 Serhiy1 commented Aug 19, 2021

  • Add an alternative way to group requests together, do deal with situations where passing in a name parameter is not possible
  • Add examples using the alternative grouping approaches.
  • Added an example of session patching when working with SDK's

@@ -1,7 +1,7 @@
.. _testing-other-systems:

========================
Testing non-HTTP systems
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe split this into two sections? ”Testing non-HTTP systems” and ”Testing requests-based SDKs”?

@cyberw
Copy link
Collaborator

cyberw commented Aug 19, 2021

Cool stuff!

  • it needs a unit test
  • it needs formatting fixed (just run Black)
  • I think the field name should be ”request_name” and not ”group_name” Grouping is just one use case for naming - you might do it to clarify or differentiate requests with the same URL. For the context version maybe rename() instead of group()? Or maybe name()? Not as sure about thay one.

@Serhiy1
Copy link
Author

Serhiy1 commented Aug 20, 2021

Cool stuff!

  • it needs a unit test
  • it needs formatting fixed (just run Black)
  • I think the field name should be ”request_name” and not ”group_name” Grouping is just one use case for naming - you might do it to clarify or differentiate requests with the same URL. For the context version maybe rename() instead of group()? Or maybe name()? Not as sure about thay one.

cheers for the feedback, I will do all the items listed out.

@Serhiy1
Copy link
Author

Serhiy1 commented Aug 21, 2021

@cyberw All the suggested changes are done though I'm not 100% happy with the function name for the context manager.

I think maybe instead of name_request changing it to request_name_context will keep it's intention clear and avoid any potential mix-ups with the field name.

@cyberw
Copy link
Collaborator

cyberw commented Aug 21, 2021

Code looks great now! I think you've duplicated the documentation? Also, the new page needs to be added to index.rst to become visible in the left hand menu in the docs.

Maybe rename_request is more clear?

@Serhiy1
Copy link
Author

Serhiy1 commented Aug 21, 2021

  • Rats, you're right about the duplicated documentation.
  • I think I've already added the new page to index.rst? Unless I mis-understood something.
  • And yeah I like rename_request more

Remove duplicated documentation
@cyberw
Copy link
Collaborator

cyberw commented Aug 21, 2021

Looks great!

@cyberw cyberw merged commit c2a6df8 into locustio:master Aug 21, 2021
@Serhiy1 Serhiy1 deleted the Alternatetive-grouping branch August 21, 2021 14:50
@cyberw
Copy link
Collaborator

cyberw commented Aug 21, 2021

Two small issues, now that I see the documentation first hand (https://docs.locust.io/en/latest/)

The title is weird, and there is an extra heading that you should probably remove.

image

@Serhiy1 Serhiy1 restored the Alternatetive-grouping branch August 21, 2021 16:40
@Serhiy1
Copy link
Author

Serhiy1 commented Aug 21, 2021

@cyberw agreed, that doesn't look great.

I've fixed that, and slightly changed the wording, shall I make another PR?

image

@cyberw
Copy link
Collaborator

cyberw commented Aug 21, 2021

Please do! Looks good! I think SDKs is more grammatically correct than SDK's, but that is a detail :)

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.

None yet

2 participants