Created attachment 132450 [details] [review] [1/3] name-test: Backport dbus-run-session wrapper from git master test-pending-call-disconnected relies on being run under a session bus. On master, the TESTS in this directory all get that treatment, but in dbus-1.10 they do not. This caused test-pending-call-disconnected to fail in minimal environments like travis-ci where there is no developer-initiated session bus. Backport part of commit ec6b220 "name-test: run most C tests directly, not via run-test.sh" to wrap it in dbus-run-session. This is better than putting it in run-test.sh because this way, its TAP output is parsed directly by Automake. It also has the side benefit of exercising dbus-run-session in the automated tests.
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.