Consider removing repetitive dbus.SystemBus calls in Tast tests |
||
Issue description
A bunch of Tast tests contain the following code:
bus, err := dbus.SystemBus()
if err != nil {
return nil, err
}
This is repetitive, and I'm worried that it may be easy for test authors to accidentally close the returned bus (which could affect other tests).
---
From https://chromium-review.googlesource.com/c/chromiumos/platform/tast-tests/+/1258703/2..4/src/chromiumos/tast/local/dbusutil/service.go#65
derat@: "as an aside, i'm wondering if we should have some function that takes a *testing.State and calls s.Fatal() if dbus.SystemBus() returns an error. i don't think that it should ever fail (unless dbus-daemon isn't running, in which case the system is totally broken), so it's a bit painful to need to repeat it in every test. it'd probably look like:
func SystemBus(ctx context.Context, s *testing.State) *dbus.Conn"
hidehiko@: "Yes, that's an option, but I wonder if we'd like to bring testing.State till here.
This is a utility to make the implementation of D-Bus wrapper structs easier, and in those cases, anyway it checks an error (e.g., timeout around L73) and thus that function wouldn't help more simplification unfortunately, I think.
Rather, if the failure of dbus.SystemBus() indicates the system is totally broken,
how about quickly checking it once before starting the test to reduce amount of less meaningful test running time?"
,
Oct 5
Re #1: yes, MustGetSystemBus approach sounds good to me.
,
Oct 9
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform/tast-tests/+/a5ca1e4165ebe328e0804b354f233af6a78b4088 commit a5ca1e4165ebe328e0804b354f233af6a78b4088 Author: Daniel Erat <derat@chromium.org> Date: Tue Oct 09 17:36:40 2018 tast-tests: Simplify D-Bus usage in local tests. Clean up D-Bus usage across local tests: - Make more test code call dbusutil.Connect. - Cache D-Bus objects in the vm package. - Remove exported service constants from dbusutil since they were all only being used by single packages. - Remove unnecessary (?) component updater waiting from vm.StartCrosvm test. BUG= chromium:892650 TEST=ran example.DBus, audio.*, network.*, and vm.* tests Change-Id: I57911eaca805e6a234e5baf5406f3fd3207800c9 Reviewed-on: https://chromium-review.googlesource.com/1266635 Commit-Ready: Dan Erat <derat@chromium.org> Tested-by: Dan Erat <derat@chromium.org> Reviewed-by: Jeffrey Kardatzke <jkardatzke@google.com> Reviewed-by: Hidehiko Abe <hidehiko@chromium.org> [modify] https://crrev.com/a5ca1e4165ebe328e0804b354f233af6a78b4088/src/chromiumos/tast/local/dbusutil/service.go [add] https://crrev.com/a5ca1e4165ebe328e0804b354f233af6a78b4088/src/chromiumos/tast/local/dbusutil/bus.go [modify] https://crrev.com/a5ca1e4165ebe328e0804b354f233af6a78b4088/src/chromiumos/tast/local/vm/termina.go [modify] https://crrev.com/a5ca1e4165ebe328e0804b354f233af6a78b4088/src/chromiumos/tast/local/shill/shill.go [modify] https://crrev.com/a5ca1e4165ebe328e0804b354f233af6a78b4088/src/chromiumos/tast/local/vm/vm.go [modify] https://crrev.com/a5ca1e4165ebe328e0804b354f233af6a78b4088/src/chromiumos/tast/local/bundles/cros/example/dbus.go [modify] https://crrev.com/a5ca1e4165ebe328e0804b354f233af6a78b4088/src/chromiumos/tast/local/vm/util.go [modify] https://crrev.com/a5ca1e4165ebe328e0804b354f233af6a78b4088/src/chromiumos/tast/local/chrome/chrome.go [modify] https://crrev.com/a5ca1e4165ebe328e0804b354f233af6a78b4088/src/chromiumos/tast/local/bundles/cros/vm/start_crosvm.go [modify] https://crrev.com/a5ca1e4165ebe328e0804b354f233af6a78b4088/src/chromiumos/tast/local/dbusutil/signal.go [modify] https://crrev.com/a5ca1e4165ebe328e0804b354f233af6a78b4088/src/chromiumos/tast/local/vm/concierge.go [modify] https://crrev.com/a5ca1e4165ebe328e0804b354f233af6a78b4088/src/chromiumos/tast/local/vm/container.go [delete] https://crrev.com/57ea0e5aaf8c2be7033d6af936a394986aee0807/src/chromiumos/tast/local/dbusutil/constants.go [modify] https://crrev.com/a5ca1e4165ebe328e0804b354f233af6a78b4088/src/chromiumos/tast/local/audio/cras.go
,
Oct 9
I was able to remove most of the repetition by updating code to call dbusutil.Connect, so this probably isn't necessary for now. |
||
►
Sign in to add a comment |
||
Comment 1 by derat@chromium.org
, Oct 5I'm not sure whether we want to check this unconditionally, as it seems possible that we could run Tast tests on some systems that don't have D-Bus (see e.g. issue 889686 -- not sure if we have D-Bus there). Go's philosophy (which I think is a good one) is that libraries shouldn't unexpectedly panic or otherwise make your program abort (which s.Fatal does). But maybe we could add something like this to dbusutil: func MustGetSystemBus() *dbus.Conn { bus, err := dbus.SystemBus() if err != nil { panic(fmt.Sprintf("Failed to connect to D-Bus system bus: %v", err)) } return bus } Does that seem reasonable? "Must" is used elsewhere in Go for things that are never expected to fail.