Bug 107194 - Run tests under valgrind
Summary: Run tests under valgrind
Status: RESOLVED MOVED
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:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-07-11 17:33 UTC by Simon McVittie
Modified: 2018-10-12 21:35 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
[1] Assert that DBUS_SERVER_LISTEN_ADDRESS_ALREADY_USED does not set error (1.02 KB, patch)
2018-07-11 17:34 UTC, Simon McVittie
Details | Splinter Review
[2] dbus_server_listen: Assert that implementations return a known result (893 bytes, patch)
2018-07-11 17:35 UTC, Simon McVittie
Details | Splinter Review
[3] loopback: Don't free credentials borrowed from the connection (989 bytes, patch)
2018-07-11 17:35 UTC, Simon McVittie
Details | Splinter Review
[4] dbus_server_listen: Don't leak first_connect_error (858 bytes, patch)
2018-07-11 17:36 UTC, Simon McVittie
Details | Splinter Review
[5] tests: Detach server from main loop during teardown (2.55 KB, patch)
2018-07-11 17:37 UTC, Simon McVittie
Details | Splinter Review
[6] tests: Interpret empty command-line arguments as --tap (1.87 KB, patch)
2018-07-11 17:40 UTC, Simon McVittie
Details | Splinter Review
[7] tests: Call dbus_shutdown() (10.72 KB, patch)
2018-07-11 17:43 UTC, Simon McVittie
Details | Splinter Review
WIP: Use AX_VALGRIND_CHECK to set up partial valgrind test coverage (3.30 KB, patch)
2018-07-11 18:33 UTC, Simon McVittie
Details | Splinter Review
WIP: Make loopback test valgrind-clean (1.59 KB, patch)
2018-07-11 18:36 UTC, Simon McVittie
Details | Splinter Review
[8] Allow longer for tests under valgrind (1.26 KB, patch)
2018-07-12 15:57 UTC, Simon McVittie
Details | Splinter Review
[9] test/dbus-daemon: Don't leak error if no machine ID was found (802 bytes, patch)
2018-07-12 15:58 UTC, Simon McVittie
Details | Splinter Review
[10] test/dbus-daemon: Don't leak expected error for max connections (723 bytes, patch)
2018-07-12 15:58 UTC, Simon McVittie
Details | Splinter Review
[11] nonce: Don't try to rmdir(NULL) on OOM (992 bytes, patch)
2018-07-12 15:58 UTC, Simon McVittie
Details | Splinter Review
[12] test/marshal: Don't leak a message and its marshalled buffer (755 bytes, patch)
2018-07-12 15:59 UTC, Simon McVittie
Details | Splinter Review
[13] test/containers: Fix some memory leaks (1.05 KB, patch)
2018-07-12 16:00 UTC, Simon McVittie
Details | Splinter Review
[14] tests: Detach most connections from main loop before closing (10.54 KB, patch)
2018-07-12 16:00 UTC, Simon McVittie
Details | Splinter Review
[15] Skip name-test/ when running under valgrind for now (6.75 KB, patch)
2018-07-12 16:01 UTC, Simon McVittie
Details | Splinter Review
[16] dispatch test: Simplify OOM testing (3.38 KB, patch)
2018-07-12 16:02 UTC, Simon McVittie
Details | Splinter Review
[17] Don't do OOM testing under valgrind by default (1.34 KB, patch)
2018-07-12 16:05 UTC, Simon McVittie
Details | Splinter Review
[18] embedded tests: Make it easier to run a single test-case (1.41 KB, patch)
2018-08-02 22:19 UTC, Simon McVittie
Details | Splinter Review
WIP: Use AX_VALGRIND_CHECK to set up partial valgrind test coverage (4.80 KB, patch)
2018-08-02 22:20 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 2018-07-11 17:33:16 UTC
On Bug #105656, Philip suggested running the tests under valgrind. This is a larger task than one might think, because many of the tests are not fully valgrind-clean (and because valgrind doesn't currently work on Debian unstable due to binutils behaviour changes, so I'm having to test all this in a Debian 9 chroot).

I've mostly found false positives, but a couple of actual bugs.
Comment 1 Simon McVittie 2018-07-11 17:34:06 UTC
Created attachment 140563 [details] [review]
[1] Assert that DBUS_SERVER_LISTEN_ADDRESS_ALREADY_USED does not set error

