-
Notifications
You must be signed in to change notification settings - Fork 284
Prevent output paths from being nested on task restart #5017
Conversation
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.
Looks good, but I'd like to see a unit test for that, because it feels like someone could unknowingly break at any time.
@etam Agreed, working on it now. Probably should've marked this PR a draft, sorry about that. |
client=self.client, | ||
old_task_id=self.task_id, | ||
task_dict=mock.ANY, | ||
subtask_ids_to_copy={'finished-subtask-id'}, | ||
ignore_gas_price=ignore_gas_price | ||
) | ||
|
||
task_dict = restart_mock.call_args[1]['task_dict'] | ||
output_path = task_dict['options']['output_path'] | ||
self.assertEqual(output_path, task_output_path) |
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.
undeclared variable task_output_path
?
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.
It's declared as top-level. I think the reason for this is to have a single test directory created for all tests from this file. If we were to change this then it could be moved into ProviderBase
. This would result in each test class having a separate temp directory.
@@ -764,6 +774,10 @@ def test_task_inactive(self, validate_funds_mock, restart_mock, *_): | |||
ignore_gas_price=ignore_gas_price | |||
) | |||
|
|||
task_dict = restart_mock.call_args[1]['task_dict'] | |||
output_path = task_dict['options']['output_path'] | |||
self.assertEqual(output_path, task_output_path) |
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.
undeclared variable task_output_path
?
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.
See answer above.
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.
ok
Codecov Report
@@ Coverage Diff @@
## b0.22 #5017 +/- ##
==========================================
+ Coverage 90.05% 90.08% +0.02%
==========================================
Files 234 234
Lines 21950 21954 +4
==========================================
+ Hits 19768 19777 +9
+ Misses 2182 2177 -5 |
Fixes #4986