Add missed dialogs to `qtbot` in `test_qt_notifications` to prevent segfaults #5171

Czaki · 2022-10-04T09:59:14Z

Description

update of test_qt_notifications.py to better guard Qt object.

Type of change

Bug-fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

References

How has this been tested?

example: the test suite for my feature covers cases x, y, and z
example: all tests pass with my change
example: I check if my changes works with both PySide and PyQt backends
as there are small differences between the two Qt bindings.

Final checklist:

My PR is the minimum possible work for the desired functionality
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works
If I included new strings, I have used trans. to make them localizable.
For more information see our translations guide.

codecov · 2022-10-04T10:15:39Z

Codecov Report

Merging #5171 (7c07fad) into main (d64b351) will increase coverage by 0.00%.
The diff coverage is 95.91%.

@@           Coverage Diff           @@
##             main    #5171   +/-   ##
=======================================
  Coverage   88.78%   88.78%           
=======================================
  Files         572      573    +1     
  Lines       48810    48892   +82     
=======================================
+ Hits        43336    43410   +74     
- Misses       5474     5482    +8

Impacted Files	Coverage Δ
napari/_qt/_tests/test_qt_notifications.py	`96.00% <95.91%> (+0.27%)`	⬆️
napari/_qt/dialogs/qt_notification.py	`89.30% <0.00%> (-3.30%)`	⬇️
napari/layers/utils/layer_utils.py	`93.12% <0.00%> (-0.19%)`	⬇️
napari/layers/shapes/shapes.py	`91.15% <0.00%> (-0.07%)`	⬇️
napari/components/dims.py	`97.68% <0.00%> (-0.03%)`	⬇️
napari/layers/points/points.py	`99.07% <0.00%> (-0.02%)`	⬇️
napari/layers/base/base.py	`91.70% <0.00%> (ø)`
napari/_qt/qt_main_window.py	`75.41% <0.00%> (ø)`
napari/utils/colormaps/colormap.py	`98.07% <0.00%> (ø)`
napari/components/_tests/test_dims.py	`100.00% <0.00%> (ø)`
... and 8 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

andy-sweet

I almost want to rewrite these tests from scratch, but there's nothing from with this changes as far as I can see, so I'm approving.

Which dialog was not being added to qtbot?

andy-sweet · 2022-10-07T17:00:57Z

napari/_qt/_tests/test_qt_notifications.py

-    qtbot.add_widget(widget)
+    show_count = 0
+
+    def mock_show_dialog(self):


Optional: might be nice to either define a fixture for this to be consistent with NapariQtNotification, or just use a plain function instead of a callable and pass the class for which you want to mock show (i.e. NapariQtNotification or TracebackDialog).

Czaki

I put comments where NapariQtNotification are created without using qtbot.

Czaki · 2022-10-07T20:36:11Z

napari/_qt/_tests/test_qt_notifications.py

@@ -125,17 +147,16 @@ def run(self):
            res = NapariQtNotification.show_notification(notif)


This created here.

Czaki · 2022-10-07T20:36:40Z

napari/_qt/_tests/test_qt_notifications.py