The only place this is set in practice is in dbus-server-win.c, which
does not set the error. If it did, dbus_server_listen() would leak it.
Comment 2 Simon McVittie 2018-07-11 17:35:05 UTC
Created attachment 140564 [details] [review]
[2] dbus_server_listen: Assert that implementations return a known result

If they didn't, we'd probably leak the server and/or the error.

---

Ideally this should probably be a switch() over an enum, but that would probably mean reindenting and a lot more diffstat.
Comment 3 Simon McVittie 2018-07-11 17:35:46 UTC
Created attachment 140565 [details] [review]
[3] loopback: Don't free credentials borrowed from the connection

We currently get away with this because the connection isn't fully
freed before we exit, but the connection is meant to own the result
of _dbus_connection_get_credentials() (it's "(transfer none)" in
GLib terminology).

---

An actual bug, albeit in a test.
Comment 4 Simon McVittie 2018-07-11 17:36:43 UTC
Created attachment 140566 [details] [review]
[4] dbus_server_listen: Don't leak first_connect_error

If an implementation fails to listen, and a subsequent implementation
succeeds, then we would have leaked this. Detected by running
tests/loopback.c under valgrind.

---

I don't think this is particularly serious, because listening O(n) times is rare, although it could be significant for Containers.
Comment 5 Simon McVittie 2018-07-11 17:37:37 UTC
Created attachment 140567 [details] [review]
[5] tests: Detach server from main loop during teardown

test_server_setup() takes a reference to the DBusServer, so we need
to release that ref by calling test_server_shutdown().
test_server_shutdown() also disconnects the server, so we don't need
to do that.
Comment 6 Simon McVittie 2018-07-11 17:40:54 UTC
Created attachment 140568 [details] [review]
[6] tests: Interpret empty command-line arguments as --tap

AX_VALGRIND_CHECK overrides LOG_COMPILER, which means we can't rely
on running under glib-tap-test.sh. Default to TAP mode by modifying
our (effective) argv instead.

If you really want the default behaviour (unstructured output) this
can still be achieved by adding some arguments that are a no-op,
such as `-m quick`.

---

I'm not particularly happy about this, but it's the best I can do right now. I don't really want to rely on a newer GLib or a newer AX_VALGRIND_CHECK because that would make it harder to test on older OSs.

TODO: I should probably enhance g_test_init() so it can take an "always use TAP" option after the argv, even if dbus isn't going to make use of that functionality yet.
Comment 7 Simon McVittie 2018-07-11 17:43:15 UTC
Created attachment 140569 [details] [review]
[7] tests: Call dbus_shutdown()

Not all of these tests will be fully valgrind-clean yet (or perhaps
ever), but it's easier to add this to all of them than to think
about it.

---

test/name-test/ excluded because adding dbus_shutdown() in obvious places in some of those made them fail, and I really didn't feel like debugging them right now; test/name-test/ needs to disappear one day, and because it's very reliant on the LOG_COMPILER we set, I'll have to exclude it from AX_VALGRIND_CHECK in the short to medium term in any case.
Comment 8 Philip Withnall 2018-07-11 17:45:16 UTC
Comment on attachment 140563 [details] [review]
[1] Assert that DBUS_SERVER_LISTEN_ADDRESS_ALREADY_USED does not set error

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

r+
Comment 9 Philip Withnall 2018-07-11 17:46:49 UTC
Comment on attachment 140564 [details] [review]
[2] dbus_server_listen: Assert that implementations return a known result

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

r+, though I’d also suggest changing the if…else if…else to a switch, so that -Wswitch-enum can help us here. That can be a follow-up patch, if you think it’s worthwhile.
Comment 10 Philip Withnall 2018-07-11 17:48:58 UTC
Comment on attachment 140565 [details] [review]
[3] loopback: Don't free credentials borrowed from the connection

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

r+, although while reviewing this I noticed that _dbus_transport_get_credentials() returns FALSE instead of NULL in an early return case. I guess that’s another patch to do.
Comment 11 Simon McVittie 2018-07-11 18:33:40 UTC
Created attachment 140570 [details] [review]
WIP: Use AX_VALGRIND_CHECK to set up partial valgrind test coverage

This deliberately doesn't cover name-test/ yet.

---

This is incomplete: I probably need to fix a lot more leaks before it will pass.
Comment 12 Simon McVittie 2018-07-11 18:34:59 UTC
Comment on attachment 140570 [details] [review]
WIP: Use AX_VALGRIND_CHECK to set up partial valgrind test coverage

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

