Add tests for extended trait change issues #537 and #538 #543
Conversation
corranwebster
changed the title
corranwebster
changed the title
Codecov Report
@@ Coverage Diff @@
## master #543 +/- ##
==========================================
+ Coverage 65.15% 65.31% +0.15%
==========================================
Files 44 44
Lines 7040 7040
Branches 1413 1413
==========================================
+ Hits 4587 4598 +11
+ Misses 2030 2017 -13
- Partials 423 425 +2
Continue to review full report at Codecov.
|
|
Is there any chance we could untangle the class inheritance a bit in this PR? We've got a lot of monkeypatch-style subclassing from leaf classes going on - it would be cleaner to restructure so that we have only abstract base classes (that don't get instantiated) and concrete subclasses of those abcs (which do). |
| ref = Instance(ArgCheckList, ()) | ||
|
|
||
|
|
||
| class Instance3(Instance2): |
There was a problem hiding this comment.
Could we replace this with a direct HasTraits subclass? With the current inheritance structure it's hard for the reader to see easily what Instance3 is doing.
| @on_trait_change("ref.value[]") | ||
| def arg_check1(self, new): | ||
| self.calls[1] += 1 | ||
| self.tc.assertEqual(new, self.dst_new) |
There was a problem hiding this comment.
Any idea what dst is supposed to stand for?
There was a problem hiding this comment.
I am guessing "dst" is destination and "src" is source, but I don't have the mental model for why those terms. It clearly made sense to someone (probably Dave Morrill) but there's no deeper explanation that I have found in the documentation that explains the terms.
| self.tc.assertIs(object, self.exp_object) | ||
| self.tc.assertEqual(name, self.exp_name) | ||
| self.tc.assertEqual(old, self.exp_old) | ||
| self.tc.assertEqual(new, self.exp_new) | ||
|
|
||
|
|
||
| class Instance2(Instance1): |
There was a problem hiding this comment.
Can we come up with a more meaningful class name?
There was a problem hiding this comment.
LOKTM. I think we can remove some of the monkey-patch-style subclassing, and there's one minor suggested change (calls = calls = ...).
I'm impressed at how this PR maintains consistency with the existing style, but I have to say that I find this test style horrible, and I want to go in and rewrite the whole module: names of classes don't give a hint about what those classes do (what is Instance4 for, as opposed to Instance3?); tests should be broken up and new instances created instead of resetting traits before doing further tests; there's way too much unnecessary subclassing going on, and the actual test assertions should be in the tests instead of hidden away in the classes under test (which then allows one to stop doing the weird and ugly create-instance-then-call-trait-set-to-initialise-it dance). But that rewrite can happen after this goes in.
Co-Authored-By: Mark Dickinson <mdickinson@enthought.com>
|
I've replaced the |
|
Super minor... With #537 and #538 being nontrivial to fix without breaking downstream projects, these tests may be here to stay for sometime. It is difficult to maintain a test for the long-term using the |
Will mark as expected failures for now. When #537 and #538 are resolved expected failures should be removed.