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

Allow tasks to be declared directly under Locust classes #1304

Merged
merged 13 commits into from
Apr 3, 2020

Conversation

heyman
Copy link
Member

@heyman heyman commented Mar 31, 2020

This PR changes how tasks and TaskSets are declared on Locust classes. It's related to #1264 and an alternative approach to #1274. It sprung from multiple discussions with @cyberw around ways to simplify Locust for very simple cases.

All tests pass, but the documentation would still need updating before this could be merged.

The PR contains the following changes:

  • Let users specify tasks directly under a Locust class, just as one would do it on a TaskSet class. These tasks will get the Locust instance as argument when executed.

    Example:

    class User(Locust):
        @task
        def some_task(self):
            pass

    or:

    class User(Locust):
        tasks = [SomeTaskSet, some_task]

    or as a dict with weights:

    class User(Locust):
        tasks = {SomeTaskSet:1, some_task:3}
  • Removes Locust.task_set from the public API, and instead let users use either the tasks attribute or the @task decorator to declare tasks and/or TaskSets on Locust user classes (see example above).

  • Introduce a Locust.abstract boolean attribute. If it's set to True the Locust class is meant to be used as a base class, and users of that type can not be spawned directly from it. This replaces the current way of considering a Locust class non-abstract if it has a task_set attribute set. (This could have been a separate PR, but it's a fairly small change that is definitely an improvement, and it would have taken quite a bit of time for me to extract those small parts into a separate PR).

* Let users specify tasks directly under a Locust class, just as one would do it on a TaskSet class. These tasks will get the Locust instance as argument when executed.
* Removed Locust.task_set from the public API, and instead let users use either the tasks attribute or the @task decorator.
* Introduce a Locust.abstract boolean attribute. If it's set to True the Locust class is meant to be used as a base class, and users of that type can not be spawned directly from it.
@heyman
Copy link
Member Author

heyman commented Mar 31, 2020

Hm, I meant to create a draft PR until I had updated the docs, and Github doesn't seem to support changing it to draft. Oh well.

@codecov
Copy link

codecov bot commented Mar 31, 2020

Codecov Report

Merging #1304 into master will increase coverage by 0.09%.
The diff coverage is 88.67%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1304      +/-   ##
==========================================
+ Coverage   79.73%   79.82%   +0.09%     
==========================================
  Files          23       23              
  Lines        2092     2112      +20     
  Branches      325      327       +2     
==========================================
+ Hits         1668     1686      +18     
- Misses        338      341       +3     
+ Partials       86       85       -1     
Impacted Files Coverage Δ
locust/contrib/fasthttp.py 88.17% <ø> (ø)
locust/inspectlocust.py 72.72% <0.00%> (-1.56%) ⬇️
locust/main.py 23.20% <ø> (ø)
locust/util/deprecation.py 76.19% <66.66%> (-3.81%) ⬇️
locust/core.py 93.84% <90.81%> (+1.31%) ⬆️
locust/runners.py 75.82% <100.00%> (-0.65%) ⬇️

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 f29e70f...014d9a5. Read the comment docs.

@cyberw
Copy link
Collaborator

cyberw commented Apr 1, 2020

Cool stuff! Looks good! Just one question, in what cases would an abstract locust class have tasks defined?

@heyman
Copy link
Member Author

heyman commented Apr 1, 2020

in what cases would an abstract locust class have tasks defined?

I could see a case where you'd want to simulate two kinds of users that have much behaviour in common. Then that behaviour could be defined in tasks on an abstract base class that two different non-abstract Locust classes inherits from.

Then there's the other way around - that you want a non-abstract Locust class without any tasks pre-declared on the class, and instead populate the tasks attribute at runtime (e.g. in the constructor).

@cyberw
Copy link
Collaborator

cyberw commented Apr 1, 2020

Ignore my last (deleted) comment, I misunderstood you :)

Ok, I understand the use case now. Feels a bit weird, but it is not a big thing. I'm +1!

@cyberw
Copy link
Collaborator

cyberw commented Apr 1, 2020

Though I do have one issue (and I think this was one I raised before): Wont users be confused by the fact that the self-parameter when their "on-locust" tasks are called is a TaskSet instance (made from a the somewhat secret DefaultTaskSet class) and not an instance of their Locust class?

If they are only accessing things like self.client that are just wrapped by TaskSet then it is not so bad, but if they for some reason add an instance property to their Locust class (in __init__ for example) it will not be visible for them.

@heyman
Copy link
Member Author

heyman commented Apr 1, 2020

Wont users be confused by the fact that the self-parameter when their "on-locust" tasks are called is a TaskSet instance (made from a the somewhat secret DefaultTaskSet class) and not an instance of their Locust class?

Very valid concern I think. This code in the DefaultTaskSet class makes sure that tasks declared on the Locust class gets the Locust instance as self-parameter:

locust/locust/core.py

Lines 432 to 438 in af1e5e8

def execute_task(self, task, *args, **kwargs):
if hasattr(task, "tasks") and issubclass(task, TaskSet):
# task is (nested) TaskSet class
task(self.locust).run(*args, **kwargs)
else:
# task is a function
task(self.locust, *args, **kwargs)

@cyberw
Copy link
Collaborator

cyberw commented Apr 1, 2020

Ah, cool, I didnt see that (in fact github hid the whole diff for core.py because it was too big :) )

👍

@cyberw
Copy link
Collaborator

cyberw commented Apr 2, 2020

Dont merge this right now, I think I have found a bug...

@cyberw
Copy link
Collaborator

cyberw commented Apr 2, 2020

For some reason the code now instantiates the base class (e.g. HttpLocust), not the user defined one (MyHttpLocust). So any class variables defined by the user are unavailable.

I couldnt really figure out what is the reason... We should add a runner test case for this I think.

@heyman
Copy link
Member Author

heyman commented Apr 2, 2020

Oh, interesting. Do you have a way to reproduce it?

I'm still working on updating the docs, and won't merge it until that's ready (and also not before the bug is fixed obviously).

@heyman
Copy link
Member Author

heyman commented Apr 3, 2020

All documentation has now been updated to reflect the new API (I think, though I might have missed something). I also did some various improvements to the documentation that I found while going through all of the docs.

I feel ready to merge :)

@cyberw
Copy link
Collaborator

cyberw commented Apr 3, 2020

Two last things...

  • What is the purpose of locust._task_set now? (if it is still intentionally there then maybe document it more clearly)
  • Maybe we should check for a task_set property and thrown an exception if we find one? I think quite a few people will have issues migrating if we dont.

Other than that it looks great!

@heyman
Copy link
Member Author

heyman commented Apr 3, 2020

What is the purpose of locust._task_set now? (if it is still intentionally there then maybe document it more clearly)

True, I see no point in having the ability to change the DefaultTaskSet. Fixed now!

Maybe we should check for a task_set property and thrown an exception if we find one? I think quite a few people will have issues migrating if we dont.

I'm ok with breaking changes for 1.0, but emitting a warning is not much extra job and might be a nice touch for people who don't read the migration guide meticulously, so I added it :).

@heyman heyman merged commit 479aef9 into master Apr 3, 2020
@heyman heyman deleted the tasks-directly-under-locust-class branch April 14, 2020 09:56
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.

2 participants