Bug 101698 - [1.10] test-pending-call-disconnected relies on having a session bus
Summary: [1.10] test-pending-call-disconnected relies on having a session bus
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: git master
Hardware: Other All
: medium normal
Assignee: Simon McVittie
QA Contact: D-Bus Maintainers
URL:
Whiteboard: review?
Keywords: patch
Depends on:
Blocks:
 
Reported: 2017-07-05 14:49 UTC by Simon McVittie
Modified: 2017-07-07 10:49 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
[1/3] name-test: Backport dbus-run-session wrapper from git master (2.70 KB, patch)
2017-07-05 14:49 UTC, Simon McVittie
Details | Splinter Review
[2/3] test/dbus-daemon: Unset DBUS_SESSION_BUS_ADDRESS (1.07 KB, patch)
2017-07-05 14:52 UTC, Simon McVittie
Details | Splinter Review
[1/3] name-test: Backport dbus-run-session wrapper from git master (2.70 KB, patch)
2017-07-05 14:53 UTC, Simon McVittie
Details | Splinter Review
[3/3] tests: Make tests fail if they try to connect to the real session bus (2.70 KB, patch)
2017-07-05 14:54 UTC, Simon McVittie
Details | Splinter Review
[2/3] test/dbus-daemon: Unset DBUS_SESSION_BUS_ADDRESS (1.14 KB, patch)
2017-07-06 19:05 UTC, Simon McVittie
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Simon McVittie 2017-07-05 14:49:52 UTC
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.
Comment 1 Simon McVittie 2017-07-05 14:52:39 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.
Comment 2 Simon McVittie 2017-07-05 14:53:34 UTC
Created attachment 132452 [details] [review]
[1/3] name-test: Backport dbus-run-session wrapper from git  master

---

This time with correct whitespace.
Comment 3 Simon McVittie 2017-07-05 14:54:10 UTC
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 4 Philip Withnall 2017-07-06 11:17:37 UTC
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 5 Philip Withnall 2017-07-06 11:18:46 UTC
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 6 Philip Withnall 2017-07-06 11:20:30 UTC
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]:
-----------------------------------------------------------------

++
Comment 7 Simon McVittie 2017-07-06 11:42:41 UTC
(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.
Comment 8 Philip Withnall 2017-07-06 12:36:14 UTC
(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.
Comment 9 Simon McVittie 2017-07-06 19:05:36 UTC
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 10 Philip Withnall 2017-07-06 19:15:48 UTC
Comment on attachment 132482 [details] [review]
[2/3] test/dbus-daemon: Unset DBUS_SESSION_BUS_ADDRESS

Review of attachment 132482 [details] [review]:
-----------------------------------------------------------------

++
Comment 11 Simon McVittie 2017-07-07 10:49:34 UTC
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.