Introduce toolkit-agnostic API for GUI testing

[kitchoi]

This PR proposes a toolkit-agnostic API for testing GUI with TraitsUI. While the target audience of this API are users of TraitsUI, the API should be useful for testing within TraitsUI as well.

This borrowed the idea of #664 as well as recent PRs that add toolkit-agnostic looking tests (thanks to both @corranwebster and @ievacerny).

Motivation:

• TraitsUI already needs to perform tests using toolkit-specific code for programmatically manipulating widgets, e.g. QPushButton.click() for Qt.
• Downstream projects typically want to write some end-to-end tests that require manipulating GUI. This involves getting to know the implementation details of an editor, ended up writing similar test code as in TraitsUI. The test code also tends to tie with a specific GUI toolkit.
• Some of these interactions, e.g. clicking a button, modify value on a text box, are extremely common.

This PR introduces a "tester" (or a "runner"?) UITester (name to be confirmed), to achieve the following:

• GUI test code can be toolkit-agnostic. TraitsUI will take care of calling the relevant toolkit-specific code.
• Editor implementation details are hidden. e.g. There are no need to worry whether it is an QTextEdit or an QLineEdit being used for a text editor. With that, TraitsUI will can modify implementation details with less risk of breaking downstream projects' test code.
• The API is (hopefully) intuitive to use: UITester.click will click something clickable (e.g. a button). UITester.set_text will set a text.
• The GUI.process_events is called where it is needed. This step is often difficult to get right. TraitsUI can do that heavy lifting.
• The API will enforce cleaning up GUI.
• The testing API will be useful for some tests in TraitsUI too. By making TraitsUI its own "user", the API is more likely to be both useful and up-to-date.

Nice-to-have that is currently achieved by this API:

• UITester does not assume the test code to be written with unittest.TestCase, let alone subclassing GuiTestAssistant from pyface.
• UITester is a standalone object and multiple instances can be used together, even on the same UI. It can be used in collaboration with GuiTestAssistant.
• It is possible to extend support for custom editors written in downstream projects. See tests that define a separate registry of "Simulators". Downstream projects can do exactly the same to use the testing API against their custom editors.

Cost:

• TraitsUI will need to selectively move some of test code into library code, which needs to be maintained with backward compatibility in mind. The benefit of this cost, as stated above, is to allow downstream code to rely less on implementation details, hence allowing more freedom for TraitsUI to change. This tradeoff needs to be borne in mind when deciding whether to move certain toolkit-specific code from TraitsUI test to the simulators.

To navigate this PR:

• Start from traitsui.testing.tests.test_ui_tester: This should more or less give the feel for how the testing API will be used.
• Then traitsui.testing.ui_tester for the public facing API.
• Then traitsui.testing.simulation.BaseSimulation on a definition of a "Simulator"
• Then an example of a toolkit-specific "Simulator", e.g. changes in traitsui.qt4 and traitsui.wx.

[kitchoi]

Note: This API does not try to cover all use cases, because there are simply too many possible ways to interact with a GUI. It should however make the most common use cases easy.

[ievacerny]

This is a very useful addition and it will definitely make testing a lot easier! Haven't thought about using this outside testing, so I don't have any comments on other aspects.

TraitsUI will need to selectively move some of test code into library code, which needs to be maintained with backward compatibility in mind. The benefit of this cost, as stated above, is to allow downstream code to rely less on implementation details, hence allowing more freedom for TraitsUI to change. This tradeoff needs to be borne in mind when deciding whether to move certain toolkit-specific code from TraitsUI test to the simulators.

I haven't tried this, but could we implement additional testing specific simulations within the test module by subclassing the simulator from the library code and changing the registration? That would allow more tests to make use of the API and perform actions "safely".

[ievacerny]

I think confirming text entry should be a separate function because it is a separate action for the user - pressing enter or removing focus. And I would say that testing for no change before confirmation, then checking the value after confirmation would be useful for quite a few editors.

[ievacerny]

Is this meant to eventually replace skip_if_*?

[kitchoi]

