-
Notifications
You must be signed in to change notification settings - Fork 3k
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
fix: edit load test missing the userclasses data #2171
Conversation
Does that work though? If you change the selected Users, that wont affect the ones that are already running, right? |
Yes,I had tested from my machine, It works as expected. It only reused the selected Users we had picked previously. |
Nice! Can you add a unit test that validates it? Here’s some inspiration: locust/locust/test/test_web.py Line 410 in 4989f2a
|
locust/web.py
Outdated
@@ -164,7 +164,8 @@ def swarm() -> Response: | |||
user_classes[user_class_name] = user_class_object | |||
|
|||
else: | |||
user_classes = self.environment.available_user_classes | |||
user_classes = {key: value for (key, value) in self.environment.available_user_classes.items if |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried out this change and got
File "/Users/michaelnester/git/locust/locust/web.py", line 167, in swarm
user_classes = {
TypeError: 'builtin_function_or_method' object is not iterable
I think it needs to be self.environment.available_user_classes.items()
, which worked for me (using python 3.9)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, sorry. My fault typo error. python 3.10 works well from myside, but I think it's better to take use the code you corrected here.
This change breaks The workflow is:
Only the user class from the first run will be used |
@alterhu2020 I think i got a fix for this: master...mikenester:locust:fix-class-picker-bug-edit-running-test |
Yes, the code is better than mine, please go ahead merged it. thanks |
@alterhu2020 If you merge this alterhu2020#1, it should automatically update this PR and run the tests |
BTW, @mikenester
I know it related that we use the relative import path, but I don't know how to bypass it? Have we any guideline that can help people quick to setup the develop contribution environment? thanks |
…ng-test Added check to swarm for class picker. Uses pre selected user classes if test is already running
LGTM, Done as well |
Nice! But the test doesnt actually validate that the Users changed, does it? |
Perhaps I misunderstood, but I thought we don't want the users to change. So an example of the expected workflow should be:
|
Yes. I think @mikenester is right. @cyberw for the Users Changed scenario I think It related to another feature I raised in issue: #2169, right? If we can support that I think it's a great improvement. |
Not sure if I can help with this one. I don't think you'll be able to successfully run individual files, since relative import in python can get broken in that scenario. In terms of testing, I personally modify
to the intended unittest path:
Then you can run:
|
Thanks the tips, but how we can active the debugger, I want to read the code step by step... |
I misunderstood what you were trying to do, sorry :) This looks good to me now. Should I merge? |
Yeah, I think it's good to go 👍 |
thanks! |
fix the problem userclasses missing issue when edit the load test