Install pytest based tests #1525

swick · 2024-12-04T19:09:39Z

I personally don't really understand the value of installed tests, so this is more of an question if it makes sense to do it. The plan is to remove the C based tests soon which would mean we'd have no more installed tests.

Only the last commit is relevant.

/cc @smcv

swick · 2024-12-04T21:39:54Z

The part that's quite awkward is that pytest-tap isn't in the ubuntu repos which means I have to install it using pip into the system for the installed tests to work.

The alternative is to just not use TAP output. Again, I don't know enough about those tests to make a reasonable decision here.

smcv · 2024-12-05T15:12:33Z

I personally don't really understand the value of installed tests, so this is more of an question if it makes sense to do it.

Most of the value of as-installed tests is that distributions can re-run the test against a new version of a dependency (like GLib or pytest), without recompiling the dependent software (in this case xdg-desktop-portal), to simulate what will happen on end-user systems in apt upgrade or equivalent.

This avoids spending time (a lot of time in some cases) on recompiling the dependent software. More importantly, it avoids hiding problems - if a library like GLib breaks its ABI, recompiling the dependent software under test against the new version will usually work around the ABI break by building new binaries that target the new ABI, whereas re-running previously-compiled tests (which expect the old ABI) will detect the ABI break so that it can be investigated and fixed.

Debian and Ubuntu routinely do this, via the autopkgtest framework.

A secondary reason to use as-installed tests is that they are often running in a less "fake" environment than build-time tests. For example, during as-installed tests, we can assume that x-d-p is a fully working D-Bus-activatable service, so if we let the message bus start it via service activation, that will detect any brokenness that might exist in its .service file. Similarly, we implicitly check for its ability to load the "real" bwrap from the PATH and its ability to find the "real" xdg-desktop-portal-validate-* executables in /usr/libexec; and unlike build-time tests being run by a distro autobuilder, the as-installed tests will usually not be running in a chroot or a heavily-constrained container, so we won't usually need workarounds like #1498.

Debian and Ubuntu currently run as-installed tests in either privileged lxc, some sort of lxd (privileged, I think), or a qemu VM, depending on CPU architecture, with Podman as a possible future replacement for lxc, and it is possible to flag tests as isolation-machine (meaning "needs machine-level testbed isolation"; we do this for xdg-desktop-portal) so that they will automatically be skipped on the architectures that are not using qemu. This makes the test environment a much more realistic reflection of what will happen on end-user systems than the heavily-constrained environment of an autobuilder.

I think Fedora also has some amount of "as-installed" testing, although I don't know the finer details. I believe Fedora uses mock (a restricted chroot/container vaguely similar to Debian's schroot) for autobuilders and build-time testing, but then uses qemu or some sort of more permissive container technology for "as-installed" testing.

swick · 2024-12-05T18:30:05Z

Thanks for the thorough explanation.

The first reason would indicate that we should install the pytest based tests as well.

The second reason cannot be realized with the pytest based tests because they tightly control startup of all components themselves. The framework is flexible enough that maybe one could add a special mode for installed tests. The current C portal tests also tightly control the startup so that isn't a regression. Some of the other C based tests however rely on a less "fake" environment.

Given that I want to drop all the C tests, do you have concerns with that?

smcv · 2024-12-05T18:47:53Z

The part that's quite awkward is that pytest-tap isn't in the ubuntu repos
...
The alternative is to just not use TAP output.

TAP output is a nice-to-have, but I don't consider it to be important.

We have https://github.com/python-tap/tappy in Debian/Ubuntu (as python3-tap), but I don't think we have its pytest plugin.

smcv · 2024-12-05T18:49:32Z

The second reason cannot be realized with the pytest based tests because they tightly control startup of all components themselves. The framework is flexible enough that maybe one could add a special mode for installed tests. The current C portal tests also tightly control the startup so that isn't a regression.

I think that's reasonable: I can see that the tests for x-d-p probably do need to exercise quite tight control over it so that it will select mock portals (which are testable) instead of the real implementations (which are not).

swick · 2025-01-07T18:12:21Z

@smcv Would be great if you could review this one. We now have all the pytest based integration tests.

smcv

I'd like to test this with Debian's autopkgtest before saying yes or no to it, but it looks plausible. Thanks!

tests/meson.build

swick · 2025-01-15T19:30:03Z

Hey @smcv. I don't want to rush you but Georges mentioned that he wants to release x-d-p soon and I'd like to get this one in if possible.

smcv · 2025-01-16T19:39:54Z

I don't want to rush you but Georges mentioned that he wants to release x-d-p soon and I'd like to get this one in if possible.

