pytest-dev / pytest-qt

pytest plugin for Qt (PyQt5/PyQt6 and PySide2/PySide6) application testing
https://pytest-qt.readthedocs.io
MIT License
402 stars 71 forks source link

High memory usage - Suspected cleanup not done per testcase #569

Open gridhead opened 18 hours ago

gridhead commented 18 hours ago

For testing our application, Loadouts for Genshin Impact, we see a very high memory usage of up to 4.0 GiB even when the actual run of the application takes somewhere in the neighbourhood of 200 MiB of memory. This starts from a meagre 100 MiB and with the run of each testcase associated with pytest-qt, it adds around 100 MiB of memory which doesn't get offloaded until the end of the entire test run. We currently have around 842 tests out of which around half are associated with pytest-qt. Ideally, if the cleanups were done per testcase, we expect the run to stay well under 500 MiB of memory usage.

Here's what the conftest module looks like where our fixture is located.

import pytest

from gi_loadouts.face.wind.main import MainWindow

@pytest.fixture
def runner(qtbot):
    testwind = MainWindow()
    qtbot.addWidget(testwind)
    return testwind
nicoddemus commented 18 hours ago

Hi @gridhead,

Each widget added via qtbot.addWidget gets cleaned up after each test, if there are widgets being created and not added to it (you only need to add the top-most widget, child widgets will get deleted automatically).

At work we have many test suites with over 3000ks+ tests, with many of the tests (more than 50%) use pytest-qt, and we do not see abnormal memory usage, so I'm fairly certain it is not a bug on pytest-qt but something somewhere in your test suite and/or application.

gridhead commented 17 hours ago

Ah, I do not claim it to be - apologies if it sounded that way.

I could use your assistance in understanding how I can lower it. From what it seems from my fixture named runner, it should have taken care of the MainWindow() instance but for some reason, it does not seem to be. I also tried explicitly removing the testwind object but I decided against it as it is anti-pattern to how qtbot operates and it did not yield any better results.

nicoddemus commented 7 hours ago

Ah, I do not claim it to be - apologies if it sounded that way.

It did to me but now worries -- not offended at all, just pointing that out because chasing that direction would not be useful.

I can show where this is handled in pytest-qt in case it helps:

We call close_widgets at each test teardown:

https://github.com/pytest-dev/pytest-qt/blob/691c7fb3403ebbcb3d63775867bd7b244bc872ac/src/pytestqt/plugin.py#L198-L205

_close_widgets explicitly calls close() and deleteLater() in all widgets being tracked in the qt_widgets attribute of the pytest.Item:

https://github.com/pytest-dev/pytest-qt/blob/691c7fb3403ebbcb3d63775867bd7b244bc872ac/src/pytestqt/qtbot.py#L743-L756

Which we track when qtbot.addWidget is called:

https://github.com/pytest-dev/pytest-qt/blob/691c7fb3403ebbcb3d63775867bd7b244bc872ac/src/pytestqt/qtbot.py#L734-L740

My guess is that MainWindow is initializing some global state each time it is being instantiated, and not freeing it properly when closed... another possibility is that close() is not actually closing the MainWindow, given that it is possible to overwrite closeEvent and ignore the event.

gridhead commented 2 hours ago

The snippets help us understand the deeper workings of pytest-qt - Thanks! ❤️

We have tried some fixes but it has mostly been shotgun debugging as we were very confused as to why things were not working as intended.