When writing (non-null) toolkit-agnostic test case, skip_if_null still isn't quite future proof because the moment one adds a new toolkit, the tests will be failing with NotImplementedError on that toolkit, unless one captures and then reraise SkipTest. It is much easier to opt-in toolkits than to opt-out toolkits in this circumstance.

When the list of toolkits contains only one item, then the choice is somewhat subjective. To be honest, I'd still much prefer stating the positive requires_one_of([WX]) or requires(WX) than the negative skip_if_not(WX). requires_one_of supports all these use cases I think. I don't mind keeping skip_if_* around, though we probably want only one way to do things if that's possible :).

[kitchoi]

Is this meant to eventually replace skip_if_*?

Sorry, so no that wasn't my intention, but I'd quite like for this requires_one_of to replace skip_if_*, if that's agreed by others :)

[ievacerny]

I think double click API would also be useful

[ievacerny]

Why is it necessary to pass ui into every single command? I haven't encountered a use case where two uis are open at the same time. I think saving ui reference somewhere in the tester would simplify the calls to the api and in turn would get rid of as ui from most context manager uses.

[kitchoi]

I see your point there.
It is possible for the tester to keep a reference to the UI to remove that one argument, but we should probably then add an optional argument to override the default behaviour in case one really needs more than one UI at the same time, because saving one positional argument does not seem worth it to kill that possibility altogether.
On the other hand, the code is more explicit when ui is passed. The code literally reads "set the text of ui's 'father.first_name' to 'B'". Explicit is better than implicit so I am still inclined towards keeping ui here. I can be convinced the other way though.

[ievacerny]

I don't have strong feelings about this. Mainly I was struggling to fit as ui into one line so I thought maybe we can get rid of it 😄

[kitchoi]

Let's file a petition to increase the maximum line length for flake8 :D

[ievacerny]

Just to make sure - these functions remove the need from having store_exceptions_on_all_threads in tests if they're performing all actions via tester?

What about adding process_events to the API for those cases where the tester is used but some custom actions are performed? The only use for store_exceptions_on_all_threads I had after switching test_enum_editor.py to use this tester was:

 with store_exceptions_on_all_threads():
    self.tester.gui.process_events()

Adding process_events with an appropriate docstring might also be a good opportunity to explain when exactly it is needed, which might help people not as familiar with GUI to avoid test interactions.

[kitchoi]

Just to make sure - these functions remove the need from having store_exceptions_on_all_threads in tests if they're performing all actions via tester?

Yes.

The only use for store_exceptions_on_all_threads I had after switching test_enum_editor.py to use this tester was:

I am quite interested in seeing that example. Do you mind sharing or pointing me to the closest line in the current code base please?

[ievacerny]

def test_radio_enum_editor_button_update(self):
    enum_edit = EnumModel()
    view = get_view("custom")

    with self.tester.create_ui(enum_edit, dict(view=view)) as ui:
        editor = ui.get_editors("value")[0]
        widget = editor.control

        # The layout is: one, three, four \n two
        self.assertEqual(
            get_all_button_status(widget), [True, False, False, False]
        )

        enum_edit.value = "two"
        with store_exceptions_on_all_threads():
            self.tester.gui.process_events()

        self.assertEqual(
            get_all_button_status(widget), [False, False, False, True]
        )

[kitchoi]

I see. I agree it is reasonable to add a method to do this.

[kitchoi]

Thank you for trying this with the EnumEditor tests!

[kitchoi]

The latest update to the API should make extension easy. So the above can be supported locally like this:

LOCAL_REGISTRY = SimulatorRegistry()


if is_current_backend_qt4:

    from traitsui.qt4.enum_editor import RadioEditor

    @simulate(RadioEditor, LOCAL_REGISTRY)
    class NewRadioEditorSimulator(BaseSimulator):
        def get_all_button_status(self):
            layout = self.editor.control.layout()
            button_status = []
            for i in range(layout.count()):
                button = layout.itemAt(i).widget()
                button_status.append(button.isChecked())
            return button_status