::: configure.ac
@@ +1201,5 @@
>  
> +# Add check-valgrind, etc. targets
> +AX_VALGRIND_CHECK
> +AM_EXTRA_RECURSIVE_TARGETS([check-valgrind])
> +AM_EXTRA_RECURSIVE_TARGETS([check-valgrind-memcheck])

I don't think we're meant to need to do this, because AX_VALGRIND_CHECK is meant to do it already... but just using AX_VALGRIND_CHECK doesn't seem to add the recursive targets. Do you happen to have an example of a project where AX_VALGRIND_CHECK works?
Comment 13 Simon McVittie 2018-07-11 18:36:40 UTC
Created attachment 140571 [details] [review]
WIP: Make loopback test valgrind-clean

---

Probably not all of this is needed, and the bits that are needed should probably be done in all the tests where they're applicable, like I did for calling dbus_shutdown() in Attachment #140569 [details] and for test_server_shutdown() in Attachment #140567 [details].
Comment 14 Philip Withnall 2018-07-12 09:07:47 UTC
Comment on attachment 140566 [details] [review]
[4] dbus_server_listen: Don't leak first_connect_error

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

r+
Comment 15 Philip Withnall 2018-07-12 09:08:45 UTC
Comment on attachment 140567 [details] [review]
[5] tests: Detach server from main loop during teardown

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

r+
Comment 16 Philip Withnall 2018-07-12 09:14:51 UTC
Comment on attachment 140568 [details] [review]
[6] tests: Interpret empty command-line arguments as --tap

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

r+, though it’s somewhat hacky
Comment 17 Philip Withnall 2018-07-12 09:15:46 UTC
Comment on attachment 140569 [details] [review]
[7] tests: Call dbus_shutdown()

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

r+
Comment 18 Philip Withnall 2018-07-12 09:23:53 UTC
Comment on attachment 140570 [details] [review]
WIP: Use AX_VALGRIND_CHECK to set up partial valgrind test coverage

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

r-, see comments

::: configure.ac
@@ +1201,5 @@
>  
> +# Add check-valgrind, etc. targets
> +AX_VALGRIND_CHECK
> +AM_EXTRA_RECURSIVE_TARGETS([check-valgrind])
> +AM_EXTRA_RECURSIVE_TARGETS([check-valgrind-memcheck])

https://gitlab.com/walbottle/walbottle is an example of a project where AX_VALGRIND_CHECK has worked for me in the past. I think AX_VALGRIND_CHECK doesn’t include AM_EXTRA_RECURSIVE_TARGETS calls because I didn’t know they existed when I wrote the macro — you could submit a pull request to add them to AX_VALGRIND_CHECK (https://github.com/autoconf-archive/autoconf-archive) and include a copy of the modified macro in-tree in dbus.git.

In other projects, I’ve always ignored the need for recursive check-valgrind targets, and have just used `make -C path/to/tests check-valgrind` to run it.

Note that if you just want to enable memcheck, you should use AX_VALGRIND_DFLT([sgcheck],[off]) etc. to disable the other Valgrind tools.

