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

changed min_wait/max_wait into between(min_wait,max_wait) #67

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

edwindj
Copy link

@edwindj edwindj commented Dec 2, 2019

changed min_wait/max_wait into between(min_wait,max_wait)

Description

The min_wait/max_wait don't work with the current locust.io, so I changed them into between(), so
that transformer generates a "locustfile.py" that can be used directly without manual editing.

Types of Changes

  • Bug fix (non-breaking change which fixes an issue)
  • I also updated the tests so that they run fine.

@zincr
Copy link

zincr bot commented Dec 2, 2019

🤖 zincr found 2 problems , 0 warnings

❌ Approvals
❌ Specification
✅ Large Commits
✅ Dependency Licensing

Details on how to resolve are provided below


Approvals

All proposed changes must be reviewed by project maintainers before they can be merged

Not enough people have approved this pull request - please ensure that 2 additional users, who have not contributed to this pull request approve the changes.

  • ❌ 2 additional approvals needed
     

Specification

All pull requests must follow certain rules for content length and form

Please ensure the follow issues are resolved:

  • ✅ Pull Request Title must be atleast 8 characters
  • ✅ Pull Request body must be atleast 8 characters
  • ❌ Pull Request body must contain issue number
     

@tortila
Copy link
Contributor

tortila commented Dec 2, 2019

Hi @edwindj, thanks for your contribution! If I'm not mistaken this change was introduced in locustio/locust#1118 and released in Locust 0.13.0. I noticed that the old min_wait and max_wait is deprecated since that release but - according to the PR - should continue to work with a warning. So I think it's a good idea to eventually move to the new API (which is what you proposed).

There's only one thing that I feel is overlooked - according to the PR the new between function, as well as wait_time take the value in seconds, whereas the old min_wait and max_wait were in milliseconds.

Given that Transformer puts hard-coded values 0 and 1 (making Locust wait between 0 and 10 ms) and doesn't really allow users to change that, I think it might be a good idea to eventually either get rid of this or make into a plugin. @thilp @edwindj @bmaher what do you think?

@edwindj
Copy link
Author

edwindj commented Dec 2, 2019

@tortila you're welcome and thanks for your detailed feedback!
You are right on the issues you mentioned:

  • I did not test it thoroughly, but for me locust 0.13.2 did not work properly with min_wait / max_wait. That is: a warning was indeed emitted, but only the first request/task was processed, not the rest of the tasks.
  • I did not take the switch in the specification from ms to s into account, so maybe this should be hard coded or configurable as you suggest.

Best,

Edwin

@thilp
Copy link
Member

thilp commented Dec 3, 2019

Codacy Here is an overview of what got changed by this pull request:

Issues
======
- Added 1
           

See the complete overview on Codacy

@thilp
Copy link
Member

thilp commented Dec 4, 2019

Given that Transformer puts hard-coded values 0 and 1 (making Locust wait between 0 and 10 ms) and doesn't really allow users to change that, I think it might be a good idea to eventually either get rid of this or make into a plugin. @thilp @edwindj @bmaher what do you think?

In general I'm also enthusiastic towards the idea of extracting features from Transformer's core to plugins.

@edwindj I thank you as well for your contribution. To unblock the CI and the automated quality review, as well as facilitate the creation of a new release, could you please adapt the formatting (make black) and update the Changelog (make prepare-…, probably make prepare-major as this breaks backward-compatibility)? We've documented how to do that in our Contributing docs (starting from point 7); let us know how we can help!

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.

3 participants