Fix segfault take 2 #129
Fix segfault take 2 #129
Conversation
Rate Limit ExceededYou have exceeded the limit for the number of files or commits that can be reviewed per hour. Please wait 11 minutes and 59 seconds before requesting another review. To re-trigger a review, convert this PR to a draft and then mark it as ready for review again. Alternatively, you can push a new commit to this PR. |
Rate Limit ExceededYou have exceeded the limit for the number of files or commits that can be reviewed per hour. Please wait 4 minutes and 40 seconds before requesting another review. To re-trigger a review, convert this PR to a draft and then mark it as ready for review again. Alternatively, you can push a new commit to this PR. |
WalkthroughThis pull request introduces significant changes to the testing suite and some core components of the application. It enhances flexibility by allowing different types of viewers to be created in tests, refactors code by removing unused dependencies and test functions, and improves signal handling in the table viewer. Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Files selected for processing (6)
- tests/conftest.py (1 hunks)
- tests/test_colormap.py (1 hunks)
- tests/test_core.py (1 hunks)
- tests/test_keyboard_ops.py (3 hunks)
- tests/test_magicwidget.py (1 hunks)
- tests/test_main_window.py (5 hunks)
Files skipped from review due to trivial changes (3)
- tests/test_colormap.py
- tests/test_core.py
- tests/test_magicwidget.py
Additional comments (Suppressed): 8
tests/conftest.py (1)
- 6-23: The new fixture
make_tabulous_viewernow supports creation of different types of viewers (TableViewer,TableViewerWidget, andMagicTableViewer) based on theclsparameter. Ensure that all test functions using this fixture have been updated to handle this change, especially if they rely on specific properties or methods unique to a particular viewer type.tests/test_keyboard_ops.py (3)
9-13: The
make_tabulous_viewerfixture is now being used to create theTableViewerinstance instead of directly instantiating it. Ensure that this fixture is correctly set up and provides a validTableViewerinstance for the tests.20-26: Similar to the previous comment, the
make_tabulous_viewerfixture is now being used to create theTableViewerinstance. Verify that this fixture is correctly set up and provides a validTableViewerinstance for the tests.42-46: Again, the
make_tabulous_viewerfixture is now being used to create theTableViewerinstance. Ensure that this fixture is correctly set up and provides a validTableViewerinstance for the tests.tests/test_main_window.py (4)
13-18: The
make_tabulous_viewerfixture now accepts a string parameterviewer_clsinstead of a class type. Ensure that the fixture correctly handles these string inputs and creates the appropriate viewer objects.74-79: The
make_tabulous_viewerfixture now accepts a string parameterviewer_clsinstead of a class type. Ensure that the fixture correctly handles these string inputs and creates the appropriate viewer objects.91-96: The
make_tabulous_viewerfixture now accepts a string parameterviewer_clsinstead of a class type. Ensure that the fixture correctly handles these string inputs and creates the appropriate viewer objects.110-115: The
make_tabulous_viewerfixture now accepts a string parameterviewer_clsinstead of a class type. Ensure that the fixture correctly handles these string inputs and creates the appropriate viewer objects.
| cls: Literal["main", "widget", "magic"] = "main", | ||
| show: bool = False, | ||
| ): | ||
| if cls == "main": | ||
| viewer = TableViewer(show=show) | ||
| elif cls == "widget": | ||
| viewer = TableViewerWidget(show=show) | ||
| elif cls == "magic": | ||
| viewer = MagicTableViewer(show=show) |
There was a problem hiding this comment.
This code block lacks error handling for invalid cls values. If a value other than "main", "widget", or "magic" is passed, the function will complete without creating a viewer, which could lead to unexpected behavior in the tests. Consider adding an else clause to raise an exception when an unsupported cls value is provided.
):
if cls == "main":
viewer = TableViewer(show=show)
elif cls == "widget":
viewer = TableViewerWidget(show=show)
elif cls == "magic":
viewer = MagicTableViewer(show=show)
+ else:
+ raise ValueError(f"Unsupported viewer type: {cls}")
viewers.add(viewer)
return viewer| @pytest.mark.parametrize("viewer_cls", classes_to_test) | ||
| def test_renaming(make_tabulous_viewer, viewer_cls: str): | ||
| viewer: TableViewerBase = make_tabulous_viewer(viewer_cls, show=False) | ||
| table0 = viewer.add_table(test_data, name="Data") | ||
| assert table0.name == "Data" | ||
| assert get_tabwidget_tab_name(viewer, 0) == "Data" |
There was a problem hiding this comment.
The test function test_renaming no longer accepts a pos parameter. If this parameter was used to test different tab positions, consider reintroducing it to ensure comprehensive testing.
Rate Limit ExceededYou have exceeded the limit for the number of files or commits that can be reviewed per hour. Please wait 2 minutes and 45 seconds before requesting another review. To re-trigger a review, convert this PR to a draft and then mark it as ready for review again. Alternatively, you can push a new commit to this PR. |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Files selected for processing (2)
- tests/conftest.py (2 hunks)
- tests/test_main_window.py (5 hunks)
Files skipped from review due to trivial changes (1)
- tests/test_main_window.py
Additional comments (Suppressed): 1
tests/conftest.py (1)
- 12-24: The
make_tabulous_viewerfixture has been updated to support the creation of different types of viewers (main,widget, andmagic). This is a significant change that enhances the flexibility of the test suite. However, it's important to ensure that all tests using this fixture have been updated accordingly to handle the newclsparameter.
| viewers.add(viewer) | ||
| return viewer | ||
|
|
||
| yield factory | ||
|
|
||
| for viewer in viewers: | ||
| viewer.close() | ||
|
|
||
| viewer.native.deleteLater() |
There was a problem hiding this comment.
The viewer object is being closed and deleted in the same loop iteration. This could potentially lead to issues if the viewer object is still referenced elsewhere after being deleted. It would be safer to first close all viewers in one loop, then delete them in a separate loop.
- for viewer in viewers:
- viewer.close()
- viewer.native.deleteLater()
+ for viewer in viewers:
+ viewer.close()
+
+ for viewer in viewers:
+ viewer.native.deleteLater()| QApplication.closeAllWindows() | ||
| QMainWindow._instances.clear() | ||
| N_PROCESS_EVENTS = 50 | ||
| for _ in range(N_PROCESS_EVENTS): | ||
| QApplication.processEvents() |
There was a problem hiding this comment.
The addition of QApplication.closeAllWindows() and the loop calling QApplication.processEvents() multiple times are intended to ensure that all windows are properly closed and all events are processed before proceeding. While this is generally a good practice, it might introduce unnecessary delays in the test suite if there are no pending events. Consider adding a condition to break the loop if there are no more events to process.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Files selected for processing (5)
- tabulous/_qt/_table/_base/_table_base.py (2 hunks)
- tabulous/widgets/_mainwindow.py (2 hunks)
- tests/conftest.py (2 hunks)
- tests/test_core.py (2 hunks)
- tests/test_main_window.py (5 hunks)
Files skipped from review due to trivial changes (1)
- tests/test_core.py
Additional comments (Suppressed): 10
tabulous/widgets/_mainwindow.py (2)
485-491: The
closemethod has been updated to disconnect theItemChangedSignalfrom each table in the viewer before clearing the tables and closing the viewer. This is a good practice as it ensures that no lingering connections remain after the viewer is closed, which could potentially lead to memory leaks or unexpected behavior.587-600: A new method
_unlink_eventshas been introduced to disconnect various events from both_tablistand_qtablist. This is a good practice for event management, especially when the object is about to be destroyed or reused, to prevent any potential issues such as memory leaks or unexpected behavior due to lingering event connections.tests/conftest.py (3)
12-24: The
make_tabulous_viewerfixture has been enhanced to support different types of viewers (main,widget, andmagic). This is a significant change that increases the flexibility of the fixture. However, it's important to ensure that all test functions using this fixture have been updated accordingly to specify the viewer type or can handle the default "main" viewer type.57-61: Closing all application windows and processing events multiple times are new steps added to the session cleanup process. While these steps can help ensure a clean state at the end of the session, they might also introduce unexpected behavior if there are still active tasks or threads when these operations are performed. Therefore, it's recommended to verify that these steps do not interfere with any ongoing processes or tests.
63-64: The check for remaining
QMainWindowinstances has been moved after the event processing loop. This change makes sense because the event processing might result in some windows being closed. However, it's important to verify that this check still works as expected and that it correctly identifies any instances that were not properly cleaned up.tests/test_main_window.py (5)
13-18: The
make_tabulous_viewerfixture now takes a string parameterviewer_clsinstead of a class type. Ensure that the fixture correctly handles these string inputs and creates the appropriate viewer objects.28-33: Similar to the previous comment, ensure that the
make_tabulous_viewerfixture correctly handles the string input forviewer_cls.74-79: Again, verify that the
make_tabulous_viewerfixture correctly handles the string input forviewer_cls.91-96: Ensure that the
make_tabulous_viewerfixture correctly handles the string input forviewer_cls.110-115: Once more, verify that the
make_tabulous_viewerfixture correctly handles the string input forviewer_cls.
| def connectItemChangedSignal(self, *args, **kwargs) -> None: | ||
| pass | ||
|
|
||
| def disconnectItemChangedSignal(self): | ||
| pass |
There was a problem hiding this comment.
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.
- def connectItemChangedSignal(self, *args, **kwargs) -> None:
- pass
-
- def disconnectItemChangedSignal(self):
- pass
+ @abstractmethod
+ def connectItemChangedSignal(self, *args, **kwargs) -> None:
+ pass
+
+ @abstractmethod
+ def disconnectItemChangedSignal(self):
+ pass| def disconnectItemChangedSignal(self): | ||
| for signal in [ | ||
| self.itemChangedSignal, | ||
| self.rowChangedSignal, | ||
| self.columnChangedSignal, | ||
| self.evaluatedSignal, | ||
| ]: | ||
| try: | ||
| signal.disconnect() | ||
| except TypeError: | ||
| pass | ||
| return None |
There was a problem hiding this comment.
The disconnectItemChangedSignal method attempts to disconnect all signals even if they were not connected before. This can lead to unnecessary TypeError exceptions being raised and caught. Consider checking if the signal is connected before attempting to disconnect it.
def disconnectItemChangedSignal(self):
for signal in [
self.itemChangedSignal,
self.rowChangedSignal,
self.columnChangedSignal,
self.evaluatedSignal,
]:
if signal.is_connected():
try:
signal.disconnect()
except TypeError:
pass
return None| for viewer in viewers: | ||
| viewer._unlink_events() | ||
| viewer.close() | ||
|
|
||
| viewer.native.deleteLater() | ||
|
|
There was a problem hiding this comment.
The cleanup steps for each viewer in the WeakSet now include unlinking events and deleting the native object. These additional steps improve the thoroughness of the cleanup process, but they also introduce potential risks if any of these operations fail or have side effects. It would be beneficial to add error handling around these operations to catch and log any exceptions, allowing the cleanup process to continue for the remaining viewers.
for viewer in viewers:
try:
viewer._unlink_events()
viewer.close()
viewer.native.deleteLater()
except Exception as e:
print(f"Error cleaning up viewer {viewer}: {e}")
Summary by CodeRabbit
make_tabulous_viewerfixture intests/conftest.pyto support different types of viewers (main,widget, andmagic), improving test flexibility.tests/test_keyboard_ops.pyandtests/test_main_window.pyto use the refactoredmake_tabulous_viewerfixture.tests/test_core.pyby removing unused dependencies and test functions.tests/test_colormap.pyandtests/test_magicwidget.py.tabulous/_qt/_table/_base/_table_base.py, enhancing control over event handling.tests/conftest.pyto ensure proper deletion of viewer objects and processing of events.