Fix handling of auth data dictionary in FastPurgeClient constructor #21
Conversation
tests/test_purge.py
Outdated
| @@ -190,3 +190,29 @@ def test_split_requests(client, requests_mocker): | |||
| # The total set of objects sent to the API should be equal to that | |||
| # provided to the client | |||
| assert sorted(all_objects) == sorted(urls) | |||
|
|
|||
|
|
|||
| @pytest.mark.xfail( | |||
There was a problem hiding this comment.
Could this please be flipped around so it's a successful test, rather than an unsuccessful test marked with xfail? In most of our projects we only use xfail to indicate "this is something we haven't got around to fixing yet".
When I suggested implementing a test to reproduce the bug, I meant a test that would fail, and then when you fix the bug you'd see the test starts to pass (i.e. test-driven development). Not a test which passes on the broken code, then when you fix the bug it starts to fail and has to be marked as xfail.
As well as being potentially confusing, tests with xfail are weak tests, because they are counted as a "pass" if they fail for any reason - not only the reason you expected while writing them. For instance if FastPurgeClient starts raising an unrelated exception which has nothing to do with auth or KeyError, this test won't be valid any more, but it'll still be considered a pass due to the xfail marker.
There was a problem hiding this comment.
Good points! I have updated the test in question. Unfortunately, I think the test is still pretty weak - all it does is assert that the first client is not the same object as the second client. FastPurgeClient doesn't have any public properties, and doesn't implement __eq__ so this is all I have for now.
I could be convinced pretty easily that this test doesn't need to stick around permanently, since the change that fixes this bug is covered by other tests (e.g. test_auth_from_dict). Let me know what you think.
There was a problem hiding this comment.
Yeah, I'm not sure. On the one hand, yes this test does look odd and niche at a glance. On the other hand, in cases like this I think "If the test wasn't here, what stops anyone from redoing the same bug - even accidentally reverting the fix?"
A good point though is that, since the test is written only in terms of the public API which should be kept stable anyway, there's essentially no reason this test should ever need to be touched again. It's going to incur practically zero maintenance cost, unlike a lower-level test depending on implementation details.
The FastPurgeClient constructor makes in-place updates to an auth data passed to it as a dictionary, which means multiple clients constructed with the same auth dictionary object can step on each other and cause errors at runtime. Add a test to reproduce this issue, update get_auth_data to make a shallow copy of any pre-existing auth dict, and fix a test that fails with the new behavior in place.
AvlWx2014
force-pushed
the
fastpurge-client-conflicting-auth-data-updates
branch
from
e3298ff
to
d75d634
Compare
The FastPurgeClient constructor makes in-place updates to an auth data passed to it as a dictionary, which means multiple clients constructed with the same auth dictionary object can step on each other and cause errors at runtime. Add a test to reproduce this issue, update get_auth_data to make a shallow copy of any pre-existing auth dict, and fix a test that fails with the new behavior in place.