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

Ensure that widgets can be garbage collected on each platform #2088

Merged
merged 8 commits into from
Sep 15, 2024

Conversation

samschott
Copy link
Member

@samschott samschott commented Aug 19, 2023

This WIP PR starts by adding tests for the garbage collection of widgets. Known issues causing test failures are:

Other test failures will need to be investigated.

PR Checklist:

  • All new features have been tested
  • All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

# Conflicts:
#	testbed/tests/widgets/test_webview.py
@@ -15,3 +18,13 @@
@pytest.fixture
async def widget():
return toga.Box(style=Pack(width=100, height=200))


async def test_cleanup():
Copy link
Member

Choose a reason for hiding this comment

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

Given this test is identical on every widget, it should be possible to pull this test definition into a generic test in properties.py, import it into the module, and use the widget fixture to provide the actual widget instance. This is how tests like test_focus_noop work; AFAICT the same pattern should work here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Potentially. Getting widget from a pytest fixture means that it will be cached until cleanup of the fixture's scope, which is always after a test case.

I haven't really found a good way to instantiate the widget in the test case itself but still share the test logic. Rewriting the fixture to return a "widget factory" might be an option, but will harm readability of all test cases. See https://docs.pytest.org/en/stable/how-to/fixtures.html#factories-as-fixtures.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've at least moved most of the shared code to assertions.py.

However, there seems to be no easy way to work around fixture magic to ensure that there are other references to the widget. Even if there was, I'd be afraid if it being brittle and breaking with pytest changes, so I'd prefer to explicitly initialize the widget as part of the test cases.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hrmp, even that does not seem to work and still leaves references in Python 3.10 (though it passes on Python 3.12). Reverted for now.

Copy link
Member

Choose a reason for hiding this comment

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

We might not be able to use fixtures, but we could still use a factory function so that the logic of the test isn't replicated:

def _cleanup_test(Widget, args=None, kwargs=None):
    async def test_cleanup():
        widget = Widget(
            *(args if args is not None else ()),
            **(kwargs if kwargs is not None else {}),
        )
        ref = weakref.ref(widget)

        del widget
        gc.collect()

        assert ref() is None

    return test_cleanup

adding in the extra logic for a generic skip and xfail as well; usage is then:

test_cleanup = _cleanup_test(Box)

Copy link
Member Author

@samschott samschott Sep 15, 2024

Choose a reason for hiding this comment

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

Fair point, something like this should work. I've only run into subtle issues with this for "container" widgets where content widgets are passed as arguments. (Edit: Not passing any content would make the test cases a lot less meaningful for those widgets.)

Generally, widgets passed in args / kwargs will hold a backref to the container and therefore need to be cleanup of as well before the assertion, instead of after the args and kwargs go out of scope.

On Windows, there seems to be an additional issue with those argument widgets being created on a different thread than where the widget under test is created. I'll see if this can be solved somehow without passing a full create_widget_under_test function to the test case.

@samschott samschott force-pushed the gc-audit branch 7 times, most recently from 5a6248b to 09a97de Compare September 14, 2024 22:36
@samschott
Copy link
Member Author

I had completely forgotten about this PR, thanks for the reminder! Cleaned it up a bit:

  1. Added missing tests to some new widgets.
  2. Added xfails to widgets / platforms with memory leaks. I chosen xfail over skip for now, to ensure that I don't accidentally add superfluous skips, and that they get cleaned up whenever leaks are fixed "accidentally".

@samschott samschott force-pushed the gc-audit branch 2 times, most recently from 50d25a9 to 9fc08c3 Compare September 15, 2024 15:30
Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

Looks good. There's obviously a follow up task to actually address the memory leaks; I'll open a separate ticket to track this.

@freakboy3742 freakboy3742 merged commit 3ee8b12 into beeware:main Sep 15, 2024
38 checks passed
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.

2 participants