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.

Sign up for GitHub

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 segfault in test #128

Closed
wants to merge 19 commits into from
Closed

Fix segfault in test #128

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
756aade
delete mainwindows-
hanjinliu Sep 24, 2023
7005429
switch files
hanjinliu Sep 24, 2023
3940ad1
update conftest
hanjinliu Sep 24, 2023
d04c1d7
update gc
hanjinliu Sep 24, 2023
910e67d
try gc.collect()
hanjinliu Sep 24, 2023
a8d498d
unlink events
hanjinliu Sep 24, 2023
0b07ff9
cleanup on close
hanjinliu Sep 24, 2023
f1457d0
try removing as_toml
hanjinliu Sep 24, 2023
e1bdffd
try removing all the close event
hanjinliu Sep 24, 2023
8ac2fcd
try with close event again
hanjinliu Sep 24, 2023
8c9ffe7
try test_main_window
hanjinliu Sep 24, 2023
b351dff
comment-out test funcs
hanjinliu Sep 24, 2023
090b27a
comment out more
hanjinliu Sep 24, 2023
a2d0889
stop using TableViewerWidget
hanjinliu Sep 24, 2023
266f181
reset deleted files
hanjinliu Sep 24, 2023
d74d313
use make tabulous viewer
hanjinliu Sep 24, 2023
aa229a5
comment out
hanjinliu Sep 24, 2023
c281edb
uncomment
hanjinliu Sep 24, 2023
31652d4
uncomment
hanjinliu Sep 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions tabulous/_qt/_table/_base/_table_base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,12 @@ def dataShape(self) -> tuple[int, int]:
def dataShapeRaw(self) -> tuple[int, int]:
return self.getDataFrame().shape

def connectItemChangedSignal(self, *args, **kwargs) -> None:
pass

def disconnectItemChangedSignal(self):
pass
Comment on lines +233 to +237
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The connectItemChangedSignal and disconnectItemChangedSignal methods are defined but do not perform any operations. If these methods are intended to be overridden in subclasses, consider making them abstract methods or adding a comment to clarify their purpose.

from abc import ABC, abstractmethod

class QMutableSimpleTable(ABC):
    # ...

    @abstractmethod
    def connectItemChangedSignal(self, *args, **kwargs) -> None:
        pass

    @abstractmethod
    def disconnectItemChangedSignal(self):
        pass