...

    def test_radio_enum_editor_button_update(self):
        enum_edit = EnumModel()
        view = get_view("custom")
        self.tester.add_registry(LOCAL_REGISTRY)
        with self.tester.create_ui(enum_edit, dict(view=view)) as ui:
            status = self.tester.get_editor_value(
                ui, "value", lambda s: s.get_all_button_status()
            )
            # The layout is: one, three, four \n two
            self.assertEqual(status, [True, False, False, False])

            enum_edit.value = "two"
            status = self.tester.get_editor_value(
                ui, "value", lambda s: s.get_all_button_status()
            )
            self.assertEqual(status, [False, False, False, True])

[ievacerny]

I think a good addition to the API would be get_editor_status which would allow to get information about the button status (e.g. get_all_button_status in _tools.py), selected items, text in all existing fields, etc.

[ievacerny]

Nit: I don't like having a simulator in between editors - it makes simulators harder to find and is an interruption if just looking at the editors. I think it would be better to have all simulators at the very end of the file.

[kitchoi]

Fair point. I will move them down.

[ievacerny]

Could we incorporate toolkit dependent utility functions from _tools.py somewhere here (press_ok_button, click_button, etc.)? They might be beneficial for downstream projects but now they are hidden in a private module.

[kitchoi]

Yes: The click method defined in BaseSimulator can be implemented for ButtonEditor for performing the same effect of click_button.
The press_ok_button in _tools.py currently just presses the ONE button it finds without checking if it is the OK button or if there are other buttons. That probably works for the few tests that use it, but moving that over here as is would be a bad idea. I think I need to think harder about that one...

[ievacerny]

click_button can also be used with set_editor and any other editors that save a reference to the button (as the control of the button can be passed directly). What I'm thinking about is keeping those functions as they are but maybe making them static methods of UITester so that they are exposed publicly, and adding processing of GUI events as required. I don't think they need to go through any simulator as they don't really need to know anything about a specific editor.

Again, I'm not really thinking about using this outside testing, so there might be an argument that my suggestion would make these functions unavailable for other uses.

[kitchoi]

I see.

We could decide to make traitsui.tests._tools.click_button public, but I prefer keeping it out of UITester.

The purpose of UITester is to promote and support test code that is free of implementation details of an editor. I am afraid in order to use click_button, one still needs to know where a widget is in an editor. For example, the use of click_button for testing SetEditor requires one to access GUI widgets like _use and _unuse (which are QPushButton when Qt is used).

Again there is a question of how common is it to test pressing up-down-left-right buttons in a set editor in downstream projects. If it is common, then we might be better off rolling out a completely separate tester for driving interactions for a more complicated editor.

[kitchoi]

could we implement additional testing specific simulations within the test module by subclassing the simulator from the library code and changing the registration? That would allow more tests to make use of the API and perform actions "safely".

I like this idea. Using the code as it is now, it is possible todo that, but it may be awkward and not very obvious:

from traitsui.qt4.text_editor import TextEditorSimulator, SimpleEditor
from traitsui.testing.simulation import REGISTRY

class MySimulator(TextEditorSimulator):
    def get_text(self):
         # overridden behaviour...

def test_something(self):
    # Let's not mutate global state.
    # Create a copy of the default registry and then modify it
    new_registry = copy.deepcopy(REGISTRY)
    new_registry.unregister(SimpleEditor)
    new_registry.register(SimpleEditor, MySimulator)
    tester = UITester(registry=new_registry)
    # now you are good to go...

It will be quite easy to extract the first three lines (in the test) there into a helper function.

I think making this dance less awkward will allow us to defer deciding whether to expose some of the methods to the public API. For example, I am not entirely sure that a method like get_all_button_status will be all that popular in downstream projects (thinking the tradeoff there). But maybe it will be popular, I don't know. Allowing the API to be modified and extended like this will not only make the lives easier for developers working on downstream project, it will also make it easier for us to see what use cases are popular and should be maintained as library code.

[ievacerny]

If this PR is going to be merged directly, SetValue needs to be moved outside confirmed clause as it is required in both cases.

[kitchoi]

