pyface icon indicating copy to clipboard operation
pyface copied to clipboard

[Do Not Merge] New GUI Test Tools

Open corranwebster opened this issue 4 years ago • 5 comments

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 the GUI class (in hopefully a sensible way).

The API is tested out on the Window class.

Tests need to be written.

corranwebster avatar Sep 14 '19 07:09 corranwebster

Codecov Report

Merging #447 into master will increase coverage by 0.36%. The diff coverage is 68.11%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
pyface/testing/util.py 0% <0%> (ø)
pyface/i_gui.py 80.55% <100%> (+2.43%) :arrow_up:
pyface/qt/__init__.py 46.93% <100%> (+2.25%) :arrow_up:
pyface/i_window.py 82.35% <100%> (-15.65%) :arrow_down:
pyface/ui/qt4/confirmation_dialog.py 100% <100%> (ø) :arrow_up:
pyface/toolkit_utils.py 100% <100%> (ø)
pyface/ui/wx/window.py 90.24% <20%> (+10.49%) :arrow_up:
pyface/ui/qt4/window.py 72.8% <57.14%> (-1.07%) :arrow_down:
pyface/ui/wx/timer/timer.py 87.5% <62.5%> (+24.34%) :arrow_up:
pyface/ui/wx/gui.py 66.66% <66.66%> (+18.39%) :arrow_up:
... and 24 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 42bdac6...6d23d86. Read the comment docs.

codecov-io avatar Sep 16 '19 09:09 codecov-io

@mdickinson could you please have a look at the proposed API (particularly for GUITestCase and EventLoopHelper). The main changes are:

  • it is a subclass of TestCase rather than a mixin (I could be convinced either way)
  • most toolkit-dependent event loop stuff is pushed to the GUI class
  • it uses pyface.timers rather than working at the toolkit level for timers
  • no misleading context managers

There are some additional changes that I'd be interested in making (such as renaming event_loop_... methods to just loop_...), and there is currently no replacement for the ModalDialogTester (and probably won't be in this PR). Also there are errors on Windows that need to be wrangled, but I'm not going to invest the effort before getting some feedback that this is a better solution than our current one.

corranwebster avatar Sep 16 '19 13:09 corranwebster

I left a few comments on the code, mostly (but not entirely) at nitpick level.

  • it is a subclass of TestCase rather than a mixin (I could be convinced either way)

Me too; there are tradeoffs either way, but overall I think I'm happier with the subclass approach. If it makes GuiTestCase usable with the usual super pattern in a multiple inheritance situation, so much the better.

Overall the API looks good to me, but I don't think I understand what EventLoopHelper.event_loop_with_timeout and GuiTestCase.event_loop_with_timeout are for. From just the name and the signature, I'd assume that these are for cases where you want to say "I've set things up so that the event loop will only run for a short finite time. Now either go and run the event loop until it stops naturally, or if it hasn't stopped after <timeout> seconds raise a timeout exception." But that doesn't seem to be what the code is doing.

Will there be unit tests for the EventLoopHelper and the GUITestCase?


(Edited to change EventLoop to EventLoopHelper and fix other minor typos.)

mdickinson avatar Sep 17 '19 08:09 mdickinson

N.B. I didn't really look at the parts of this PR that aren't directly related to the GuiTestCase.

mdickinson avatar Sep 17 '19 08:09 mdickinson

Yes, event_loop_with_timeout was problematic, and the docstring got out of sync with the final version. The original intent was as in the current GuiTestAssistant: run the actual event loop at least repeat times with a timeout if it takes too long to repeat that many times.

I then went to the start which the current docstring suggests which was "run for timeout seconds" no matter what due to issues getting it to work on WxPython; but it turned out that the problem was in fact that under WxPython you can't start the event loop until there is at least one window in existence; so I reverted to the original intent and used a different approach for the use case which was causing problems.

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 event_loop_with_condition where the condition always returns False.

corranwebster avatar Sep 17 '19 09:09 corranwebster