[Do Not Merge] New GUI Test Tools #447
Conversation
Adds some sefeguards against deleted objects when tearing down.
Codecov Report
@@ Coverage Diff @@
## master #447 +/- ##
==========================================
+ Coverage 36.63% 36.99% +0.36%
==========================================
Files 481 487 +6
Lines 26424 26722 +298
Branches 3917 3953 +36
==========================================
+ Hits 9680 9886 +206
- Misses 16321 16394 +73
- Partials 423 442 +19
Continue to review full report at Codecov.
|
|
@mdickinson could you please have a look at the proposed API (particularly for
There are some additional changes that I'd be interested in making (such as renaming |
| finally: | ||
| self.gui.quit_on_last_window_close = flag | ||
|
|
||
| def event_loop(self, repeat=1, allow_user_events=True): |
There was a problem hiding this comment.
I'd really like to include some advice in the docstring that if you find yourself needing to use a repeat of greater than 1, you should consider whether you can use event_loop_until_condition instead.
| repeat : positive int | ||
| The number of times to call process events. Default is 1. | ||
| allow_user_events : bool | ||
| Whether to process user-generated events. |
There was a problem hiding this comment.
Perhaps add "for example keyboard and mouse events" to clarify what's meant by "user-generated events" in this context?
| ---------- | ||
| condition : callable | ||
| A callable to determine if the stop criteria have been met. This | ||
| should accept no arguments. |
There was a problem hiding this comment.
I think this should also be explicit about the expected return type of the callable.
| should accept no arguments. | ||
|
|
||
| timeout : float | ||
| Number of seconds to run the event loop in the case that the trait |
There was a problem hiding this comment.
Copypasta here: "the trait change does not occur".
| ) | ||
| repeat_timer.on_trait_change(self._on_stop, 'active') | ||
|
|
||
| try: |
There was a problem hiding this comment.
Is there a way to engineer a test for the corner case where the timer timeout expires before the event loop is started? (Perhaps by patching start_event_loop to sleep for some time first.) I'm reasonably convinced that the current logic is fine (_on_stop must be called from the main thread, so it can't possibly be called until the event loop starts), but it would be good to have a test to prevent future regressions.
What happens if the timeout is zero? Is that a use-case we want to support, or should the docstring specify that timeout should be positive?
| Parameters | ||
| ---------- | ||
| timeout: float, optional, keyword only | ||
| Number of seconds to run the event loop. Default value is 10.0. |
There was a problem hiding this comment.
The repeat parameter isn't documented.
There was a problem hiding this comment.
We should also document the conditions under which ConditionTimeoutError is raised.
I'm not really sure I understand when this method would be used, and when it should be considered that an error has occurred.
|
|
||
| Parameters | ||
| ---------- | ||
| timeout: float, optional, keyword only |
There was a problem hiding this comment.
"keyword only" doesn't seem to actually be true here. Presumably the intent is to add that extra "*" to the signature after Python 2 support has been dropped? (If so, should there be an issue open for that?)
| A callable to determine if the stop criteria have been met. This | ||
| should accept no arguments. | ||
|
|
||
| timeout : float |
There was a problem hiding this comment.
Should this also be documented as "keyword only", to match event_loop_with_timeout?
| self.addCleanup(self._gc_collect) | ||
| self.addCleanup(self.event_loop_helper.event_loop, 5, False) | ||
|
|
||
| super(GuiTestCase, self).setUp() |
There was a problem hiding this comment.
I'm not sure this is useful. It doesn't do anything as-is, so presumably the intent is that in a multiple inheritance situation (class MyTestCase(GuiTestCase, OtherTestCaseSubclass): ...), MyTestCase.setUp could do a super(MyTestCase, self).setUp() and have both GuiTestCase.setUp and OtherTestCaseSubclass.setUp being called as a result. But that likely won't work, because of the UnittestTools mixin (which doesn't use super), so I think this line just ends up misleading people into thinking that the super cooperative multiple inheritance pattern will work.
If we want this to work, there may be a case for inlining the UnittestTools machinery instead of subclassing UnittestTools. Otherwise, I'd suggest just removing this line (and the corresponding line in the tearDown).
There was a problem hiding this comment.
But that likely won't work
Okay, thinking about this, I'm not sure any more. It may work after all. The UnittestTools class will just be effectively invisible to the super machinery, because it doesn't implement setUp and tearDown at all.
Can we have a test for multiple inheritance cases that verifies that both the parent class setUp methods get called as expected (regardless of the ordering of the parents)? One way that this could potentially break is if some future version of UnittestTools does decide to implement setUp, so it would be useful to have a test that alerts us to the breakage.
| # Some top-level widgets may only be present due to cyclic garbage not | ||
| # having been collected; force a garbage collection before we decide to | ||
| # close windows. This may need several rounds. | ||
| for _ in range(10): |
There was a problem hiding this comment.
It may be worth adding an "else" clause here that complains loudly (perhaps with a warnings.warn warning of some kind) if we reach it; if 10 rounds of garbage collection still haven't cleaned up everything, then something's likely seriously wrong (e.g., a separate thread that's busily creating new cycles while we're trying to clean them up) and worthy of investigation.
| """ If 'stream' is None, provide a temporary handle to /dev/null. """ | ||
|
|
||
| if stream is None: | ||
| out = open(os.devnull, 'w') |
There was a problem hiding this comment.
For Python 3, I suggest adding an explicit encoding here. Seems nitpicky, but there's at least a possibility that the system encoding doesn't allow arbitrary Unicode to be written.
|
I left a few comments on the code, mostly (but not entirely) at nitpick level.
Me too; there are tradeoffs either way, but overall I think I'm happier with the subclass approach. If it makes Overall the API looks good to me, but I don't think I understand what Will there be unit tests for the (Edited to change |
|
N.B. I didn't really look at the parts of this PR that aren't directly related to the |
|
Yes, I then went to the start which the current docstring suggests which was "run for But given that we are using it in just one place, it maybe needs a re-think or removal (even the old version is not used much). After all, without the repeat condition, it is just |
This is a proposed new version of the GUI test assistant that is more stand-alone and backend-independent. The biggest changes are to rely more on
pyface.timer(which in turn implies some changes to that API) and to push a lot of the backend dependent behaviour onto theGUIclass (in hopefully a sensible way).The API is tested out on the
Windowclass.Tests need to be written.