Thank you!
I was thinking when this PR is finalized for review, the implementations for EnumEditor and TextEditor will be removed, but an implementation for ButtonEditor simulator will be added instead (clicking a button is much simpler). I chose to implement EnumEditor and TextEditor for the draft because they are a bit more complicated so they are useful for demonstration/proof-of-concept purposes.
Your effort to test the EnumEditor is still very helpful though because we will certainly want to implement a simulator for this editor (I think/hope).

[kitchoi]

Thank you @ievacerny for your feedback. They are really helpful.

I haven't tried this, but could we implement additional testing specific simulations within the test module by subclassing the simulator from the library code and changing the registration? That would allow more tests to make use of the API and perform actions "safely".

I think the need within TraitsUI to implement simulation methods without having them exposed publicly is a very legitimate use case, and is in fact a very useful scenario to simulate how downstream projects can extend the API to cover use cases not already supported by the built-in method.

The last three commits try to make extension easier. This commit adds a test to simulate how this would look like in Traitsui test modules or downstream projects' test code: 1f9c81e

Essentially, TraitsUI will provide a default registry of simulators, but projects can add and maintain as many separate registries as they like, and use them without TraitsUI's registry, or together with TraitsUI's registry. No subclassing is needed. I'd prefer keeping the simulators as leaf classes, i.e. subclass-at-your-own-risk.

[corranwebster]

This turned out long, so tl;dr: perhaps consider a redesign where there isn't a Simulator system of classes, but we just add needed get/set methods directly to the editor classes, because they'll probably be useful there anyway.

A quick read of this and if I understand correctly what you're effectively doing is doing a (safe, properly handled) monkeypatching of the editor objects. Eg. because the TextEditor class doesn't have a set_placeholder and get_placeholder method that wraps the toolkit object, you create an Simulator object that encodes how to do this; you're effectively adding functionality to the underlying TextEditor in this way.