Try 1 - We tried to remove the deconstructor of the MainWindow class (ref. https://github.com/gridhead/gi-loadouts/blob/7ed8ff2ad47f85c2375a973cf1feeaf24cafed94/gi_loadouts/face/wind/main.py#L19-L20) because we thought that it might be messing with the way qtbot handles context for an object. We have the deconstructor to ensure that we can remove the temporary files that we create at the start of the application.

Try 2 - We tried to move the fixture named runner (ref. https://github.com/gridhead/gi-loadouts/blob/7ed8ff2ad47f85c2375a973cf1feeaf24cafed94/test/conftest.py#L6-L10) to each of the test modules because we suspected that maybe the context is retained across multiple test runs. That ended up not being the case as our tests were as predictable as they would run on a fresh instance of the application.

Try 3 - We tried to modify the fixture named runner (ref. https://github.com/gridhead/gi-loadouts/blob/7ed8ff2ad47f85c2375a973cf1feeaf24cafed94/test/conftest.py#L6-L10) to include destruction of the objects by converting the fixture to a generator and explicitly calling for close(), deleteLater() and del, the example of which you can find below.

@pytest.fixture
def runner(qtbot):
    testwind = MainWindow()
    qtbot.addWidget(testwind)
    yield testwind
    testwind.close()
    testwind.deleteLater()
    del testwind

Try 4 - We tried commenting out the codebase of creation of temporary data (to check if that was the cause of this problem) and the static resources (ref. https://github.com/gridhead/gi-loadouts/tree/test/gi_loadouts/face/rsrc) converted from QResource files (to confirm if those lingered on in the memory after the context) but either they were imported anyway or this action is unrelated to the actual problem.

Try 5 - We tried to observe the memory consumption per module using tracemalloc (ref. https://docs.python.org/3/library/tracemalloc.html) in our tests. However, the most consumption we got to see according to tracemalloc was in the order of magnitude of B and MB while the actual memory consumption was in MB - so we were not sure if this is workable information.

Top 10 memory consumers (Arranged in descending order)
<frozen importlib._bootstrap_external>:753: size=380 KiB (+380 KiB), count=3175 (+3175), average=123 B
/home/altruism/Projects/gi-loadouts/gi_loadouts/face/wind/wind.py:30: size=141 KiB (+141 KiB), count=3 (+3), average=46.9 KiB
/home/altruism/Projects/gi-loadouts/venv/lib/python3.12/site-packages/pytestqt/qtbot.py:697: size=24.9 KiB (+24.9 KiB), count=316 (+316), average=81 B
/usr/lib/python3.12/linecache.py:139: size=22.9 KiB (+22.9 KiB), count=218 (+218), average=108 B
/usr/lib/python3.12/unittest/mock.py:2152: size=18.0 KiB (+18.0 KiB), count=308 (+308), average=60 B
/usr/lib/python3.12/enum.py:596: size=14.9 KiB (+14.9 KiB), count=41 (+41), average=372 B
/home/altruism/Projects/gi-loadouts/gi_loadouts/face/wind/wind.py:1436: size=8136 B (+8136 B), count=31 (+31), average=262 B
<frozen importlib._bootstrap>:488: size=6774 B (+6774 B), count=91 (+91), average=74 B
/usr/lib/python3.12/unittest/mock.py:434: size=5466 B (+5466 B), count=19 (+19), average=288 B
/usr/lib/python3.12/enum.py:269: size=5000 B (+5000 B), count=34 (+34), average=147 B

Try 6 - We tried renaming the fixture named runner (ref. ref. https://github.com/gridhead/gi-loadouts/blob/7ed8ff2ad47f85c2375a973cf1feeaf24cafed94/test/conftest.py#L6-L10) because we suspected that we might be shadowing an internal identifier with the same name that is expected to live as long as the entire test run. That, of course, was a shot in the dark.

Observation - We did observe that the __del__ method or the deconstructor of the MainWindow (ref. https://github.com/gridhead/gi-loadouts/blob/7ed8ff2ad47f85c2375a973cf1feeaf24cafed94/gi_loadouts/face/wind/main.py#L19-L20) was not being called as evidenced by the cache file staying back in the temporary directory. While the following contextualization of MainWindow class in the subsequent testcases did ensure that the make_temp_file method (ref. https://github.com/gridhead/gi-loadouts/blob/7ed8ff2ad47f85c2375a973cf1feeaf24cafed94/gi_loadouts/face/rsrc/__init__.py#L15-L47) ran and that included a part where we also cleanup residual data from the previous executions that did not exit cleanly - there was no cleanup done after the last testcase finished. This meant that the deconstructor which had the cleanup instruction did not run.