AWS utils: Add unit tests for restart_workers #2604
Conversation
…dk91ght/EvalAI into aws-utils-test-RestartWorker Merge master
Codecov Report
@@ Coverage Diff @@
## master #2604 +/- ##
=========================================
Coverage ? 73.82%
=========================================
Files ? 82
Lines ? 5338
Branches ? 0
=========================================
Hits ? 3941
Misses ? 1397
Partials ? 0
Continue to review full report at Codecov.
|
nikochiko
changed the title
| end_date=timezone.now() + timedelta(days=1), | ||
| ) | ||
| self.ecs_client.create_cluster(clusterName="cluster") | ||
| self.client_token = "abc123" |
There was a problem hiding this comment.
@nikochiko can we use a randomly generated string here
| queryset = super(TestRestartWorkers, self).queryset(pklist) | ||
|
|
||
| expected_count = 3 | ||
| expected_num_of_workers = [1, 1, 1] |
There was a problem hiding this comment.
Can this 1 be declared as a class variable so that we can use the variable name where needed
There was a problem hiding this comment.
@Ram81 I think it is better if we keep it this way. As it states the number of workers and could be any number (0, 1, 2, 3, etc.) the number of variables would be too much to handle.
What do you think?
There was a problem hiding this comment.
@nikochiko but the number is based on the number of challenges right? so can't we use that to compute the number of workers?
There was a problem hiding this comment.
Also as I can see in the code above you are setting the number of workers for every challenge. So I think we can just create 1 variable with a value that can be used in both setting the number of worker and using that in expected_num_of_workers. Let me know if this makes sense
There was a problem hiding this comment.
@Ram81 Okay, I think that makes sense now. So let me know if I have the right idea:
# in setup
self.no_active_workers = 0
self.one_active_worker = 1
# in tests
self.set_challenge_worker(self.challenge, self.one_active_worker)
expected_num_of_workers = [self.one_active_worker, self.one_active_worker, self.no_active_worker] # For [1, 1, 0] There was a problem hiding this comment.
but the number is based on the number of challenges right? so can't we use that to compute the number of workers?
I'm sorry I didn't get what you meant by computing the number of workers. Can you please elaborate?
There was a problem hiding this comment.
If you are referring to computing the expected_count, I want to clarify that it is only the number of workers successfully restarted and not the number of active workers. It can be different for example in this case: https://github.com/Cloud-CV/EvalAI/pull/2604/files#diff-cc74cf6292b78dc74ea17124c792900eR194-R195
| queryset = super(TestRestartWorkers, self).queryset(pklist) | ||
|
|
||
| expected_count = 2 | ||
| expected_num_of_workers = [0, 1, 1] |
| pklist = [self.challenge.pk, self.challenge2.pk, self.challenge3.pk] | ||
| queryset = super(TestRestartWorkers, self).queryset(pklist) | ||
|
|
||
| expected_count = 2 |
| queryset = super(TestRestartWorkers, self).queryset(pklist) | ||
|
|
||
| expected_count = 3 | ||
| expected_num_of_workers = [1, 1, 1] |
There was a problem hiding this comment.
@nikochiko but the number is based on the number of challenges right? so can't we use that to compute the number of workers?
| queryset = super(TestRestartWorkers, self).queryset(pklist) | ||
|
|
||
| expected_count = 3 | ||
| expected_num_of_workers = [1, 1, 1] |
There was a problem hiding this comment.
Also as I can see in the code above you are setting the number of workers for every challenge. So I think we can just create 1 variable with a value that can be used in both setting the number of worker and using that in expected_num_of_workers. Let me know if this makes sense
| from rest_framework.test import APITestCase, APIClient | ||
|
|
||
| import challenges.aws_utils as aws_utils | ||
| from challenges.models import Challenge |
There was a problem hiding this comment.
@nikochiko please reorder all imports lexicographically. Another thing all from imports should come at last and all the direct import statements should be at start. Your lines 5-7 are correct but all from imports should be considered together to order lexicographically
There was a problem hiding this comment.
Done! 👍 Please see if it is okay now.
…dk91ght/EvalAI into aws-utils-test-RestartWorker Synchronize
|
LGTM 👍 |
|
@yashdusing Could you comment here what the error was with the deploymentController? I can't find your comment. Thanks! |
|
@KhalidRmb I thought it had to do with braces and unpacking. But, after inspecting the logs, I inferred that botocore was being reinstalled to a lower version after awscli was installed (only in Travis). The previous versions of botocore did not have provisions for deploymentController which was causing the issues. |
|
Oh...Thanks for the catch! @yashdusing |
|
@yashdusing Thanks for the fix, nice catch! 👌🎉 |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
This pull request has been automatically closed as there is no further activity. Thank you for your contributions. |
Changes proposed in this PR:
Reference PR: #2413