test: allow installing built test executables #351
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dvdhrm
requested changes
Mar 22, 2024
| @@ -1,6 +1,7 @@ | |||
| option('apparmor', type: 'boolean', value: false, description: 'AppArmor support') | |||
| option('audit', type: 'boolean', value: false, description: 'Audit support') | |||
| option('docs', type: 'boolean', value: false, description: 'Build documentation') | |||
| option('install-tests', type: 'boolean', value: false, description: 'Install test executables') | |||
There was a problem hiding this comment.
How about just tests similar to docs?
There was a problem hiding this comment.
The issue here is that the docs option controls whether we build the docs at all, but introducing tests in its current form would control only the installation of the tests since we build most of the tests by default - i.e. -Dtests=false would still build tests. Maybe the current behavior should change, so the tests aren't build by default, and let it be controlled by -Dtests=?
| test('FD Stream Constraints', test_fdstream, kwargs: suite) | ||
| test('Client Lifetime', test_lifetime, kwargs: suite) | ||
| test('Signals and Matches', test_matches, kwargs: suite) | ||
| endforeach |
test/dbus/test-driver.c
Outdated
| @@ -1162,7 +1162,7 @@ static void test_remove_match(void) { | |||
| r = sd_bus_call_method(bus, "org.freedesktop.DBus", "/org/freedesktop/DBus", "org.freedesktop.DBus", | |||
| "RemoveMatch", &error, NULL, | |||
| "s", "sender=org.freedesktop.DBus"); | |||
| if (!getenv("DBUS_BROKER_TEST_DAEMON")) { | |||
| if (!getenv("TEST_DBUS_DAEMON")) { | |||
There was a problem hiding this comment.
Hmmm. In general we prefer DBUS_BROKER_* prefixes for any global namespace. Any reason to change this?
There was a problem hiding this comment.
I just found the original name slightly confusing (especially when introducing its dbus-broker counterpart), but I don't really have a strong opinion on this one.
test/dbus/util-broker.c
Outdated
| @@ -318,7 +321,8 @@ void util_fork_daemon(sd_event *event, int pipe_fd, pid_t *pidp) { | |||
| c_assert(r >= 0); | |||
|
|
|||
| /* exec dbus-daemon */ | |||
| bin = getenv("DBUS_BROKER_TEST_DAEMON") ?: "/usr/bin/dbus-daemon"; | |||
| bin = getenv("TEST_DBUS_DAEMON") ?: "/usr/bin/dbus-daemon"; | |||
There was a problem hiding this comment.
If we drop TEST_DBUS_BROKER, I'd be fine with making this a boolean env-var and using BINDIR "/dbus-daemon".
Let's make the reference and benchmark tests work standalone, i.e. without a build dir, since they're useful on their own. This introduces a matching counterpart for the $DBUS_BROKER_TEST_DAEMON env variable - $DBUS_BROKER_TEST_BROKER - that can be used to override the path to the dbus-broker binary.
This introduces a new meson option 'install-tests' (disabled by default)
that allows installing built test executables along with regular stuff,
which can be used to ensure the installed dbus-broker works as expected.
The tests are installed into <prefix>/lib/dbus-broker/tests directory,
and split into 'unit' and 'dbus' subdirectories, similarly as they're in
the source tree:
$ meson setup --prefix=/usr -Dselinux=true -Dreference-test=true -Dinstall-tests=true build
...
$ DESTDIR=$PWD/install-test ninja -C build install
...
$ tree install-test/
install-test/
└── usr
├── bin
│ ├── dbus-broker
│ └── dbus-broker-launch
└── lib
├── dbus-broker
│ └── tests
│ ├── dbus
│ │ ├── bench-connect
│ │ ├── bench-message
│ │ ├── test-broker
│ │ ├── test-driver
│ │ ├── test-fdstream
│ │ ├── test-lifetime
│ │ └── test-matches
│ └── unit
│ ├── test-address
│ ├── test-apparmor
│ ├── test-config
...
│ ├── test-systemd
│ └── test-user
└── systemd
├── catalog
│ ├── dbus-broker.catalog
│ └── dbus-broker-launch.catalog
├── system
│ └── dbus-broker.service
└── user
└── dbus-broker.service
12 directories, 36 files
$ install-test/usr/lib/dbus-broker/tests/dbus/test-broker
Using dbus-broker binary /usr/bin/dbus-broker
Using dbus-broker binary /usr/bin/dbus-broker
Using dbus-broker binary /usr/bin/dbus-broker
Using dbus-broker binary /usr/bin/dbus-broker
mrc0mmand
force-pushed
the
more-test-shenanigans
branch
from
9b6e6ff to
1ffa13c
Compare
dvdhrm
approved these changes
Apr 11, 2024
|
Thanks a lot! I have minor style nits, but I will fix them post merge. |
mrc0mmand
added a commit
to mrc0mmand/dbus-broker
that referenced
this pull request
Apr 16, 2024
Let's finish the reference tests saga and make use of installed reference tests ([0]) provided by a recently introduced dbus-broker-tests package ([1]). This commit introduces a simple test that runs the reference tests (and benchmarks) against both dbus-daemon and dbus-broker. [0] bus1#351 [1] https://src.fedoraproject.org/rpms/dbus-broker/pull-request/14
mrc0mmand
added a commit
to mrc0mmand/dbus-broker
that referenced
this pull request
Apr 16, 2024
Let's finish the reference tests saga and make use of installed reference tests ([0]) provided by a recently introduced dbus-broker-tests package ([1]). This commit introduces a simple test that runs the reference tests (and benchmarks) against both dbus-daemon and dbus-broker. [0] bus1#351 [1] https://src.fedoraproject.org/rpms/dbus-broker/pull-request/14
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR introduces two changes: making the reference tests work without a build dir and, subsequently, allowing both reference and unit tests to be installed along with dbus-broker itself, so they can be later used to verify if the installed dbus-broker behaves as expected.
The first change unifies an already existing behavior, where the path to a dbus-daemon binary can be tweaked via an environment variable - the name of the variable was changed to
TEST_DBUS_DAEMON, and a new env variable -TEST_DBUS_BROKER- was introduced that does the same thing, but for the dbus-broker binary (instead of hardcoding the path directly). Both paths now default to/usr/bin/{dbus-daemon,dbus-broker}; when running the tests using meson the path for dbus-broker is overridden to point to the just built binary. This should make the tests behave correctly both when run standalone and during development.The second change makes both unit and reference tests installable (if requested) under
<prefix>/share/dbus-broker. The idea here is to follow-up this change with a change in the dbus-broker spec file to split the tests into a separate RPM (like dbus-broker-tests), which can be then easily consumed by the CI infra introduced in #348. The<prefix>/share/dbus-brokerpath is more like a proposal than a set thing, as I have no preference here. I went through several test packages in Fedora (likesystemd-tests,dbus-tests,firewalld-test, etc.), and the<prefix>/share/...path seems to be the most used one.Also a note: the changes make a heavy use of the meson's "kwargs" feature, which reduces code duplication quite a lot. This was introduced in meson 0.49.0, but since we already depend on at least 0.60.0, we should be, hopefully, fine.