::: test/dbus.supp
@@ +10,5 @@
> +   ...
> +   fun:gobject_init
> +}
> +{
> +   <insert_a_suppression_name_here>

Nitpick: You may want to name this suppression.

::: test/name-test/Makefile.am
@@ +90,5 @@
>  endif
> +
> +# Don't use VALGRIND_CHECK_RULES here yet: we're very reliant on
> +# LOG_COMPILER for the tests in this directory, and can't cope with
> +# AX_VALGRIND_CHECK overriding it to be a valgrind tool.

Nitpick: Maybe include ‘FIXME’ in this comment?
Comment 19 Philip Withnall 2018-07-12 09:26:16 UTC
Comment on attachment 140571 [details] [review]
WIP: Make loopback test valgrind-clean

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

r-, though it’s marked as WIP anyway

::: test/loopback.c
@@ +437,5 @@
> +    {
> +      test_connection_shutdown (f->ctx, f->client_conn);
> +      dbus_connection_close (f->client_conn);
> +
> +      while ((message = dbus_connection_pop_message (f->server_conn)))

f->client_conn?
Comment 20 Simon McVittie 2018-07-12 15:57:09 UTC
Created attachment 140598 [details] [review]
[8] Allow longer for tests under valgrind
Comment 21 Simon McVittie 2018-07-12 15:58:13 UTC
Created attachment 140599 [details] [review]
[9] test/dbus-daemon: Don't leak error if no machine ID was  found

---

This is a real corner case, and I only found it because valgrind is currently broken on Debian unstable so I'm doing my testing in a Debian 9 chroot with no init, dbus, /etc/machine-id or /var/lib/dbus/machine-id.
Comment 22 Simon McVittie 2018-07-12 15:58:36 UTC
Created attachment 140600 [details] [review]
[10] test/dbus-daemon: Don't leak expected error for max  connections
Comment 23 Simon McVittie 2018-07-12 15:58:58 UTC
Created attachment 140601 [details] [review]
[11] nonce: Don't try to rmdir(NULL) on OOM

If re-initializing the string fails, it will be left in a state
where it has a length of 0 and a NULL buffer. That's valid to
"free", but not valid to pass to rmdir().
Comment 24 Simon McVittie 2018-07-12 15:59:18 UTC
Created attachment 140602 [details] [review]
[12] test/marshal: Don't leak a message and its marshalled  buffer
Comment 25 Simon McVittie 2018-07-12 16:00:04 UTC
Created attachment 140603 [details] [review]
[13] test/containers: Fix some memory leaks
Comment 26 Simon McVittie 2018-07-12 16:00:35 UTC
Created attachment 140604 [details] [review]
[14] tests: Detach most connections from main loop before  closing

We don't need to do this for connections that were never set up
with the main loop.
Comment 27 Simon McVittie 2018-07-12 16:01:01 UTC
Created attachment 140605 [details] [review]
[15] Skip name-test/ when running under valgrind for now

These tests are very reliant on their custom LOG_COMPILER,
which AX_VALGRIND_CHECK replaces.
Comment 28 Simon McVittie 2018-07-12 16:02:50 UTC
Created attachment 140606 [details] [review]
[16] dispatch test: Simplify OOM testing

Instead of having separate test wrappers for the cases that do and
don't take a DBusConnection, we can just pass a NULL DBusConnection
to the one that doesn't.

---

The naming is now even less justifiable than when these functions were added 15 years ago, but I'm not renaming Check2 to something more sensible right now to keep the diffstat down.
Comment 29 Simon McVittie 2018-07-12 16:05:41 UTC
Created attachment 140607 [details] [review]
[17] Don't do OOM testing under valgrind by default

It's just painfully slow, particularly when we fork (as we do in
test-bus to test service activation).

---

At some point I'll leave a machine running for however many days it takes to see whether the slow tests are valgrind-clean, but today is not that day.
Comment 30 Simon McVittie 2018-07-12 16:10:21 UTC
(In reply to Philip Withnall from comment #18)
> I think AX_VALGRIND_CHECK
> doesn’t include AM_EXTRA_RECURSIVE_TARGETS calls because I didn’t know they
> existed when I wrote the macro

The version in autoconf-archive does include them (someone added them after you contributed the macro); but they still don't seem to *work* unless I put them into configure.ac directly.

> In other projects, I’ve always ignored the need for recursive check-valgrind
> targets, and have just used `make -C path/to/tests check-valgrind` to run it.

FYI, since their conversion into recursive targets, check-valgrind-am has that role instead.

> Note that if you just want to enable memcheck, you should use
> AX_VALGRIND_DFLT([sgcheck],[off]) etc. to disable the other Valgrind tools.

Done, because I don't want to get into importing the bits of glib.supp that pacify drd right now...

> Nitpick: You may want to name this suppression.

Done, and added some more

> > +# Don't use VALGRIND_CHECK_RULES here yet
> 
> Nitpick: Maybe include ‘FIXME’ in this comment?

Superseded by Attachment #140605 [details] which makes these tests be SKIP'd with an explanatory message, which seems an equally good indicator that they should eventually be fixed (by turning them into self-contained tests that fork their own dbus-daemon)
Comment 31 Philip Withnall 2018-07-12 23:05:12 UTC
Comment on attachment 140598 [details] [review]
[8] Allow longer for tests under valgrind

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

r+
Comment 32 Philip Withnall 2018-07-12 23:05:48 UTC
Comment on attachment 140599 [details] [review]
[9] test/dbus-daemon: Don't leak error if no machine ID was  found

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

r+
Comment 33 Philip Withnall 2018-07-12 23:06:21 UTC
Comment on attachment 140600 [details] [review]
[10] test/dbus-daemon: Don't leak expected error for max  connections

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

r+
Comment 34 Philip Withnall 2018-07-12 23:07:16 UTC
Comment on attachment 140601 [details] [review]
[11] nonce: Don't try to rmdir(NULL) on OOM

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

r+
Comment 35 Philip Withnall 2018-07-12 23:08:14 UTC
Comment on attachment 140602 [details] [review]
[12] test/marshal: Don't leak a message and its marshalled  buffer

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

r+
Comment 36 Philip Withnall 2018-07-12 23:09:44 UTC
Comment on attachment 140603 [details] [review]
[13] test/containers: Fix some memory leaks

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

r+
Comment 37 Philip Withnall 2018-07-12 23:11:31 UTC
Comment on attachment 140604 [details] [review]
[14] tests: Detach most connections from main loop before  closing

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

r+
Comment 38 Philip Withnall 2018-07-12 23:12:16 UTC
Comment on attachment 140605 [details] [review]
[15] Skip name-test/ when running under valgrind for now

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

r+
Comment 39 Philip Withnall 2018-07-12 23:15:56 UTC
Comment on attachment 140606 [details] [review]
[16] dispatch test: Simplify OOM testing

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

r+, neat
Comment 40 Philip Withnall 2018-07-12 23:16:41 UTC
Comment on attachment 140607 [details] [review]
[17] Don't do OOM testing under valgrind by default

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

r+, seems reasonable
Comment 41 Simon McVittie 2018-08-02 16:09:49 UTC
(In reply to Simon McVittie from comment #2)
> [2] dbus_server_listen: Assert that implementations return a known result
> 
> Ideally this should probably be a switch() over an enum, but that would
> probably mean reindenting and a lot more diffstat.

TODO, but not blocking on this

(In reply to Philip Withnall from comment #10)
> [3] loopback: Don't free credentials borrowed from the connection
> 
> r+, although while reviewing this I noticed that
> _dbus_transport_get_credentials() returns FALSE instead of NULL in an early
> return case. I guess that’s another patch to do.

TODO, but not blocking on this
Comment 42 Philip Withnall 2018-08-02 16:19:37 UTC
(In reply to Simon McVittie from comment #41)
> (In reply to Simon McVittie from comment #2)
> > [2] dbus_server_listen: Assert that implementations return a known result
> > 
> > Ideally this should probably be a switch() over an enum, but that would
> > probably mean reindenting and a lot more diffstat.
> 
> TODO, but not blocking on this

👍

> (In reply to Philip Withnall from comment #10)
> > [3] loopback: Don't free credentials borrowed from the connection
> > 
> > r+, although while reviewing this I noticed that
> > _dbus_transport_get_credentials() returns FALSE instead of NULL in an early
> > return case. I guess that’s another patch to do.
> 
> TODO, but not blocking on this

👍
Comment 43 Simon McVittie 2018-08-02 22:14:22 UTC
Comment on attachment 140563 [details] [review]
[1] Assert that DBUS_SERVER_LISTEN_ADDRESS_ALREADY_USED does not set error

Applied for 1.13.6
Comment 44 Simon McVittie 2018-08-02 22:14:43 UTC
Comment on attachment 140564 [details] [review]
[2] dbus_server_listen: Assert that implementations return a known result

Applied for 1.13.6
Comment 45 Simon McVittie 2018-08-02 22:15:11 UTC
Comment on attachment 140565 [details] [review]
[3] loopback: Don't free credentials borrowed from the connection

Applied for 1.13.6 (not for other branches because it's test-only)
Comment 46 Simon McVittie 2018-08-02 22:15:28 UTC
Comment on attachment 140566 [details] [review]
[4] dbus_server_listen: Don't leak first_connect_error

Applied for 1.13.6 and 1.12.10
Comment 47 Simon McVittie 2018-08-02 22:15:42 UTC
Comment on attachment 140567 [details] [review]
[5] tests: Detach server from main loop during teardown

Applied for 1.13.6
Comment 48 Simon McVittie 2018-08-02 22:15:53 UTC
Comment on attachment 140568 [details] [review]
[6] tests: Interpret empty command-line arguments as --tap

Applied for 1.13.6
Comment 49 Simon McVittie 2018-08-02 22:16:07 UTC
Comment on attachment 140569 [details] [review]
[7] tests: Call dbus_shutdown()

Applied for 1.13.6
Comment 50 Simon McVittie 2018-08-02 22:16:20 UTC
Comment on attachment 140598 [details] [review]
[8] Allow longer for tests under valgrind

Applied for 1.13.6
Comment 51 Simon McVittie 2018-08-02 22:16:29 UTC
Comment on attachment 140599 [details] [review]
[9] test/dbus-daemon: Don't leak error if no machine ID was  found

Applied for 1.13.6
Comment 52 Simon McVittie 2018-08-02 22:16:39 UTC
Comment on attachment 140600 [details] [review]
[10] test/dbus-daemon: Don't leak expected error for max  connections

Applied for 1.13.6
Comment 53 Simon McVittie 2018-08-02 22:16:51 UTC
Comment on attachment 140601 [details] [review]
[11] nonce: Don't try to rmdir(NULL) on OOM

Applied for 1.13.6 and 1.12.10
Comment 54 Simon McVittie 2018-08-02 22:17:01 UTC
Comment on attachment 140602 [details] [review]
[12] test/marshal: Don't leak a message and its marshalled  buffer

Applied for 1.13.6
Comment 55 Simon McVittie 2018-08-02 22:17:16 UTC
Comment on attachment 140603 [details] [review]
[13] test/containers: Fix some memory leaks

Applied for 1.13.6
Comment 56 Simon McVittie 2018-08-02 22:17:27 UTC
Comment on attachment 140604 [details] [review]
[14] tests: Detach most connections from main loop before  closing

Applied for 1.13.6
Comment 57 Simon McVittie 2018-08-02 22:17:42 UTC
Comment on attachment 140605 [details] [review]
[15] Skip name-test/ when running under valgrind for now

Applied for 1.13.6
Comment 58 Simon McVittie 2018-08-02 22:17:55 UTC
Comment on attachment 140606 [details] [review]
[16] dispatch test: Simplify OOM testing

Applied for 1.13.6
Comment 59 Simon McVittie 2018-08-02 22:18:11 UTC
Comment on attachment 140607 [details] [review]
[17] Don't do OOM testing under valgrind by default

Applied for 1.13.6
Comment 60 Simon McVittie 2018-08-02 22:19:56 UTC
Created attachment 140942 [details] [review]
[18] embedded tests: Make it easier to run a single test-case

When running tests under "make check" or similar to take advantage
of facilities like AM_TESTS_ENVIRONMENT and AX_VALGRIND_CHECK, it's
more straightforward to set an environment variable than to pass a
command-line option.
Comment 61 Simon McVittie 2018-08-02 22:20:53 UTC
Created attachment 140943 [details] [review]
WIP: Use AX_VALGRIND_CHECK to set up partial valgrind test  coverage

---

Now with documentation.
Comment 62 Simon McVittie 2018-08-02 22:22:12 UTC
TODO: It would be nice if there was a way to make spawn_dbus_daemon() run the dbus-daemon under valgrind too, but that probably involves fighting with and/or contributing incompatible changes to AX_VALGRIND_CHECK.
Comment 63 Philip Withnall 2018-08-03 10:18:26 UTC
Comment on attachment 140942 [details] [review]
[18] embedded tests: Make it easier to run a single test-case

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

True, dat.

r+
Comment 64 Philip Withnall 2018-08-03 10:20:30 UTC
Comment on attachment 140943 [details] [review]
WIP: Use AX_VALGRIND_CHECK to set up partial valgrind test  coverage

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

::: test/Makefile.am
@@ +659,5 @@
> +# Add rules for valgrind testing, as defined by AX_VALGRIND_CHECK
> +
> +# Potentially useful additions:
> +#VALGRIND_FLAGS = --num-callers=30 --gen-suppressions=all --read-var-info=yes
> +#VALGRIND_memcheck_FLAGS = --show-leak-kinds=all --errors-for-leak-kinds=definite,possible --leak-check=full

OOI, why have these commented out? Are they only meant to be informative?
Comment 65 Simon McVittie 2018-08-03 15:56:46 UTC
(In reply to Philip Withnall from comment #64)
> OOI, why have these commented out? Are they only meant to be informative?

Yes, my intention was that they're an opt-in for "show me more leaks, slower" rather than something that developers use all the time.
Comment 66 GitLab Migration User 2018-10-12 21:35:47 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/dbus/dbus/issues/218.


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.