@@ -162,9 +183,9 @@ def test_notification_display(
    notif = Notification('hi', severity, actions=[('click', lambda x: None)])
    NapariQtNotification.show_notification(notif)


This created here

andy-sweet · 2022-10-07T22:21:48Z

napari/_qt/_tests/test_qt_notifications.py

+
+    def mock_show_dialog(self):
+        stat.show_count += 1
+        qtbot.add_widget(self)


This only adds the widget on each call of show. If there are no calls that means it won't be added, which I guess is why we also have the other calls to add_widget in the tests. Could we patch __init__ so that we add it exactly once instead?

could you check now?

If I recall correctly, all the tests were passing before this change, but now the flaky windows 3.8 pyside2 test is failing again due to test_qt_notifications. We might have just got lucky before this change and I don't see any reason why the new changes are worse than the previous ones, but just wondered if you had any thoughts?

I found 2 places..
But they are failing only if run all tests, not when running individual tests.

andy-sweet · 2022-10-07T22:22:26Z

Thanks for clarifying. Added an optional question/suggestion, but still looks fine to me.

andy-sweet · 2022-10-10T23:19:05Z

napari/_qt/_tests/test_qt_notifications.py

+        old_traceback_init(self, *args, **kwargs)
+        qtbot.add_widget(self)
+
+    monkeypatch.setattr(NapariQtNotification, "__init__", mock_notif_init)


Will the class definition (e.g. NapariQtNotification) be imported fresh for each test function? Or is there a risk that old_notif_init may be the definition from the last test function run?

pytest monkeypatch, restore previous value on test end

Czaki

I found 2 calls of show functions that also start animations, that may be reason of faliture.

Czaki · 2022-10-11T09:16:03Z

napari/_qt/_tests/test_qt_notifications.py

+@pytest.fixture(autouse=True)
+def raise_on_show(monkeypatch, qtbot):
+    def _raise_on_show(self, *args, **kwargs):
+        raise RuntimeError('error!')
+
+    monkeypatch.setattr(NapariQtNotification, 'show', _raise_on_show)
+    monkeypatch.setattr(TracebackDialog, 'show', _raise_on_show)


This fixture raises an exception on the show call if another fixture does not mock it.

Czaki · 2022-10-11T09:16:36Z

napari/_qt/_tests/test_qt_notifications.py

+        old_traceback_init(self, *args, **kwargs)
+        qtbot.add_widget(self)
+
+    monkeypatch.setattr(NapariQtNotification, "__init__", mock_notif_init)


pytest monkeypatch, restore previous value on test end

Czaki · 2022-10-11T09:17:32Z

napari/_qt/_tests/test_qt_notifications.py

+    monkeypatch.setattr(
+        NapariQtNotification, "show_notification", lambda x: None
+    )


For me, it is only a workaround, but I do not have any idea how to do this better.

Czaki · 2022-10-11T09:19:12Z

napari/_qt/_tests/test_qt_notifications.py

+    monkeypatch.setattr(
+        NapariQtNotification, "show_notification", lambda x: None
+    )


For me, it is only a workaround, but I do not have any idea how to do this better.

Czaki · 2022-10-11T11:25:39Z

Still fails...

Czaki · 2022-10-11T11:34:30Z

napari/_qt/_tests/test_qt_notifications.py


    bttn = dialog.row2_widget.findChild(QPushButton)
    assert bttn.text() == 'View Traceback'
-    mock_show.assert_not_called()
+    assert count_show.show_traceback_count == 0
    bttn.click()


it looks like I found a source of the problem. The click of this button is called close_with_fade which starts a timer.
And just after this test, there is a segfault.

goanpeca

Looks good @Czaki ! thanks for working on this

andy-sweet · 2022-10-14T18:40:25Z

I'll merge this after 24 hours unless there are any objections.

…egfaults (#5171) * check another approach to collect items * guard other dialogs * mock __init__ for add to qtbot * test to find show calls * disable show_notifications * put monkeypatch in better place * close with fade

check another approach to collect items

a68845e

github-actions bot assigned Czaki Oct 4, 2022

github-actions bot added qt Relates to qt tests Something related to our tests labels Oct 4, 2022

guard other dialogs

a913f96

Czaki changed the title ~~Another approach to prevent leak of Qt objects.~~ Add more dialogs to qtbot in test_qt_notifications to prevent segfaults Oct 4, 2022

Czaki requested a review from andy-sweet October 4, 2022 10:52

andy-sweet approved these changes Oct 7, 2022

View reviewed changes

Czaki commented Oct 7, 2022

View reviewed changes

andy-sweet reviewed Oct 7, 2022

View reviewed changes

mock __init__ for add to qtbot

869f969

Czaki force-pushed the bugfix/anothe_guard_for_dialog branch from 1b928a3 to 869f969 Compare October 7, 2022 23:50

Czaki changed the title ~~Add more dialogs to qtbot in test_qt_notifications to prevent segfaults~~ Add missed dialogs to qtbot in test_qt_notifications to prevent segfaults Oct 8, 2022

andy-sweet reviewed Oct 10, 2022

View reviewed changes

Czaki added 3 commits October 11, 2022 11:13

test to find show calls

b76dfeb

disable show_notifications

759c646

put monkeypatch in better place

44b31cd

Czaki commented Oct 11, 2022

View reviewed changes

close with fade

7c07fad

Czaki commented Oct 11, 2022

View reviewed changes

goanpeca approved these changes Oct 13, 2022

View reviewed changes

andy-sweet merged commit fb81ff6 into napari:main Oct 17, 2022

Czaki mentioned this pull request Dec 8, 2022

Access violation as test runner exits in github ci #3012

Closed

Czaki deleted the bugfix/anothe_guard_for_dialog branch June 7, 2023 11:16

Czaki mentioned this pull request Jun 7, 2023

v0.4.18 - release process #5911

Closed

Czaki added the bugfix PR with bugfix label Jun 7, 2023

Czaki added this to the 0.4.18 milestone Jun 7, 2023

Czaki added the triaged-0.4.18 To mark PR that is triaged in 0.4.18 release process label Jun 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add missed dialogs to `qtbot` in `test_qt_notifications` to prevent segfaults #5171

Add missed dialogs to `qtbot` in `test_qt_notifications` to prevent segfaults #5171

Czaki commented Oct 4, 2022 •

edited

Loading

codecov bot commented Oct 4, 2022 •

edited

Loading

andy-sweet left a comment

andy-sweet Oct 7, 2022

Czaki left a comment

Czaki Oct 7, 2022

Czaki Oct 7, 2022

andy-sweet Oct 7, 2022

Czaki Oct 7, 2022

andy-sweet Oct 10, 2022

Czaki Oct 11, 2022

andy-sweet commented Oct 7, 2022

andy-sweet Oct 10, 2022

Czaki Oct 11, 2022

Czaki left a comment

Czaki Oct 11, 2022

Czaki Oct 11, 2022

Czaki Oct 11, 2022

goanpeca Oct 13, 2022

Czaki Oct 11, 2022

Czaki commented Oct 11, 2022

Czaki Oct 11, 2022

goanpeca left a comment

andy-sweet commented Oct 14, 2022

		@@ -125,17 +147,16 @@ def run(self):
		res = NapariQtNotification.show_notification(notif)

		@@ -162,9 +183,9 @@ def test_notification_display(
		notif = Notification('hi', severity, actions=[('click', lambda x: None)])
		NapariQtNotification.show_notification(notif)

Add missed dialogs to qtbot in test_qt_notifications to prevent segfaults #5171

Add missed dialogs to qtbot in test_qt_notifications to prevent segfaults #5171

Conversation

Czaki commented Oct 4, 2022 • edited Loading

Description

Type of change

References

How has this been tested?

Final checklist:

codecov bot commented Oct 4, 2022 • edited Loading

Codecov Report

andy-sweet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Czaki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andy-sweet commented Oct 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Czaki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Czaki commented Oct 11, 2022

Choose a reason for hiding this comment

goanpeca left a comment

Choose a reason for hiding this comment

andy-sweet commented Oct 14, 2022

Add missed dialogs to `qtbot` in `test_qt_notifications` to prevent segfaults #5171

Add missed dialogs to `qtbot` in `test_qt_notifications` to prevent segfaults #5171

Czaki commented Oct 4, 2022 •

edited

Loading

codecov bot commented Oct 4, 2022 •

edited

Loading