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

Add ack for worker connection #2077

Merged
merged 9 commits into from
Jun 1, 2022

Conversation

Nosibb
Copy link

@Nosibb Nosibb commented Apr 21, 2022

adding test for worker connection.
Details is issue: #2044

@cyberw
Copy link
Collaborator

cyberw commented Apr 21, 2022

@heyman What do you think?

Would it be possible to make a test case that really tests the functionality? (maybe by patching master to discard the first connect message)

Is this backwards compatible, assuming an old worker discards the ack? I'm thinking mainly of other worker implementations like @myzhan 's Boomer

@Nosibb
Copy link
Author

Nosibb commented Apr 21, 2022

it requires fixes for sure. I'm working on it

@cyberw
Copy link
Collaborator

cyberw commented May 2, 2022

@Nosibb If you can address the stuff in my comment I can probably merge this.

@cyberw
Copy link
Collaborator

cyberw commented May 19, 2022

@Nosibb ?

@Nosibb
Copy link
Author

Nosibb commented May 20, 2022

@cyberw
I remember about this. I was busy :) I'll finish it next week

@cyberw
Copy link
Collaborator

cyberw commented May 20, 2022

No rush, I just wanted to make sure you were't waiting for me or something :)

@Nosibb
Copy link
Author

Nosibb commented May 25, 2022

Let me know what do you think about the new test.
In my opinion, it is backwards compatible. In older versions, we will see a warning. Is it enough?

@@ -3077,6 +3094,7 @@ def my_task(self):

with mock.patch("locust.rpc.rpc.Client", mocked_rpc()) as client:
environment = Environment()
client.mocked_send(Message("ack", {}, "dummy_client_id"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

this kind of bloats all the tests, can it be simplified?

Copy link
Author

Choose a reason for hiding this comment

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

you are right. It is now simplified

@cyberw
Copy link
Collaborator

cyberw commented May 27, 2022

@heyman Any comments? Looks reasonable to me but I didnt originally write the connection logic.

I’d prefer the constants be named CONNECT_* instead of CONNECTION_* (to indicat that they relate to connection setup, not once connection has been enabled)

@cyberw cyberw merged commit 5514930 into locustio:master Jun 1, 2022
@cyberw
Copy link
Collaborator

cyberw commented Jun 1, 2022

Thx! I’ll make sure to bump the minor version when I do the next release.

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