Summary: | [1.10] test-pending-call-disconnected relies on having a session bus | ||
---|---|---|---|
Product: | dbus | Reporter: | Simon McVittie <smcv> |
Component: | core | Assignee: | Simon McVittie <smcv> |
Status: | RESOLVED FIXED | QA Contact: | D-Bus Maintainers <dbus> |
Severity: | normal | ||
Priority: | medium | Keywords: | patch |
Version: | git master | ||
Hardware: | Other | ||
OS: | All | ||
Whiteboard: | review? | ||
i915 platform: | i915 features: | ||
Attachments: |
[1/3] name-test: Backport dbus-run-session wrapper from git master
[2/3] test/dbus-daemon: Unset DBUS_SESSION_BUS_ADDRESS [1/3] name-test: Backport dbus-run-session wrapper from git master [3/3] tests: Make tests fail if they try to connect to the real session bus [2/3] test/dbus-daemon: Unset DBUS_SESSION_BUS_ADDRESS |
Description
Simon McVittie
2017-07-05 14:49:52 UTC
Created attachment 132451 [details] [review] [2/3] test/dbus-daemon: Unset DBUS_SESSION_BUS_ADDRESS When we intend to exercise the default behaviour in the absence of XDG_RUNTIME_DIR and DBUS_SESSION_BUS_ADDRESS, it would help if we unset DBUS_SESSION_BUS_ADDRESS. Otherwise we'll just connect to the real session bus, if there is one. --- This is such a small fix that I think it should go to 1.10 as well as master. Created attachment 132452 [details] [review] [1/3] name-test: Backport dbus-run-session wrapper from git master --- This time with correct whitespace. Created attachment 132453 [details] [review] [3/3] tests: Make tests fail if they try to connect to the real session bus It is too easy for a developer working in an environment that has a session bus to write tests that pass locally, but fail in minimal environments. This is also risky because the tests might do destructive things on the developer's real session bus. We can avoid connecting to the session bus by consistently removing its address from the environment, and replacing it with something that will always fail. --- I think this would be good for dbus-1.10 too. Comment on attachment 132452 [details] [review] [1/3] name-test: Backport dbus-run-session wrapper from git master Review of attachment 132452 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 132451 [details] [review] [2/3] test/dbus-daemon: Unset DBUS_SESSION_BUS_ADDRESS Review of attachment 132451 [details] [review]: ----------------------------------------------------------------- > When we intend to exercise the default behaviour in the absence of > XDG_RUNTIME_DIR and DBUS_SESSION_BUS_ADDRESS Er, this code *sets* XDG_RUNTIME_DIR. That doesn’t look like ‘the absence of’ to me. Comment on attachment 132453 [details] [review] [3/3] tests: Make tests fail if they try to connect to the real session bus Review of attachment 132453 [details] [review]: ----------------------------------------------------------------- ++ (In reply to Philip Withnall from comment #5) > Comment on attachment 132451 [details] [review] > [2/3] test/dbus-daemon: Unset DBUS_SESSION_BUS_ADDRESS > > > When we intend to exercise the default behaviour in the absence of > > XDG_RUNTIME_DIR and DBUS_SESSION_BUS_ADDRESS > > Er, this code *sets* XDG_RUNTIME_DIR. That doesn’t look like ‘the absence > of’ to me. An entirely valid point. What we actually intend to exercise here is the default behaviour in the absence of DBUS_SESSION_BUS_ADDRESS but in the presence of XDG_RUNTIME_DIR. Does the change look OK, other than the commit message? The closest thing we have to exercising the absence of DBUS_SESSION_BUS_ADDRESS and XDG_RUNTIME_DIR is test_autolaunch() in test/test-dbus-launch-x11.sh, although that doesn't actually test the ability to run dbus-launch as a side-effect of the autolaunch: transport or no explicit transport at all. Perhaps it should. (In reply to Simon McVittie from comment #7) > (In reply to Philip Withnall from comment #5) > > Comment on attachment 132451 [details] [review] [review] > > [2/3] test/dbus-daemon: Unset DBUS_SESSION_BUS_ADDRESS > > > > > When we intend to exercise the default behaviour in the absence of > > > XDG_RUNTIME_DIR and DBUS_SESSION_BUS_ADDRESS > > > > Er, this code *sets* XDG_RUNTIME_DIR. That doesn’t look like ‘the absence > > of’ to me. > > An entirely valid point. What we actually intend to exercise here is the > default behaviour in the absence of DBUS_SESSION_BUS_ADDRESS but in the > presence of XDG_RUNTIME_DIR. Does the change look OK, other than the commit > message? The change does look OK. Just need to fix the commit message. > The closest thing we have to exercising the absence of > DBUS_SESSION_BUS_ADDRESS and XDG_RUNTIME_DIR is test_autolaunch() in > test/test-dbus-launch-x11.sh, although that doesn't actually test the > ability to run dbus-launch as a side-effect of the autolaunch: transport or > no explicit transport at all. Perhaps it should. Created attachment 132482 [details] [review] [2/3] test/dbus-daemon: Unset DBUS_SESSION_BUS_ADDRESS When we intend to exercise the default behaviour in the absence of DBUS_SESSION_BUS_ADDRESS (but with an XDG_RUNTIME_DIR present), it would help if we unset DBUS_SESSION_BUS_ADDRESS. Otherwise we'll just connect to the real session bus, if there is one. --- Also pushed [1/3] for 1.10.22, since you already reviewed it. That's enough to get travis-ci passing again. Comment on attachment 132482 [details] [review] [2/3] test/dbus-daemon: Unset DBUS_SESSION_BUS_ADDRESS Review of attachment 132482 [details] [review]: ----------------------------------------------------------------- ++ Pushed for 1.11.16 and 1.10.22, thanks for the reviews! |
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.