Fragile D-Bus code in trunks |
||||
Issue descriptionsrc/aosp/system/tpm/trunks/mock_power_manager_proxy.h implements org::chromium::PowerManagerProxyInterface, which I think is a generated D-Bus interface for powerd. It overrides all D-Bus methods exported by powerd, almost all of which trunks doesn't call. When people make changes to powerd's D-Bus interface and update src/platform2/power_manager/dbus_bindings/org.chromium.PowerManager.xml accordingly, the trunks unit tests will apparently fail to build unless trunks's mock class is also updated. This happened in https://chromium-review.googlesource.com/c/541304: trunks-0.0.1-r1880: FAILED: aosp/system/tpm/trunks/trunks_testrunner.power_manager_test.o trunks-0.0.1-r1880: x86_64-cros-linux-gnu-clang++ -MMD -MF aosp/system/tpm/trunks/trunks_testrunner.power_manager_test.o.d -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64 -I/build/poppy/tmp/portage/chromeos-base/trunks-0.0.1-r1880/work/trunks-0.0.1/aosp/system/tpm -Iaosp/system/tpm/trunks/trunks_testrunner.gen/include -Igen/include -I/build/poppy/tmp/portage/chromeos-base/trunks-0.0.1-r1880/work/trunks-0.0.1/platform2 -I/build/poppy/tmp/portage/chromeos-base/trunks-0.0.1-r1880/work/trunks-0.0.1/platform -I/build/poppy/usr/include -Wall -Wno-psabi -ggdb3 -fstack-protector-strong -Wformat=2 -fvisibility=internal -Wa,--noexecstack -Werror --sysroot=/build/poppy -DUSE_RTTI_FOR_TYPE_TAGS -Wno-c++11-extensions -Wno-unused-local-typedefs -DBASE_VER=395517 -pthread -I/build/poppy/usr/include/chromeos -I/build/poppy/usr/include/base-395517 -I/build/poppy/usr/include/glib-2.0 -I/build/poppy/usr/lib64/glib-2.0/include -I/build/poppy/usr/include/nss -I/build/poppy/usr/include/nspr -I/build/poppy/usr/include/dbus-1.0 -I/build/poppy/usr/lib64/dbus-1.0/include -fPIE -std=gnu++11 -DVCSID='"0.0.1-r1880-aa9d5df099f95246f2c843a87d6c8a6ad66b904d"' -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -O2 -pipe -O2 -pipe -O2 -pipe -march=corei7 -g -fno-exceptions -fno-unwind-tables -fno-asynchronous-unwind-tables -clang-syntax -clang-syntax -fno-exceptions -fno-unwind-tables -fno-asynchronous-unwind-tables -c ../../../../../../../tmp/portage/chromeos-base/trunks-0.0.1-r1880/work/trunks-0.0.1/aosp/system/tpm/trunks/power_manager_test.cc -o aosp/system/tpm/trunks/trunks_testrunner.power_manager_test.o trunks-0.0.1-r1880: ../../../../../../../tmp/portage/chromeos-base/trunks-0.0.1-r1880/work/trunks-0.0.1/aosp/system/tpm/trunks/power_manager_test.cc:280:37: error: field type 'StrictMock<trunks::MockPowerManagerProxy>' is an abstract class trunks-0.0.1-r1880: StrictMock<MockPowerManagerProxy> proxy_{object_proxy_.get()}; trunks-0.0.1-r1880: ^ trunks-0.0.1-r1880: ../../../../../../../usr/include/power_manager-client/power_manager/dbus-proxies.h:233:16: note: unimplemented pure virtual method 'IgnoreNextPowerButtonPress' in 'StrictMock' trunks-0.0.1-r1880: virtual bool IgnoreNextPowerButtonPress( trunks-0.0.1-r1880: ^ trunks-0.0.1-r1880: ../../../../../../../usr/include/power_manager-client/power_manager/dbus-proxies.h:241:16: note: unimplemented pure virtual method 'IgnoreNextPowerButtonPressAsync' in 'StrictMock' trunks-0.0.1-r1880: virtual void IgnoreNextPowerButtonPressAsync( trunks-0.0.1-r1880: ^ trunks-0.0.1-r1880: ../../../../../../../tmp/portage/chromeos-base/trunks-0.0.1-r1880/work/trunks-0.0.1/aosp/system/tpm/trunks/power_manager_test.cc:300:3: error: cannot initialize object parameter of type 'trunks::PowerManagerTest' with an expression of type 'trunks::PowerManagerTest_StartSuccess_Test' trunks-0.0.1-r1880: NormalStart(); ... Please find a way to test trunks's D-Bus code that doesn't require trunks to be updated every time a change is made to powerd's D-Bus interface. For example, is it possible to also automatically generate this mock class?
,
Jun 22 2017
The following revision refers to this bug: https://chromium.googlesource.com/aosp/platform/system/tpm/+/7c315e77c52c9b981a833070f70dd7889cf90d16 commit 7c315e77c52c9b981a833070f70dd7889cf90d16 Author: Vincent Palatin <vpalatin@chromium.org> Date: Thu Jun 22 19:27:43 2017 trunks: update power_manager mock Add the new IgnoreNextPowerButtonPress method to the power_manager proxy after updating the D-Bus API. BUG= chromium:735764 , b:35545754 TEST='FEATURES=test emerge power_manager power_manager-client trunks' CQ-DEPEND=CL:541304 Change-Id: Iaa7d5ff289d0a322fda2c4fc8fbda5e1ab973a86 Reviewed-on: https://chromium-review.googlesource.com/544296 Commit-Ready: Vincent Palatin <vpalatin@chromium.org> Tested-by: Vincent Palatin <vpalatin@chromium.org> Reviewed-by: Dan Erat <derat@chromium.org> [modify] https://crrev.com/7c315e77c52c9b981a833070f70dd7889cf90d16/trunks/mock_power_manager_proxy.h
,
Jun 23 2017
A random idea: Is it possible to move 'tpm' or just 'trunks' to platform2? That'd allow us to update the powerd's D-Bus interface and the trunks code in a single patch.
,
Jun 23 2017
#3: Possibly (per issue 690513), but I don't want to update other packages' test code unnecessarily whenever I make a powerd change even if it's in the same repository. :-) It sounds like the right fix is for trunks to just use generated mocks instead. Andrey, do you think you'll be able to take a look at doing that soon?
,
Jun 23 2017
I've got it.
,
Jun 24 2017
trunks also rolls its own dbus::MockObjectProxy, but trying to swap that out for the libchrome-provided one won't work yet because the relevant methods aren't mocked out in the libchrome revision we're currently on. This can be fixed after we uprev libchrome, but it's a very odd position to be in otherwise.
,
Jun 24 2017
The following revision refers to this bug: https://chromium.googlesource.com/aosp/platform/system/tpm/+/a729103393b9d1bfac8840f394bf7fbae47f665d commit a729103393b9d1bfac8840f394bf7fbae47f665d Author: Eric Caruso <ejcaruso@chromium.org> Date: Sat Jun 24 03:57:51 2017 trunks: fix power_manager tests to use generated mock This prevents fragility when the D-Bus interface for power_manager changes. Instead of keeping a separate header that has to be updated in lockstep, just incorporate the libpower_manager-client-test package so we have access to a generated mock. BUG= chromium:735764 TEST=unit tests Change-Id: Ieb45bd9f3b7a916dc288657bdba043eed243a29a Reviewed-on: https://chromium-review.googlesource.com/546336 Commit-Ready: Eric Caruso <ejcaruso@chromium.org> Tested-by: Eric Caruso <ejcaruso@chromium.org> Reviewed-by: Dan Erat <derat@chromium.org> [modify] https://crrev.com/a729103393b9d1bfac8840f394bf7fbae47f665d/trunks/power_manager_test.cc [modify] https://crrev.com/a729103393b9d1bfac8840f394bf7fbae47f665d/trunks/power_manager.h [modify] https://crrev.com/a729103393b9d1bfac8840f394bf7fbae47f665d/trunks/trunks.gyp [delete] https://crrev.com/7c315e77c52c9b981a833070f70dd7889cf90d16/trunks/mock_power_manager_proxy.h
,
Jun 26 2017
,
Jan 22 2018
|
||||
►
Sign in to add a comment |
||||
Comment 1 by ejcaruso@chromium.org
, Jun 22 2017