-
Notifications
You must be signed in to change notification settings - Fork 95
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Don't use html for a variable name when using html module from standard library #1540
Conversation
CI is stalling on the new tests but I am unable to reproduce locally. I will investigate more tomorrow |
I was able to reproduce the test suite hanging on an Ubuntu machine. Investigating more now Edit: |
I got this working locally on Ubuntu, but I had to force the help dialog to be modal... :/ which we very well may want to avoid. The help window that pops up is blocking the test, however it is not modal so trying to us modal dialog tester did not work. However if I call This is a change in behavior and so should probably be avoided. I am unsure how to resolve this test hanging without this... EDIT: Looks like its still occurring with pyqt. Locally I had reproduced and resolved the issue with pyqt5 using what I described above. Also to clarify, this is purely a testing issue at this point. Behavior is as expected with the fixes in this PR (aside from the change to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with a few comments/caveat. I tested this with the code from #1538 - and it works as expected.
class MyTester(ModalDialogTester): | ||
def get_dialog_widget(self): | ||
return self._qt_app.activeWindow() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW the test passed for me on mac and ubuntu locally without this but without this the call of x.click_button(OK)
effectively ends up doing nothing as we are trying to call a method on None.
This way we are actually closing the window we open which feels like the right thing to do.
The real cause of the test hang I believe was not having auto_process_events=False
. I should have realized this much faster than I did. Previously I had only remembered this being necessary when using UITester
inside the use of the ModalDialogTester callback.
For example:
traitsui/traitsui/tests/editors/test_instance_editor.py
Lines 211 to 243 in 62033f5
@unittest.skipIf(no_modal_dialog_tester, "ModalDialogTester unavailable") | |
def test_simple_editor_modal(self): | |
# Test launching the instance editor with kind set to 'modal' | |
obj = ObjectWithInstance() | |
ui_tester = UITester() | |
with ui_tester.create_ui(obj, dict(view=modal_view)) as ui: | |
def click_button(): | |
ui_tester.find_by_name(ui, "inst").perform(MouseClick()) | |
def when_opened(tester): | |
with tester.capture_error(): | |
try: | |
dialog_ui = tester.get_dialog_widget()._ui | |
# If auto_process_events was not set to false, this | |
# will block due to deadlocks with ModalDialogTester. | |
ui_tester = UITester(auto_process_events=False) | |
value = ui_tester.find_by_name(dialog_ui, "value") | |
value.perform(KeySequence("Hello")) | |
self.assertEqual(obj.inst.value, "") | |
ok_button = ui_tester.find_by_id(dialog_ui, "OK") | |
ok_button.perform(MouseClick()) | |
finally: | |
# If the block above fails, the dialog will block | |
# forever. Close it regardless. | |
if tester.get_dialog_widget() is not None: | |
tester.close(accept=True) | |
mdtester = ModalDialogTester(click_button) | |
mdtester.open_and_run(when_opened=when_opened) | |
self.assertTrue(mdtester.dialog_was_opened) | |
self.assertEqual(obj.inst.value, "Hello") |
In cases where we just use UI Tester to trigger the dialog being opened, it has not been needed (until now at least AFAICR)
e.g.
traitsui/traitsui/examples/demo/Standard_Editors/tests/test_ButtonEditor_demo.py
Lines 42 to 69 in 62033f5
@unittest.skipIf(no_modal_dialog_tester, "ModalDialogTester unavailable") | |
class TestButtonEditorDemo(unittest.TestCase): | |
def test_button_editor_demo(self): | |
demo = runpy.run_path(DEMO_PATH)["demo"] | |
tester = UITester() | |
with tester.create_ui(demo) as ui: | |
simple_button = tester.find_by_id(ui, "simple") | |
custom_button = tester.find_by_id(ui, "custom") | |
# funcion object for instantiating ModalDialogTester should be a | |
# function that opens the dialog | |
# we want clicking the buttons to do that | |
def click_simple_button(): | |
simple_button.perform(MouseClick()) | |
def click_custom_button(): | |
custom_button.perform(MouseClick()) | |
mdtester_simple = ModalDialogTester(click_simple_button) | |
mdtester_simple.open_and_run(lambda x: x.click_button(OK)) | |
self.assertTrue(mdtester_simple.dialog_was_opened) | |
mdtester_custom = ModalDialogTester(click_custom_button) | |
mdtester_custom.open_and_run(lambda x: x.click_button(OK)) | |
self.assertTrue(mdtester_custom.dialog_was_opened) |
Nonetheless, this test does what we want it to now and test suite seems to not be hanging
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But now this test is Qt-specific, without being marked as such
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it implicitly will be skipped if there is no qt as modal dialog tester only exists for qt currently, but you are right we should definitely be explicit here in saying this is a very qt specific test.
Will update this now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly looks OK, but the test is Qt-only
class MyTester(ModalDialogTester): | ||
def get_dialog_widget(self): | ||
return self._qt_app.activeWindow() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But now this test is Qt-specific, without being marked as such
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
CI is failing with:
I believe we have/had an issue open related to this intermittent failure. 2 CI runs in a row now :/ it appears this is happening more frequently |
fixes #1538
Previously we were using the html module from the python standard library in a function, as well as defining a local variable
html
that held on to the html content to be used to create anHTMLHelpWindow
instance. This PR adds a very simple test to showcase the bug, and fixes it by giving thehtml
variable a new namehtml_content
. Additionally, I changed the import statement to only import what we need fromhtml
. Namely,from html import escape
. Either of these fixes would work alone (it may be overkill to do them both), but I figured it couldn't hurt.