Skip to content
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

[FIX] owboxplot: Fix an error in widget cleanup during tests #6136

Merged
merged 4 commits into from
Sep 30, 2022

Conversation

ales-erjavec
Copy link
Contributor

Issue

Since biolab/orange-widget-base#204 was released, Orange.widgets.tests.test_workflows fails with

Traceback (most recent call last):
  File "/home/runner/work/orange3/orange3/.tox/pyqt6/lib/python3.9/site-packages/Orange/widgets/visualize/owboxplot.py", line 257, in eventFilter
    if obj is self.box_view.viewport() and \
AttributeError: 'OWBoxPlot' object has no attribute 'box_view'
ERROR: InvocationError for command /home/runner/work/orange3/orange3/.tox/pyqt6/bin/python -m unittest Orange.widgets.tests (exited with code -6 (SIGABRT)) (exited with code -6)
Description of changes

Change order of conditions in eventFilter to short-circuit access to widget members which can no longer exist (empty self.__dict__).

Includes
  • Code changes
  • Tests
  • Documentation

@codecov
Copy link

codecov bot commented Sep 16, 2022

Codecov Report

Merging #6136 (ee44ae0) into master (79e4237) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #6136   +/-   ##
=======================================
  Coverage   86.48%   86.48%           
=======================================
  Files         315      315           
  Lines       67771    67771           
=======================================
+ Hits        58610    58611    +1     
+ Misses       9161     9160    -1     

@ales-erjavec ales-erjavec marked this pull request as draft September 16, 2022 09:50
@ales-erjavec ales-erjavec force-pushed the fixes/owboxplot-test-cleanup branch 5 times, most recently from 7cb448c to 545418f Compare September 16, 2022 11:27
@janezd
Copy link
Contributor

janezd commented Sep 16, 2022

This looks bad. Do I understand correctly that some methods can be invoked when the widget is being destroyed and already has an empty __dict__?

Are there any other methods (that we use) in which we need to be careful?

Is this limited to tests (which have no screen) or can it also happen to the user?

@ales-erjavec ales-erjavec force-pushed the fixes/owboxplot-test-cleanup branch 2 times, most recently from bbb0e44 to dc9b1fc Compare September 21, 2022 11:21
@PrimozGodec
Copy link
Contributor

Tests are failing due to changes in pandas. I propose a temporary solution since I think it is a bug in pandas #6145

@ales-erjavec ales-erjavec marked this pull request as ready for review September 22, 2022 09:28
@ales-erjavec
Copy link
Contributor Author

Do I understand correctly that some methods can be invoked when the widget is being destroyed and already has an empty __dict__?

Yes.

Are there any other methods (that we use) in which we need to be careful?

event(), but other virtual event handlers could have the same problem.

Is this limited to tests (which have no screen) or can it also happen to the user?

I guess it can happen to the user also.

@ales-erjavec ales-erjavec force-pushed the fixes/owboxplot-test-cleanup branch 2 times, most recently from 2744dfb to ee8afe6 Compare September 23, 2022 06:58
@ales-erjavec ales-erjavec marked this pull request as draft September 28, 2022 10:58
@janezd janezd self-assigned this Sep 30, 2022
@ales-erjavec ales-erjavec marked this pull request as ready for review September 30, 2022 07:39
@markotoplak
Copy link
Member

I am fixing the oldest deps tests in a separate PR - no need to worry about that.

@markotoplak markotoplak merged commit f358595 into biolab:master Sep 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants