The current D-Bus tests have a few problems: * they embed significant amounts of extra goo in libdbus and the binaries, requiring them to be disabled if you care about size or performance (i.e. they need to be switched off in production builds) * it's difficult to run a single test (I added some support for this to bus-test, but it's a hack) * they can't test an installed libdbus, because they're statically linked against libdbus-internal.la (which is not actually libdbus at all, although it's compiled from the same source code!) * they lack nice assertion macros which provide clearer messages on failure, like g_assert_cmpstr() Rather than reinventing more strangely-licensed wheels, I propose to use GLib's "GTest" module, and use it to implement some black-box tests. The patches I'm about to attach have already uncovered a bug, Bug #34569; the patch from that bug needs to be applied, for the test added by the last patch here to pass.
Created attachment 43658 [details] [review] Don't disable GLib assertions when disabling our own assertions We no longer use GLib internally, and assertions are how it'll report test failures when we add GTest-based tests.
Created attachment 43659 [details] [review] Add support for building "modular" tests, which require GLib and dbus-glib For the moment, the CMake build system only knows about the existing "embedded tests"; make it define both symbols, though. We use GLib because it has GTester (and life's too short to write yet another JUnit clone), and dbus-glib for the main-loop integration only (see fd.o #31515 for thoughts on incorporating just those two functions in a separate library in the dbus tarball). I'm not using DBusLoop for the main loop because I specifically don't want to use non-public API or ABI of libdbus in the modular tests. If we make sure they work against a shared libdbus, we can use them to test the installed system, with "make installcheck".
Created attachment 43660 [details] [review] Add an end-to-end sanity check for TCP and Unix DBusServer/DBusConnection
Created attachment 43661 [details] [review] Test nonce-tcp transport (This demonstrates Bug #34569; the patch from that bug needs to be applied before this test can be added.)
Integration tests look awesome. I don't think they replace the existing tests though, or really could... in your list of issues with those tests, you're omitting the advantages, the main one is they are unit tests, not integration tests - they can test private APIs and often do. A more subjective advantage is that they are maintained in the same source file as the stuff under test, which is good (and bad) for the same reasons inline docs are good (and bad). A big win of an integration test could be ability to test other implementations of dbus.
(In reply to comment #5) > Integration tests look awesome. Is that a positive review/permission to merge? :-) > I don't think they replace the existing tests > though, or really could... in your list of issues with those tests, you're > omitting the advantages, the main one is they are unit tests, not integration > tests - they can test private APIs and often do. Sure, we should keep the existing unit tests too; you may notice my patches don't delete anything. However, the current tests can't test an installed libdbus. Since they're statically linked against objects compiled from the same source as libdbus with different compiler options, you might consider them to not test libdbus at all... In the other libraries I work on (Telepathy's GLib and Qt bindings), we have a policy of testing as much as possible via public API (using lcov to check coverage where needed), and using static linking/internal headers to access non-public API only where there's no public way to test a particular case. After all, the public API is the only thing our users are interested in. > A more subjective advantage is > that they are maintained in the same source file as the stuff under test, which > is good (and bad) for the same reasons inline docs are good (and bad). I'm hesitant to say this is such a big advantage for tests; it gets good code coverage by hooking into the code's internals, but can also lead to the tests being so tightly coupled to the implementation that they're testing implementation details as much as high-level behaviour (particularly when the tests are in the same translation unit and use access to static functions). If nothing else, the high-level behaviour deserves tests too. > A big win of an integration test could be ability to test other implementations > of dbus. I'm not sure whether we're using the same definition of "integration test"; perhaps my definition isn't right. The feature I'm requesting is the thing added by the patch I provided, whatever you want to call that :-) Having interoperability tests would be good too, but what I'm interested in here is having black-box tests of the public D-Bus API, as a bunch of binaries that are linked against the shared libdbus-1, exit 0 on success, raise SIGABRT on failure, and can be run individually (or even with command-line options for finer granularity, here) to run a single isolated test when debugging failures. Ideally we can implement "make installcheck" with them too. For now, the thing being tested is the public API of libdbus; in future I hope to expand that to "libdbus and dbus-daemon" (at which point, sure, we could run the same code against the ndesk-dbus C# reimplementation of dbus-daemon).
Created attachment 43812 [details] [review] loopback test: unref messages after use (To be squashed into Attachment #43660 [details].)
Created attachment 43813 [details] [review] Attempt to reproduce fd.o #34393 via another regression test This doesn't actually reproduce the crash, but it seems a good thing to have anyway.
Comment on attachment 43661 [details] [review] Test nonce-tcp transport Moved to Bug #34569.
Created attachment 44349 [details] [review] Give the tests DBUS_TEST_DAEMON and DBUS_TEST_DATA in their environment This will allow modular tests to spawn a dbus-daemon with a specified config file; nothing uses this just yet.
Created attachment 44351 [details] [review] Run integration tests on the installed dbus binaries during installcheck Comment #6: > Ideally we can implement "make installcheck" with them too. Done!
Created attachment 44352 [details] [review] Add a simple integration test for dbus-daemon This just pushes 2000 messages (or 100000 in performance-testing mode) through the dbus-daemon, to an echo service and back. --- Comment #6: > For now, the thing being tested is the public API of libdbus; in future I hope > to expand that to "libdbus and dbus-daemon" (at which point, sure, we could run > the same code against the ndesk-dbus C# reimplementation of dbus-daemon). Also done. (It's a trivial test, but we can expand coverage over time.)
Attachment #44313 [details] can also be applied (Reviewed-by: davidz) as soon as this branch is.
Personally, I'd prefer integration tests to be a separate module. (I plan to eventually split out most of the GLib tests, even if I have to twist mclasen's arm to do so) As far as the definition: http://en.wikipedia.org/wiki/Integration_testing The point here concretely is that what OS builders really want is to build the deliverable, then install a bunch of tests associated with components of the deliverable, and run them all at once. This is difficult for "distributions" because they are just sets of packages that one can mix and match, so integration testing is harder because of combinatorial interactions. However if I was using say Yocto to build an OS image for my Linux-based watch or whatever, what I would really want is the ability to install tests for both DBus and GLib that only use public API from both, and run them on the final device image. As an example of a concrete difference between unit and integration tests in this context would be that the tests run in the target environment and not say on ancient Linux kernels that tend to be run on build machines. So - make a new git repository dbus/dbus-tests ? In this model, consider we could also install tests for the system bus (test services, etc.).
Created attachment 47723 [details] [review] Add support for installing most of the modular tests This is just the low-hanging fruit - test-dbus-daemon is a little harder. (This requires all earlier patches from this bug, the patch that davidz already reviewed on Bug #34569, and the two patches from my attempts to make tests on Bug #15578. See also my installable-tests-34570 branch, which has the whole lot rebased to apply cleanly to dbus-1.4.)
Created attachment 47724 [details] [review] Alter test-dbus-daemon so it tests the installed dbus-daemon by default For installcheck, adjust it to use things from DESTDIR.
This branch now adds tests that can work in four ways: * in-tree, using the just-compiled libdbus and dbus-daemon (make check) * in-tree, using the just-compiled libdbus and the just-installed dbus-daemon and configuration file (make installcheck) * in-tree, using the just-installed libdbus and dbus-daemon (make installcheck with --enable-installed-tests - but I now realise this ought to be removed, since it doesn't set the right LD_LIBRARY_PATH to use the libdbus from the DESTDIR, and if it did it wouldn't be portable) * out-of-tree, using the system libdbus and dbus-daemon (--enable-installed-tests, make install, and run /usr/lib/dbus-1.0/test/test-dbus-daemon or whatever) (In reply to comment #14) > The point here concretely is that what OS builders really want is to build the > deliverable, then install a bunch of tests associated with components of the > deliverable, and run them all at once. To some extent, we do that in Maemo by extracting tests from packages - it's usually a pain to do, because tests typically expect a lot of the source package's infrastructure to be present. dbus is currently one of the packages where we never bothered, because the tests don't actually even act on the shared library, and because building them requires a complete second copy of dbus. The tests added by this branch would be entirely suitable, though. > So - make a new git repository dbus/dbus-tests ? As discussed on IRC, I'd really rather not raise the bar for adding tests high enough for people to trip over it (I seem to be the only person adding any tests as it is), and having the code and the tests share a repository avoids having to think about version skew between the two. > In this model, consider we could also install tests for the system bus (test > services, etc.). In the flying car future, sure, why not? But for now, I'd like to stick to tests that developers can run in "make check" and not have to think about, otherwise nobody will ever run them.
Created attachment 47725 [details] [review] installcheck: don't run installed tests against installed library if in a DESTDIR That probably won't work, because it'll find the system-wide library which might be older.
Review of attachment 44352 [details] [review]: ::: test/dbus-daemon.c @@ +249,3 @@ + g_error ("OOM"); + + if (!dbus_connection_send_with_reply (f->left_conn, m, &pc, 0x7FFFFFFF) is there no #define for INT_MAX?
(In reply to comment #19) > Review of attachment 44352 [details] [review]: > > ::: test/dbus-daemon.c > @@ +249,3 @@ > + g_error ("OOM"); > + > + if (!dbus_connection_send_with_reply (f->left_conn, m, &pc, 0x7FFFFFFF) > > is there no #define for INT_MAX? The documentation says INT_MAX (which doesn't seem to exist), but it really means "a maximal int32". dbus-pending-call checks for equality with _DBUS_INT_MAX, which is actually unconditionally _DBUS_INT32_MAX; neither is exposed as API, because that would be making life easy for libdbus users, so obviously isn't allowed. dbus implicitly assumes int has at least 32 bits; I infer that it's meant to work with larger ints (ILP64), although in practice it'll fail to build because if int is 64 bits, then we won't be able to find both a 32-bit type and a 16-bit type (they can't both be short). In a better but still sub-ideal world, we'd have: #define DBUS_TIMEOUT_INFINITE ((int) 0x7fffffff) or even just expose the generically-useful type-limit macros like GLib does. (In an ideal world that only tangentially resembles the one we live in, all platforms would have implemented a 12 year old C standard by now, and we could stop caring about people who can't #include <stdint.h>.)
Created attachment 47816 [details] [review] Add and use DBUS_TIMEOUT_INFINITE and DBUS_TIMEOUT_USE_DEFAULT The documentation claimed that INT_MAX (whatever that means) meant the default, but the value that has actually always been checked for is 0x7fffffff (aka INT32_MAX on the competent platforms we sadly don't restrict our portability to), so we should use that. (In practice D-Bus probably never worked on platforms where int wasn't 32 bits, though.)
Created attachment 47817 [details] [review] Use DBUS_TIMEOUT_INFINITE in dbus-daemon.c Amends Attachment #44352 [details]. Branch: dbus-timeout-infinite.
Review of attachment 47816 [details] [review]: Looks good.
All fixed in git for 1.4.12, will be merged to master before 1.5.4.
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.