Handle unexpected return value

[BjarneHerland]

Issue
Resolves #2004

Approach
Handle the unexpected return-value properly

[BjarneHerland]

I think I'll include a test for this case before review...

[jondequinor]

I think I'll include a test for this case before review...

Yes, this part of ERT is very poorly tested. There are numerous examples of qt testing, however, so setup should be fairly copy-paste.

[BjarneHerland]

The application-code uses static methods from Qt to display modal windows. The proposed test applies a delayed timer and relies on the application-instance to provide a hook for the window in order to close it programmatically.

Note that the modal dialogs must not use native widgets in the test because closing these crashes Python. We assume that these methods return the same values for native and Qt-based widgets.

[jondequinor]

The application-code uses static methods from Qt to display modal windows. The proposed test applies a delayed timer and relies on the application-instance to provide a hook for the window in order to close it programmatically.

Did you take a look at the other QT tests? You're not supposed to instantiate QApplication directly because it will probably not work as intended: https://pytest-qt.readthedocs.io/en/latest/qapplication.html

If you're simply testing some dialogs, use qtbot as we do elsewhere.

[BjarneHerland]

Did you take a look at the other QT tests? You're not supposed to instantiate QApplication directly because it will probably not work as intended: https://pytest-qt.readthedocs.io/en/latest/qapplication.html

Good point - will check out the exit-issue mentioned here.

If you're simply testing some dialogs, use qtbot as we do elsewhere.

IMO it's not obvious how qtbot helps getting a reference to the widget spun up by the static methods we need to test. But I'll give it another hard look on Monday.

A completely different approach is to parse the __doc__ attribute of said static methods and verify the expected return-type, but this feels a little hackish... what do you think?

[BjarneHerland]

A completely different approach is to parse the __doc__ attribute of said static methods and verify the expected return-type, but this feels a little hackish... what do you think?

Hmm... when phrasing the goal of the test and the quoted question, I realize that adding static typing achieves the same goal. The missing piece is to set up mypy to include ert_gui in the analysis. Please consider this approach instead....

[jondequinor]

No, we add a test. Here it is:

from qtpy.QtWidgets import QFileDialog
from ert_gui.ertwidgets.pathchooser import PathChooser
from ert_gui.ertwidgets.models.path_model import PathModel


def test_selectfile(qtbot, tmpdir, monkeypatch):
    model = PathModel(tmpdir, must_be_a_file=True)
    widget = PathChooser(model)
    qtbot.addWidget(widget)

    monkeypatch.setattr(QFileDialog, "getOpenFileName", lambda *args: ("foo", "bar"))

    with qtbot.waitActive(widget):
        widget.show()
    widget.selectPath()

[jondequinor]

Change

    with qtbot.waitActive(widget):
        widget.show()

to something like

widget.show()
qbot.add_widget(widget)
qtbot.waitForWindowShown(widget)
…
asserts

Activation requires a window manager, but we don't need that here. So my bad!

[jondequinor]

Super! Thanks