I'm trying. As a first step (to make the tests pass without this change): #1577.

smcv · 2025-01-16T21:47:22Z

https://github.com/smcv/xdg-desktop-portal/tree/installed-pytest mostly works, but I'm seeing one failure - in test_document_fuse.py::TestDocumentFuse::test_multi_thread, g-io-error-quark: Error calling StartServiceByName for org.freedesktop.portal.Documents: The connection is closed (18) - and a lot of warnings.

If we can tell pytest to use a different cache directory (environment variable?), I think that would help to avoid warnings like this:

214s /usr/lib/python3/dist-packages/_pytest/stepwise.py:51: PytestCacheWarning: could not create cache path /usr/libexec/installed-tests/xdg-desktop-portal/tests/.pytest_cache/v/cache/stepwise: [Errno 13] Permission denied: '/usr/libexec/installed-tests/xdg-desktop-portal/tests/pytest-cache-files-btnx48xn'

swick · 2025-01-17T14:21:16Z

I've addressed everything except the TestDocumentFuse failure which I can now reproduce locally and without installing. This might be tricky to resolve, so I'm inclined to maybe skip the test for now. WDYT?

swick · 2025-01-17T14:27:39Z

Funnily, ./tests/run-test.sh -n 0 tests/test_document_fuse.py::TestDocumentFuse::test_multi_thread works fine, but running ./tests/run-test.sh -n 0 tests/test_document_fuse.py::TestDocumentFuse makes test_multi_thread fail, so this somehow is connected to the previous test messing up some state.

swick · 2025-01-20T16:46:44Z

@smcv I've added a commit which disables the racy test for now. I can remove it if we'd rather have to racy test in there. Just tell me your preference. @GeorgesStavracas wants to release later today and says that you'd have to approve this PR for it to land in the release.

smcv · 2025-01-20T17:24:42Z

Re-testing this now

smcv · 2025-01-20T17:53:27Z

I've added a commit which disables the racy test for now

I think that's appropriate, given that it fails for you too.

If it works reliably when the two test-cases are not combined into one run, then a nice followup would be to generate two separate .test files, each of which runs one of the two test-cases (single- and multi-threaded), as two separate processes - but that's not a blocker for this IMO. (Or obviously if someone can figure out the actual problem and fix it, e.g. by cleaning up state more thoroughly between the two test-cases, that would be even better.)

smcv

Mostly LGTM. I merged the two prerequisites #1578 and #1579, so it might benefit from rebasing.

smcv · 2025-01-20T18:03:55Z

tests/meson.build

+
+      exec = [pytest.full_path(), installed_tests_dir / 'tests']
+      exec += pytest_args
+      exec += ['-o', 'cache_dir=/tmp/xdp-pytest-cache']


Let's not introduce /tmp symlink attacks via hard-coded filenames in /tmp, please. (protected_symlinks mitigates this into only being a denial of service, but it's still best avoided.)

Can we use -o cache_dir=. or -o cache_dir=/proc/self/cwd, relying on the fact that ginsttest-runner runs each test with its current working directory set to a temporary directory that it automatically cleans up afterwards?

Or perhaps -p no:cacheprovider?

relying on the fact that ginsttest-runner runs each test with its current working directory set to a temporary directory

Reference: https://wiki.gnome.org/Initiatives(2f)GnomeGoals(2f)InstalledTests.html

Testing https://github.com/smcv/xdg-desktop-portal/tree/wip/pytest-installed now

Yes, that works. Please consider applying the last commit of that branch, either squashed into build/tests: Install pytest based tests or separately.

wtf. It works, and says cachedir: ., but I still get (non-fatal!) warnings about attempting to write into /usr/libexec/installed-tests. Sorry, I don't understand pytest well enough to diagnose this... but the tests pass so that's an excellent start.

Went for -p no:cacheprovider and it seems to work well. Would appreciate if you could verify it as well.

Yeah, this works nicely

If the key is not in the dict, pop still works without throwing any errors. This is just a bit safer.

Running ./tests/run-test.sh -n 0 \ tests/test_document_fuse.py::TestDocumentFuse::test_multi_thread works fine, but with ./tests/run-test.sh -n 0 \ tests/test_document_fuse.py::TestDocumentFuse the `test_multi_thread` test is failing. For now, let's skip the test and turn it on again when we have fixed it.

GeorgesStavracas · 2025-01-20T20:53:28Z

@smcv whenever you're happy with this PR, please feel free to merge

swick · 2025-01-20T20:59:49Z

Thanks a lot Simon!

swick force-pushed the wip/pytest-installed branch 2 times, most recently from a99bb49 to 5362611 Compare December 4, 2024 20:37