As far as it goes, I think this is fine: I have some minor quibbles with some things (eg. I think "Simulator" isn't a good name, because at first I thought it was some sort of mock, but naming things is hard). But it is essentially working hard to paper over a fundamental architectural problem, which is that TraitsUI isn't properly factoring out the toolkit-specific stuff (you can see this by looking at home much almost but not quite the same code there is in the Wx and Qt versions of editors).

Eg. these problems would almost entirely go away if the TextEditor instead were to wrap a Pyface ITextField/TextField is it provides (tested) traits for the placeholder, and private methods for getting and setting the placeholder which are used by the testing code.

So I'm fine with something like this going in: it solves a problem that we have right now; but I don't think it makes sense to spend time writing tests using this (which will involve writing lots of Simulators) as opposed to changing the existing code to expose what we want directly. As a simple example, if we were to instead add methods get_placeholder_text and set_placeholder_text to the TextEditor object (maybe make them private), eg. in Qt:

def get_placeholder_text(self):
    return self.control.placeholderText()

def set_placeholder_text(self, placeholder):
    self.control.setPlaceholderText(placeholder)

then this:

• simplifies what the GUI testing apparatus needs to do (you need to find the editor, then you're good, no matter what toolkit)
• allows later refactoring so that the editor uses these methods exclusively internally,
• longer term, allows us to factor out all the rest of the code into common classes, leaving the toolkit classes to be mostly just a collection of these sorts of accessors.

[kitchoi]

Ooops, I am really sorry @corranwebster. I think my choice of using placeholder as an example to demonstrate how downstream projects could define their custom simulator is a terrible choice :( . The simulators should be simulating user interactions, like clicking a button, entering some text in a text field. Setting the placeholder isn't something a user can do to a GUI.

Perhaps I should remove the click_index on the EnumEditor simulator and then try to contribute it in that TestUITesterSimulateExtension test. Let me try that again...

[kitchoi]

Perhaps I should remove the click_index on the EnumEditor simulator and then try to contribute it in that TestUITesterSimulateExtension test. Let me try that again...

Done.

[kitchoi]

A quick read of this and if I understand correctly what you're effectively doing is doing a (safe, properly handled) monkeypatching of the editor objects. Eg. because the TextEditor class doesn't have a set_placeholder and get_placeholder method that wraps the toolkit object, you create an Simulator object that encodes how to do this; you're effectively adding functionality to the underlying TextEditor in this way.

Although placeholder was a bad example (my fault, very sorry!), I think this statement still stands if we replace get_placeholder and set_placeholder with get_text (to get the text displayed by a combox box directly via Qt/wx, not via editor.value) and click_index (to simulate user clicking an item on a combox box).

While adding these methods directly onto the editor will also work, I think they don't belong there because they serve different purposes: testing. Tests have different needs to application code.

On the other hand, I see how this UITester could be abused for monkeypatching editors indeed. I think it is worth keeping the word "test" in the name to indicate that if someone is using it outside of testing, they are abusing it, just like unittest.mock can be abused to monkeypatch anything.

[corranwebster]

While adding these methods directly onto the editor will also work, I think they don't belong there because they serve different purposes: testing. Tests have different needs to application code.

Part of my point was although we might be adding these methods to support testing, and they might only used by testing now, they are more generally useful and will make subsequent changes that we want to make easier.

And more generally I'd disagree that methods to support testing (or more generally, introspection) don't belong on the objects. A lot of the recent work in traits has been to expose internal things so that they can be introspected. This was useful for testing, but is also useful for interactive debugging and may be useful for functional code later. Methods can be private if we are worried about incorrect usage or too-busy public APIs.

Also now it is clearer what Simulator means. Perhaps UserSimulator (or SimulatedUser or ActionSimulator or something similar) might be a clearer name. Given their purpose I'm leaning even more towards suggesting that toolkit stuff be implemented on the objects as much as possible: if I am writing a simulator for a complex set of actions (eg. enter this value into a field, now set this toggle, now press this button, now what is the value displayed in this thing...) I want to do that in a standard language as much as possible, rather than having to write two very similar but not quite the same versions in Qt and Wx.

[kitchoi]

Thank you! UserSimulator would be a better name indeed. I think I would give the simulators private names, i.e. ones with preceding underscore. This will allow TraitsUI more freedom to move and refactor code later.

I see your point about future refactoring needs, and I agree there might be opportunities, but I am not sure if that should happen now.

Refactoring is good if the code being refactored are true duplications, and not false duplications. True duplications are code objects/modules that not only look similar, perform similar tasks, but also share the same reasons to change. False duplications are code objects/modules that look very similar or even identical now, but they will change for different reasons, at different times. If we allow false duplications of code to stay separate, we allow them to evolve and diverge independently. If we refactor false duplications, we risk coupling objects that should not be coupled. We need to resist the temptation to "refactor" false duplications, but keep an eye out for refactoring true duplications. Hiding implementation details will allow refactoring to happen later when we know better where the true duplications are.

I think the API of this UITester will support this future refactoring: It allows and encourages test code to be free of internal implementation details of the editors. By doing so, we allow traitsui more freedom to change, e.g. like switching to wrap pyface components instead. If downstream test code relies heavily on a simple ButtonEditor having a QPushButton on control, or an update_object method that takes a specific Qt event object, it is going to be hard for TraitsUI to change without causing a lot of test failures.

The objective of encouraging test code that is free of implementation details is defeated a bit by the ability to define and provide custom simulators. This ability already suits TraitsUI's own testing needs (see this example). For downstream projects, it would also be justified if the editors targeted by the custom simulators are defined within the downstream projects: because the downstream projects own the implementation details of their editors. At the same time, we need to acknowledge that however hard we try and however much resources we have, this is not going to cover 100% use cases. Having this ability to define custom simulations is to acknowledge that, yes, sometimes, maybe you do need to write test code that depends strongly on the editor's implementation details. The design encourages such code to be isolated within the simulators, rather than being scattered across many tests.

[kitchoi]

Thanks again @ievacerny and @corranwebster for the review.
Over time I have revised the API quite substantially.
I am going to close this and open a new one with the revised API. This PR can be kept on the record for future reference (I think I will reference this from the new PR).