Revisit rebalance unit tests #5697
Conversation
|
All test failures are unrelated; ready for review and merge |
gjoseph92
left a comment
gjoseph92
left a comment
There was a problem hiding this comment.
The switch to managed memory and non-Nanny workers is way nicer to read, big improvement.
|
|
||
|
|
||
| @gen_cluster(client=True, config=REBALANCE_MANAGED_CONFIG) | ||
| async def test_rebalance_managed_memory(c, s, a, b): |
There was a problem hiding this comment.
Isn't this a duplicate of test_client.py::test_rebalance (besides calling rebalance on the client vs the scheduler)? I understand wanting to unit-test the actual rebalance logic on the scheduler, and test that the client is invoking it correctly, but testing so much of the rebalance logic in the client tests too feels a little redundant? (Same goes for other tests here.)
There was a problem hiding this comment.
All tests on test_client specifically test the Client API. It's very easy to accidentally forget a parameter or to convert a None to an empty list (which have different meanings).
Additionally, some behaviour is slightly different on Client.rebalance vs Scheduler.rebalance (not my design):
test_rebalance_unprepared: Client waits for unfinished tasks. Scheduler expects all tasks to be already finished.test_rebalance_raises_on_explicit_missing_data: exception handling is specifically implemented client side
2 participants
Closes #5688
Supersedes #5691
Revisit the test suite around rebalance() to be faster, simpler to maintain, and more robust to variations in unmanaged memory (which is very hard to control).