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.

Sign up for GitHub

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

Jump to bottom

Rename slave to worker (except changelog) #1303

Merged
merged 3 commits into from
Mar 31, 2020
Merged

Rename slave to worker (except changelog) #1303

merged 3 commits into from
Mar 31, 2020

Conversation

anuj-ssharma
Copy link
Contributor

#220 Rename slave to worker

@codecov
Copy link

codecov bot commented Mar 29, 2020 •
edited
Loading

Codecov Report

Merging #1303 into master will increase coverage by 0.08%.
The diff coverage is 51.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1303      +/-   ##
==========================================
+ Coverage   79.60%   79.69%   +0.08%     
==========================================
  Files          23       23              
  Lines        2079     2093      +14     
  Branches      323      325       +2     
==========================================
+ Hits         1655     1668      +13     
+ Misses        342      338       -4     
- Partials       82       87       +5
Impacted Files Coverage Δ
locust/env.py 100.00% <ø> (ø)
locust/main.py 23.20% <6.66%> (-0.40%) ⬇️
locust/web.py 89.33% <16.66%> (ø)
locust/runners.py 76.70% <70.58%> (+0.94%) ⬆️
locust/argument_parser.py 72.04% <100.00%> (+0.93%) ⬆️
locust/event.py 90.62% <100.00%> (ø)
locust/stats.py 88.19% <100.00%> (ø)
locust/contrib/fasthttp.py 88.17% <0.00%> (-0.60%) ⬇️

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 9a09016...904db2a. Read the comment docs.

@anuj-ssharma
Copy link
Contributor Author

UI snapshot

image

image

@anuj-ssharma
Copy link
Contributor Author

I think this is ready for review for now. This is my first PR (still learning Python) please be gentle ;:)

@anuj-ssharma anuj-ssharma marked this pull request as ready for review March 29, 2020 03:42
@cyberw
Copy link
Collaborator

cyberw commented Mar 29, 2020

Looks good to me! I dont have time to test it extensively atm, but I'll merge this tomorrow, if nobody objects (@heyman ?), and do some tests next week.

@cyberw
Copy link
Collaborator

cyberw commented Mar 29, 2020 •
edited
Loading

Could you add a hidden argument (https://stackoverflow.com/questions/11114589/creating-hidden-arguments-with-python-argparse) for —slave that just throws an exception, pointing people to the —worker argument?

@anuj-ssharma
Copy link
Contributor Author

@cyberw Updated, let me know if the exception message makes sense or not.

genev
genev reviewed Mar 31, 2020
View reviewed changes
locust/static/locust.js Outdated
@@ -164,7 +164,7 @@ function updateStats() {
window.report = report;

renderTable(report);
renderSlaveTable(report);
renderworkerTable(report);
Copy link

Choose a reason for hiding this comment

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

Capitalize 'w'

locust/static/locust.js Outdated
renderSlaveTable(window.report);
workerSortAttribute = $(this).attr("data-sortkey");
workerDesc = !workerDesc;
renderworkerTable(window.report);
Copy link

Choose a reason for hiding this comment

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

Capitalize 'w'

locust/static/locust.js Outdated
if (report.slaves) {
var slaves = (report.slaves).sort(sortBy(slaveSortAttribute, slaveDesc));
$("#slaves tbody").empty();
function renderworkerTable(report) {
Copy link

Choose a reason for hiding this comment

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

Capitalize 'w'

locust/test/test_runners.py Outdated
@@ -658,21 +658,21 @@ class MyLocust(Locust):
self.assertEqual(2, exception["count"])


class TestSlaveLocustRunner(LocustTestCase):
class TestworkerLocustRunner(LocustTestCase):
Copy link

Choose a reason for hiding this comment

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

Capitalize 'w'

locust/test/test_runners.py Outdated
def setUp(self):
super(TestSlaveLocustRunner, self).setUp()
super(TestworkerLocustRunner, self).setUp()
Copy link

Choose a reason for hiding this comment

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

Capitalize 'w'

locust/test/test_runners.py Outdated
#self._report_to_master_event_handlers = [h for h in events.report_to_master._handlers]

def tearDown(self):
#events.report_to_master._handlers = self._report_to_master_event_handlers
super(TestSlaveLocustRunner, self).tearDown()
super(TestworkerLocustRunner, self).tearDown()
Copy link

Choose a reason for hiding this comment

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

Capitalize 'w'

locust/web.py Outdated
slave_count=slave_count,
is_step_load=environment.step_load,
)
state=environment.runner.state,
Copy link

Choose a reason for hiding this comment

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

Looks like some unnecessary whitespace here and below?

@cyberw
Copy link
Collaborator

cyberw commented Mar 31, 2020

@cyberw Updated, let me know if the exception message makes sense or not.

LGTM! @genev had some good suggestions too, fix those and I'll merge it!

@cyberw cyberw merged commit f84d76c into locustio:master Mar 31, 2020
@cyberw
Copy link
Collaborator

cyberw commented Mar 31, 2020

Thanks!

@anuj-ssharma anuj-ssharma deleted the rename_slave_to_worker branch March 31, 2020 20:15
@heyman heyman mentioned this pull request Apr 1, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@genev genev genev left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@anuj-ssharma @cyberw @genev