Fix segfault in test #128
Fix segfault in test #128
Conversation
WalkthroughThis pull request primarily focuses on enhancing resource management and test setup in the codebase. It introduces garbage collection, event unlinking, and window closing to ensure proper cleanup after tests. Additionally, it refactors test functions to use a factory function for creating instances of 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 (1)
- tests/conftest.py (2 hunks)
Additional comments (Suppressed): 1
tests/conftest.py (1)
- 45-45: The clearing of
QMainWindow._instanceshas been moved up before the event processing and garbage collection. This change seems logical as it ensures that the list of instances is cleared before any further cleanup actions are taken. However, please verify if there are any side effects of this change, especially if there are other parts of the code that rely on the state ofQMainWindow._instancesafter the session fixture.
| @pytest.fixture(scope="session", autouse=True) | ||
| def session(): | ||
| from tabulous._utils import init_config, update_config, get_config | ||
| from tabulous._qt._mainwindow import QMainWindow | ||
| from qtpy.QtWidgets import QApplication |
There was a problem hiding this comment.
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
tests/conftest.py
Outdated
| QApplication.closeAllWindows() | ||
| QMainWindow._instances.clear() | ||
| for i in range(10): | ||
| QApplication.processEvents() |
There was a problem hiding this comment.
The addition of QApplication.closeAllWindows() and multiple calls to QApplication.processEvents() is a good practice for ensuring that all windows are closed and all events are processed before proceeding, which can help prevent segmentation faults during testing. However, the magic number 10 used in the loop could be replaced with a constant to improve readability and maintainability.
- for i in range(10):
+ EVENT_PROCESSING_CYCLES = 10
+ for i in range(EVENT_PROCESSING_CYCLES):
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Files selected for processing (1)
- tests/conftest.py (3 hunks)
Additional comments (Suppressed): 2
tests/conftest.py (2)
47-47: The
QApplication.closeAllWindows()method is being called after individual instances have been closed and deleted. This might be redundant if all instances have already been properly cleaned up. However, it can serve as a safety net in case some windows were missed during the individual cleanup process.53-54: The code checks if any instances of
QMainWindowremain after cleanup and raises aRuntimeErrorif any are found. This is a good practice as it ensures that all resources have been properly cleaned up, helping to prevent memory leaks and other related issues.
tests/conftest.py
Outdated
|
|
||
| gc.collect(1) |
There was a problem hiding this comment.
The gc.collect(1) function is being called with an argument of 1. This means that only the youngest generation of objects will be collected. If there are older generations of objects that need to be collected, they will be ignored. Consider calling gc.collect() without any arguments to ensure all generations of objects are collected.
- gc.collect(1)
+ gc.collect()
tests/conftest.py
Outdated
| N_PROCESS_EVENTS = 10 | ||
| for _ in range(N_PROCESS_EVENTS): | ||
| QApplication.processEvents() | ||
| gc.collect() |
There was a problem hiding this comment.
The code is processing events multiple times (10 times in this case) to ensure thorough cleanup. While this may help in certain scenarios where events are queued up, it could potentially introduce unnecessary overhead if there are no events to process. Consider adding a condition to break the loop if no events are left to process.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Files selected for processing (1)
- tests/conftest.py (3 hunks)
Additional comments (Suppressed): 3
tests/conftest.py (3)
3-3: The
gcmodule is imported but it was already imported in the old code. Ensure that this import is necessary.47-51: Closing all application windows and processing events multiple times can be resource-intensive. Make sure that this is necessary and doesn't negatively impact performance. Also, consider making
N_PROCESS_EVENTSa constant at the top of the file or a configurable parameter, rather than hardcoding it here.52-54: Garbage collection is forced and then a check is performed to see if any
QMainWindowinstances remain. If any do, a RuntimeError is raised. This is good for catching memory leaks, but make sure that raising an error is the desired behavior in this case, as it could halt the execution of your program.
| @@ -16,13 +17,15 @@ def factory(show=False): | |||
|
|
|||
| for viewer in viewers: | |||
| viewer.close() | |||
| viewer.native.deleteLater() | |||
There was a problem hiding this comment.
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()
Rate Limit ExceededYou have exceeded the limit for the number of files or commits that can be reviewed per hour. Please wait 29 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. |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Files selected for processing (3)
- tabulous/_qt/_table/_base/_table_base.py (2 hunks)
- tabulous/widgets/_mainwindow.py (2 hunks)
- tests/conftest.py (3 hunks)
Additional comments (Suppressed): 5
tests/conftest.py (4)
3-3: The
gcmodule is imported but not used in this hunk. However, it's used later in the code (line 52). It's a good practice to keep all imports at the top of the file.19-21: The viewer object now has additional cleanup steps: unlinking events and deleting the native object. This should help with memory management and prevent potential issues related to dangling references or memory leaks.
47-51: Closing all application windows and processing events multiple times can help ensure that all resources are properly released and cleaned up. This is especially important in GUI applications where lingering resources can cause issues like memory leaks or crashes.
52-54: Garbage collection is explicitly invoked to clean up any remaining objects. If there are still instances of
QMainWindow, an exception is raised. This is a good practice as it ensures that all resources are properly cleaned up after each test session.tabulous/widgets/_mainwindow.py (1)
- 485-491: The
closemethod has been updated to disconnect theItemChangedSignalfor each table inself.tablesbefore clearing the list. This is a good practice as it ensures that signals are disconnected before the objects they're connected to are deleted, preventing potential issues such as dangling pointers or memory leaks. However, please verify if there are any other places in the code where tables are being removed without disconnecting their signals first.
| 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() |
There was a problem hiding this comment.
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.
| 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.
from abc import ABC, abstractmethod
class QMutableSimpleTable(ABC):
# ...
@abstractmethod
def connectItemChangedSignal(self, *args, **kwargs) -> None:
pass
@abstractmethod
def disconnectItemChangedSignal(self):
pass| def disconnectItemChangedSignal(self): | ||
| self.itemChangedSignal.disconnect() | ||
| self.rowChangedSignal.disconnect() | ||
| self.columnChangedSignal.disconnect() | ||
| self.evaluatedSignal.disconnect() | ||
| return None |
There was a problem hiding this comment.
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
Rate Limit ExceededYou have exceeded the limit for the number of files or commits that can be reviewed per hour. Please wait 29 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 25 minutes and 43 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 21 minutes and 16 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/test_keyboard_ops.py (3 hunks)
- tests/test_main_window.py (7 hunks)
Additional comments (Suppressed): 6
tests/test_keyboard_ops.py (3)
10-13: The change from directly creating a
TableViewerinstance to using themake_tabulous_viewerfactory function is a good practice. It enhances modularity and maintainability of the test suite by centralizing the creation ofTableViewerinstances. However, ensure that themake_tabulous_viewerfunction is correctly implemented and returns a validTableViewerinstance.21-24: Similar to the previous comment, the use of
make_tabulous_viewerhere is a good practice. Ensure that this function is correctly implemented and returns a validTableViewerinstance.43-46: Again, the use of
make_tabulous_vieweris a good practice. Please verify that this function is correctly implemented and returns a validTableViewerinstance.tests/test_main_window.py (3)
11-16: The
make_tabulous_viewerfactory function is used to create instances ofTableViewer. This change simplifies the test setup and makes it more maintainable. However, ensure that this factory function correctly initializes theTableViewerinstances as required by these tests.26-32: The
pytest.mark.parametrizedecorator forviewer_clshas been commented out. This means that the test is no longer being run for bothTableViewerandTableViewerWidget. If this was intentional, make sure that the functionality is still adequately tested. If it was not intentional, consider uncommenting these lines.111-116: Similar to the previous comment, the
pytest.mark.parametrizedecorator forviewer_clshas been commented out in multiple places. Verify whether this change was intentional and if the functionality is still adequately tested.
| @@ -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() | |||
There was a problem hiding this comment.
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.
| @@ -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() | |||
There was a problem hiding this comment.
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.
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 27 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 15 minutes and 2 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: 0
Files selected for processing (3)
- tests/test_core.py (1 hunks)
- tests/test_keyboard_ops.py (3 hunks)
- tests/test_magicwidget.py (1 hunks)
Files skipped from review due to trivial changes (2)
- tests/test_core.py
- tests/test_magicwidget.py
Additional comments (Suppressed): 3
tests/test_keyboard_ops.py (3)
8-12: The
make_tabulous_viewerfactory function is used instead of directly creating an instance ofTableViewer. This change could be due to the need for additional setup or teardown steps that are encapsulated in the factory function. Ensure that this new approach does not affect the functionality being tested.19-25: The
make_tabulous_viewerfactory function is used here as well. As mentioned earlier, verify that this change doesn't impact the test's functionality.41-45: Again, the
make_tabulous_viewerfactory function is used. Please ensure that this change doesn't affect the test's functionality.
Summary by CodeRabbit
paste_dataand_pass_pytablefunctions for better event handling.TableViewer, improving test isolation.