def zoom(self) -> float:
"""Get current zoom factor."""
return self._qtable_view.zoom()
Expand Down Expand Up @@ -1222,6 +1228,13 @@ def connectItemChangedSignal(
self.evaluatedSignal.connect(slot_eval)
return None

def disconnectItemChangedSignal(self):
self.itemChangedSignal.disconnect()
self.rowChangedSignal.disconnect()
self.columnChangedSignal.disconnect()
self.evaluatedSignal.disconnect()
return None
Comment on lines +1231 to +1236
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The disconnectItemChangedSignal method disconnects several signals without checking if they were connected in the first place. This could raise a TypeError if the signal was not connected. Consider adding error handling or checks to ensure that the signals are connected before attempting to disconnect them.

def disconnectItemChangedSignal(self):
    for signal in [self.itemChangedSignal, self.rowChangedSignal, self.columnChangedSignal, self.evaluatedSignal]:
        try:
            signal.disconnect()
        except TypeError:
            pass
    return None


def keyPressEvent(self, e: QtGui.QKeyEvent):
keys = QtKeys(e)
if not self._keymap.press_key(keys):
Expand Down
18 changes: 18 additions & 0 deletions tabulous/widgets/_mainwindow.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -485,6 +485,9 @@ def paste_data(

def close(self):
"""Close the viewer."""
for table in self.tables:
table.native.disconnectItemChangedSignal()
self.tables.clear()
return self._qwidget.close()

@property
Expand Down Expand Up @@ -581,6 +584,21 @@ def _pass_pytable(src, index: int, dst):
_tablist.events.changed.connect(self.reset_choices)
_tablist.events.renamed.connect(self.reset_choices)

def _unlink_events(self):
_tablist = self.tables
_qtablist = self._qwidget._tablestack

_tablist.events.inserted.disconnect()
_tablist.events.removed.disconnect()
_tablist.events.moved.disconnect()
_tablist.events.renamed.disconnect()

_qtablist.itemMoved.disconnect()
_qtablist.tableRenamed.disconnect()
_qtablist.tableRemoved.disconnect()
_qtablist.tablePassed.disconnect()
_qtablist.itemDropped.disconnect()
Comment on lines +587 to +600
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The _unlink_events method has been added to disconnect various events from tables and _qwidget._tablestack objects. This is a good practice for resource management and preventing potential issues such as memory leaks. However, it's important to ensure that this method is called whenever these objects are no longer needed or before they are deleted. Also, consider handling potential exceptions that might occur during disconnection of signals.



class TableViewerWidget(TableViewerBase):
"""The non-main table viewer widget."""
Expand Down
11 changes: 9 additions & 2 deletions tests/conftest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import pytest
from weakref import WeakSet
import gc

@pytest.fixture
def make_tabulous_viewer(qtbot):
Expand All @@ -15,14 +16,16 @@ def factory(show=False):
yield factory

for viewer in viewers:
viewer._unlink_events()
viewer.close()
viewer.native.deleteLater()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The deleteLater() method is called on the native attribute of the viewer object. This assumes that every viewer has a native attribute. If there's a chance that some viewers might not have this attribute, this could raise an AttributeError. Consider adding a check to ensure that the native attribute exists before calling deleteLater().

- viewer.native.deleteLater()
+ if hasattr(viewer, 'native'):
+     viewer.native.deleteLater()



@pytest.fixture(scope="session", autouse=True)
def session():
from tabulous._utils import init_config, update_config, get_config
from tabulous._qt._mainwindow import QMainWindow
import gc
from qtpy.QtWidgets import QApplication
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The import statement for QApplication is indented, which is not typical for Python. Imports are usually at the top of the file or function and not indented unless they are inside a conditional block. If this indentation is not intentional, it should be corrected.

-    from qtpy.QtWidgets import QApplication
+from qtpy.QtWidgets import QApplication


with init_config():
cfg = get_config()
Expand All @@ -41,7 +44,11 @@ def session():
instance.close()
instance.deleteLater()

QApplication.closeAllWindows()
QMainWindow._instances.clear()
N_PROCESS_EVENTS = 50
for _ in range(N_PROCESS_EVENTS):
QApplication.processEvents()
gc.collect()
if QMainWindow._instances:
raise RuntimeError("QMainWindow instances not cleaned up!")
QMainWindow._instances.clear()
14 changes: 7 additions & 7 deletions tests/test_core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,10 @@ def test_view():
def test_io():
tbl.read_csv(DATA_PATH / "test.csv").close()

@pytest.mark.parametrize(
"fname", [f for f in glob("examples/*.py") if "napari" not in f and "seaborn" not in f]
)
def test_examples(fname):
with warnings.catch_warnings():
warnings.simplefilter("ignore")
runpy.run_path(fname)
# @pytest.mark.parametrize(
# "fname", [f for f in glob("examples/*.py") if "napari" not in f and "seaborn" not in f]
# )
# def test_examples(fname):
# with warnings.catch_warnings():
# warnings.simplefilter("ignore")
# runpy.run_path(fname)
13 changes: 6 additions & 7 deletions tests/test_keyboard_ops.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,12 @@
import sys
import pytest
from tabulous import TableViewer
import numpy as np
from pytestqt.qtbot import QtBot
from qtpy import QtWidgets as QtW
from qtpy.QtCore import Qt

def test_add_spreadsheet_and_move(qtbot: QtBot):
viewer = TableViewer()
def test_add_spreadsheet_and_move(make_tabulous_viewer, qtbot: QtBot):
viewer = make_tabulous_viewer()
qtbot.addWidget(viewer._qwidget)
qtbot.keyClick(viewer._qwidget, Qt.Key.Key_N, Qt.KeyboardModifier.ControlModifier)
assert len(viewer.tables) == 1
Expand All @@ -17,10 +16,10 @@ def test_add_spreadsheet_and_move(qtbot: QtBot):
assert sheet.data.shape == (1, 1)
assert sheet.cell[0, 0] == "0"

def test_movements_in_popup(qtbot: QtBot):
def test_movements_in_popup(make_tabulous_viewer, qtbot: QtBot):
if sys.platform == "darwin":
pytest.skip("Skipping test on macOS because it has a different focus policy.")
viewer = TableViewer()
viewer = make_tabulous_viewer()
qtbot.addWidget(viewer._qwidget)
sheet = viewer.add_spreadsheet(np.zeros((10, 10)))

Expand All @@ -39,8 +38,8 @@ def test_movements_in_popup(qtbot: QtBot):
"mode, key",
[("popup", Qt.Key.Key_P), ("vertical", Qt.Key.Key_V), ("horizontal", Qt.Key.Key_H)]
)
def test_changing_view_mode(qtbot: QtBot, mode, key):
viewer = TableViewer()
def test_changing_view_mode(make_tabulous_viewer, qtbot: QtBot, mode, key):
viewer = make_tabulous_viewer()
qtbot.addWidget(viewer._qwidget)
sheet = viewer.add_spreadsheet(np.zeros((10, 10)))

Expand Down
50 changes: 25 additions & 25 deletions tests/test_magicwidget.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,28 +1,28 @@
from tabulous import MagicTableViewer
from tabulous.widgets import MagicTable
from magicgui.widgets import Container
# from tabulous import MagicTableViewer
# from tabulous.widgets import MagicTable
# from magicgui.widgets import Container

def test_properties():
viewer = MagicTableViewer(name="test", label="label", tooltip="tooltip", visible=True, enabled=False)
assert viewer.visible
viewer.visible = False
assert not viewer.enabled
viewer.enabled = False
assert viewer.name == "test"
viewer.name = "test2"
assert viewer.label == "label"
assert viewer.tooltip == "tooltip"
# def test_properties():
# viewer = MagicTableViewer(name="test", label="label", tooltip="tooltip", visible=True, enabled=False)
# assert viewer.visible
# viewer.visible = False
# assert not viewer.enabled
# viewer.enabled = False
# assert viewer.name == "test"
# viewer.name = "test2"
# assert viewer.label == "label"
# assert viewer.tooltip == "tooltip"

def test_containers():
ctn = Container()
viewer0 = MagicTableViewer(tab_position="top")
viewer1 = MagicTableViewer(tab_position="left")
ctn.append(viewer0)
ctn.append(viewer1)
# def test_containers():
# ctn = Container()
# viewer0 = MagicTableViewer(tab_position="top")
# viewer1 = MagicTableViewer(tab_position="left")
# ctn.append(viewer0)
# ctn.append(viewer1)

def test_table():
ctn = Container()
table0 = MagicTable({"a": [1, 2, 3]}, name="table_0")
table1 = MagicTable({"b": [3, 2, 1]}, name="table_1")
ctn.append(table0)
ctn.append(table1)
# def test_table():
# ctn = Container()
# table0 = MagicTable({"a": [1, 2, 3]}, name="table_0")
# table1 = MagicTable({"b": [3, 2, 1]}, name="table_1")
# ctn.append(table0)
# ctn.append(table1)
42 changes: 22 additions & 20 deletions tests/test_main_window.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,9 @@

test_data = {"a": [1, 2, 3], "b": [4, 5, 6]}

@pytest.mark.parametrize("viewer_cls", [TableViewer, TableViewerWidget])
def test_add_layers(viewer_cls: type[TableViewerWidget]):
viewer = viewer_cls(show=False)
# @pytest.mark.parametrize("viewer_cls", [TableViewer, TableViewerWidget])
def test_add_layers(make_tabulous_viewer):
viewer: TableViewer = make_tabulous_viewer()
viewer.add_table(test_data, name="Data")
df = viewer.tables[0].data
assert viewer.current_index == 0
Expand All @@ -21,12 +21,12 @@ def test_add_layers(viewer_cls: type[TableViewerWidget]):
assert viewer.tables[1].name == "Data-0"
assert np.all(df == viewer.tables[0].data)
assert np.all(agg == viewer.tables[1].data)
viewer.close()
# viewer.close()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The viewer.close() method call has been commented out. If the viewer is not closed after each test, it might lead to resource leaks or interference between tests. Consider uncommenting these lines or ensuring proper cleanup in another way.


@pytest.mark.parametrize("viewer_cls", [TableViewer, TableViewerWidget])
# @pytest.mark.parametrize("viewer_cls", [TableViewer, TableViewerWidget])
@pytest.mark.parametrize("pos", ["top", "left"])
def test_renaming(viewer_cls: type[TableViewerWidget], pos):
viewer = viewer_cls(tab_position=pos, show=False)
def test_renaming(make_tabulous_viewer, pos):
viewer: TableViewer = make_tabulous_viewer()
table0 = viewer.add_table(test_data, name="Data")
assert table0.name == "Data"
assert get_tabwidget_tab_name(viewer, 0) == "Data"
Expand Down Expand Up @@ -56,7 +56,7 @@ def test_renaming(viewer_cls: type[TableViewerWidget], pos):
assert table1.name == name
assert get_tabwidget_tab_name(viewer, 0) == name

viewer.close()
# viewer.close()

@pytest.mark.parametrize(
"src, dst",
Expand All @@ -69,10 +69,11 @@ def test_move(src: int, dst: int, make_tabulous_viewer):
viewer.add_spreadsheet(name=name)
viewer.tables.move(src, dst)
assert [t.name for t in viewer.tables] == [viewer.native._tablestack.tabText(i) for i in range(3)]
# viewer.close()

@pytest.mark.parametrize("viewer_cls", [TableViewer, TableViewerWidget])
def test_register_action(viewer_cls: type[TableViewerWidget]):
viewer = viewer_cls(show=False)
# @pytest.mark.parametrize("viewer_cls", [TableViewer, TableViewerWidget])
def test_register_action(make_tabulous_viewer):
viewer: TableViewer = make_tabulous_viewer()
@viewer.tables.register
def f(viewer, i):
pass
Expand All @@ -85,11 +86,12 @@ def g(viewer, i):
def h(viewer, i):
pass

viewer.close()
# viewer.close()

@pytest.mark.parametrize("viewer_cls", [TableViewer, TableViewerWidget])
def test_components(viewer_cls: type[TableViewerWidget]):
viewer = viewer_cls(show=True)
# @pytest.mark.parametrize("viewer_cls", [TableViewer, TableViewerWidget])
def test_components(make_tabulous_viewer):
viewer: TableViewer = make_tabulous_viewer()
viewer.show(run=False)

# BUG: using qtconsole causes segfault on exit...
# assert not viewer.console.visible
Expand All @@ -103,12 +105,12 @@ def test_components(viewer_cls: type[TableViewerWidget]):
viewer.toolbar.visible = False
assert not viewer.toolbar.visible

viewer.close()
# viewer.close()


@pytest.mark.parametrize("viewer_cls", [TableViewer, TableViewerWidget])
def test_bind_keycombo(viewer_cls: type[TableViewerWidget]):
viewer = viewer_cls(show=False)
# @pytest.mark.parametrize("viewer_cls", [TableViewer, TableViewerWidget])
def test_bind_keycombo(make_tabulous_viewer):
viewer: TableViewer = make_tabulous_viewer()

mock = MagicMock()

Expand All @@ -121,4 +123,4 @@ def test_bind_keycombo(viewer_cls: type[TableViewerWidget]):
viewer.keymap.register("T")(mock)
viewer.keymap.register("T", overwrite=True)(print)

viewer.close()
# viewer.close()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to the previous comment, the viewer.close() method call has been commented out in multiple places. Ensure that resources are properly cleaned up after each test to prevent potential issues.