swick mentioned this pull request Dec 4, 2024

Remove C based integration tests #1526

Draft

swick force-pushed the wip/pytest-installed branch from 5362611 to dd4ec29 Compare December 4, 2024 21:38

swick force-pushed the wip/pytest-installed branch from dd4ec29 to 435c5bb Compare January 7, 2025 18:09

swick marked this pull request as ready for review January 7, 2025 18:10

swick force-pushed the wip/pytest-installed branch from 435c5bb to 504333f Compare January 7, 2025 18:48

smcv reviewed Jan 8, 2025

View reviewed changes

tests/meson.build Outdated Show resolved Hide resolved

swick force-pushed the wip/pytest-installed branch from 504333f to 99efdb6 Compare January 9, 2025 15:43

smcv mentioned this pull request Jan 16, 2025

tests: Translate pytest exit status to Meson/ginsttest-runner convention #1577

Closed

swick force-pushed the wip/pytest-installed branch from 99efdb6 to 05ae3ae Compare January 17, 2025 14:16

swick force-pushed the wip/pytest-installed branch from 05ae3ae to a9cff70 Compare January 20, 2025 16:45

smcv self-requested a review January 20, 2025 17:24

smcv requested changes Jan 20, 2025

View reviewed changes

swick added 2 commits January 20, 2025 20:38

tests/conftest.py: Use dict.pop instead of del to avoid KeyErrors

8814703

If the key is not in the dict, pop still works without throwing any errors. This is just a bit safer.

build/tests: Install pytest based tests

083b69a

swick force-pushed the wip/pytest-installed branch from a9cff70 to d29db4a Compare January 20, 2025 20:15

smcv approved these changes Jan 20, 2025

View reviewed changes

smcv added this pull request to the merge queue Jan 20, 2025

GeorgesStavracas added this to the 1.20 milestone Jan 20, 2025

GeorgesStavracas added the tests Test suite label Jan 20, 2025

Merged via the queue into flatpak:main with commit d6abe3c Jan 20, 2025
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Install pytest based tests #1525

Install pytest based tests #1525

swick commented Dec 4, 2024 •

edited

Loading

swick commented Dec 4, 2024

smcv commented Dec 5, 2024

swick commented Dec 5, 2024

smcv commented Dec 5, 2024

smcv commented Dec 5, 2024

swick commented Jan 7, 2025

smcv left a comment

swick commented Jan 15, 2025

smcv commented Jan 16, 2025

smcv commented Jan 16, 2025

swick commented Jan 17, 2025 •

edited

Loading

swick commented Jan 17, 2025

swick commented Jan 20, 2025

smcv commented Jan 20, 2025

smcv commented Jan 20, 2025

smcv left a comment

smcv Jan 20, 2025

smcv Jan 20, 2025

smcv Jan 20, 2025

smcv Jan 20, 2025

smcv Jan 20, 2025

swick Jan 20, 2025 •

edited

Loading

smcv Jan 20, 2025

smcv Jan 20, 2025

GeorgesStavracas commented Jan 20, 2025

swick commented Jan 20, 2025

Install pytest based tests #1525

Install pytest based tests #1525

Conversation

swick commented Dec 4, 2024 • edited Loading

swick commented Dec 4, 2024

smcv commented Dec 5, 2024

swick commented Dec 5, 2024

smcv commented Dec 5, 2024

smcv commented Dec 5, 2024

swick commented Jan 7, 2025

smcv left a comment

Choose a reason for hiding this comment

swick commented Jan 15, 2025

smcv commented Jan 16, 2025

smcv commented Jan 16, 2025

swick commented Jan 17, 2025 • edited Loading

swick commented Jan 17, 2025

swick commented Jan 20, 2025

smcv commented Jan 20, 2025

smcv commented Jan 20, 2025

smcv left a comment

Choose a reason for hiding this comment

smcv Jan 20, 2025

Choose a reason for hiding this comment

smcv Jan 20, 2025

Choose a reason for hiding this comment

smcv Jan 20, 2025

Choose a reason for hiding this comment

smcv Jan 20, 2025

Choose a reason for hiding this comment

smcv Jan 20, 2025

Choose a reason for hiding this comment

swick Jan 20, 2025 • edited Loading

Choose a reason for hiding this comment

smcv Jan 20, 2025

Choose a reason for hiding this comment

smcv Jan 20, 2025

Choose a reason for hiding this comment

GeorgesStavracas commented Jan 20, 2025

swick commented Jan 20, 2025

swick commented Dec 4, 2024 •

edited

Loading

swick commented Jan 17, 2025 •

edited

Loading

swick Jan 20, 2025 •

edited

Loading