Fixes error introduced in commit 5dcd716 #1951
Conversation
assoc_obj was the real culprit which returned CLBNode with _drained_at=None all the time. This made the unit tests pass as code and test got objects with None drained_at. Fixed by switching to attr and using its assoc method which ensures the attribute exists before accepting.
|
jenkins please run integration tests |
Current coverage is 92.53% (diff: 100%)@@ master #1951 diff @@
==========================================
Files 83 83
Lines 7493 7500 +7
Methods 0 0
Messages 0 0
Branches 1019 1019
==========================================
+ Hits 6933 6940 +7
Misses 463 463
Partials 97 97
|
|
jenkins please run integration tests |
|
@glyph review please |
There was a problem hiding this comment.
Continuing the migration to attrs is definitely a good thing. You should definitely carefully consider if the mutation-rather-than-returning-a-modified-value is desirable here, but otherwise it seems fine.
However, it does make me question rather having an attribute raise DrainingUnavailable is actually a good interface here; perhaps it should be a method, or perhaps the try: suite should be capturing less code? I don't have an immediate suggestion, just that this looks like a subtle magnet for future bugs.
| @@ -605,7 +604,7 @@ def test_lb_disappeared_during_feed_fetch(self): | |||
| eff = get_clb_contents() | |||
| self.assertEqual( | |||
| perform_sequence(seq, eff), | |||
| ([assoc_obj(CLBNode.from_node_json(2, node21), drained_at=2.0)], | |||
| ([attr.assoc(CLBNode.from_node_json(2, node21), _drained_at=2.0)], | |||
There was a problem hiding this comment.
Huh. That's an interesting bug in assoc; I thought it would have behaved the same as __init__ when it came to private variables.
There was a problem hiding this comment.
| return assoc_obj(node, drained_at=extract_clb_drained_at(feed)) | ||
| else: | ||
| return node | ||
| node.drained_at = extract_clb_drained_at(feed) |
There was a problem hiding this comment.
Why is this being changed to mutate its input rather than return a new value? What is the input being shared with?
There was a problem hiding this comment.
Why is this being changed to mutate its input rather than return a new value?
I didn't want to do call assoc with a private variable.
What is the input being shared with?
What input? node argument? Sorry, didn't understand.
There was a problem hiding this comment.
As far as calling assoc with a private variable... that seems like a bug in attrs which this is just a workaround for (and you're not reaching across library boundaries, at least)
assoc_objwas the real culprit in commit 5dcd716 which returnedCLBNodewith_drained_at=Noneall the time. (I never liked it from the beginning). This made the unit tests pass as code and test got objects withNonedrained_at. Fixed by switching to attr and using its assoc method which ensures the attribute exists before accepting.