tests: fix wrongly used mock #1966
Conversation
8287102 to
e25b987
Compare
Codecov Report
@@ Coverage Diff @@
## master #1966 +/- ##
===========================================
- Coverage 85.63% 53.47% -32.17%
===========================================
Files 183 180 -3
Lines 10469 10437 -32
===========================================
- Hits 8965 5581 -3384
- Misses 1504 4856 +3352
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Latency summaryCurrent PR yields:
Breakdown
Backed by latency-tracking. Further commits will update this comment. |
| @@ -47,3 +47,9 @@ def rm_files(file_paths): | |||
| elif os.path.isdir(file_path): | |||
| shutil.rmtree(file_path, ignore_errors=False, > | |||
|
|
|||
|
|
|||
| def validate_callback(mock, validate_func): | |||
There was a problem hiding this comment.
Could we perhaps create a new instance of Mocker here and avoid spamming it in the code of the tests?
There was a problem hiding this comment.
I don't get the suggestion 🤔
There was a problem hiding this comment.
We could pass mocker instead of mock as an argument and use mocker.patch() or mocker.patch.object.
There was a problem hiding this comment.
Do we need to create m = mocker.Mock() in the body of the test? Can't we create it inside this validate_cb instead, for each situation?
There was a problem hiding this comment.
Yes, because it is send to the Flow as the on_done callback and collects the callback data there.
There was a problem hiding this comment.
we could maybe offer this as a fixture that yields the mock and when finished passes the callback through this validation?
There was a problem hiding this comment.
This would need to be very well documented however
e25b987 to
1897416
Compare
|
I have not a lot of experience around these tests, but could someone check, if my fix for |
2232787 to
51530ea
Compare
This is the right usage of
mocker.Mockwith a validation function. It generates way better tracebacks, when the tests fail.Furthermore, the correct naming would have been
wrapsinstead ofwrap.There are still tests, which do it via calling the mock inside the validation function like
tests/distributed/test_join_local_from_remote/test_integration.py. Anyhow, they work correctly, but don't give a bad traceback. Can be refactored how it is done in this PR.