test: make unit tests relocatable and add an "install-tests" make target by martinpitt · Pull Request #5298 · systemd/systemd

martinpitt · 2017-02-09T21:59:35Z

It is useful to package test-* binaries and run them as root under
autopkgtest or manually on particular machines. They currently have a
built-in hardcoded absolute path to their test data, which does not work
when running the test programs from any other path than the original
build directory.

Add a new "install-tests" make target that installs our unit test-* executables and their test data files into /usr/lib/systemd/tests/.

Today I added a new autopkgtest to CI which runs all (automatic) unit tests as root. This has uncovered a few issues like the arch specific seccomp failures, so let's start to gate on all of them. However, 7 tests fail due to their hardcoded TEST_DIR and thus are blacklisted in the test. With this PR we can enable them.

martinpitt · 2017-02-09T22:01:07Z

Note that even with this, test-execute still fails, but much later on (and due to an unrelated reason). But at least the blacklist will then be down to that and test-catalog, both of which are TBI.

keszybz · 2017-02-10T02:39:36Z

I don't quite understand how you plan this to work for installed tests. (I'm assuming that this eventually is supposed to lead to #5257.) It would be nice to have the installed tests work without an environment variable. So the tests should compile in the installation path, and when running from the build directory, the env var could be used, i.e. the opposite of what this patch does, afaiu.

martinpitt · 2017-02-10T07:00:53Z

@keszybz: Right now this allows me to run TOP_SRCDIR=$(pwd) ./test-foo or TOP_SRCDIR=$(pwd) /some/where/else/test-foo in a checked out git tree, as I don't currently copy the test files. However, you are right that this is a bit lopsided towards autopkgtest where we always have the source tree available.

A more generic solution would be to also install the test data files. Then we don't need an env var at all, but tests should simply look in a dir relative to their own path (dirname of $0) instead of a builtin absolute path.

mbiebl · 2017-02-10T07:27:32Z

I wonder whether it would make sense to support
https://wiki.gnome.org/Initiatives/GnomeGoals/InstalledTests

I see a couple of packages in Debian already following that scheme:

clutter-1.0-tests
dbus-tests
flatpak-tests
gjs-tests
glib-networking-tests
libappstream-glib-dev
libglib2.0-tests
ostree-tests
pango1.0-tests
python3-dbus-tests
python-dbus-tests

For example https://github.com/flatpak/flatpak/tree/master/tests

keszybz · 2017-02-10T15:29:56Z

A more generic solution would be to also install the test data files. Then we don't need an env var at all, but tests should simply look in a dir relative to their own path (dirname of $0) instead of a builtin absolute path.

~~Yeah, I think we should aim for that.~~
(I misspoke here. We seem to need an variable when running from the build root.)

I wonder whether it would make sense to support
https://wiki.gnome.org/Initiatives/GnomeGoals/InstalledTests

This seems to be very specific to graphical applications. Some parts don't apply to us. But we might follow at least the fs-layout that they suggest, and add more compatibility later on.

There's also a wider question of converting our output to TAP. I think it would make a lot of sense, allowing us to get better reporting from our tests, but it's a great undertaking.

martinpitt · 2017-02-12T20:56:03Z

tests should simply look in a dir relative to their own path (dirname of $0)

I played around with that, and that's a bit ugly. E. g. builddir/.libs/test-dns-packet expects its test data files in src/resolve/test-data, so there is no single relative path which always works, and it's quite dramatically different to where it would be in the installed target. So maybe going the other way around and simply defaulting to searching for the test data in the directory of the executable, and override this in the build tree with env variables is better.

keszybz · 2017-02-12T21:11:28Z

Just to clarify, this matches what I said in #5298 (comment)?

martinpitt · 2017-02-12T21:33:37Z

Mostly, just that I don't want to compile in an absolute installation path either. I still think relative path names are nicer, but they need to be overridden by env vars for running tests in the build tree.

keszybz · 2017-02-12T21:36:03Z

Agreed, relative paths are better.

martinpitt · 2017-02-12T22:58:15Z

Reworked as above, and I also added a install-tests make target. I can now run

make install-tests DESTDIR=/tmp/x
/tmp/x/usr/lib/systemd/tests/test-dns-packet

and things Just Work™.

keszybz · 2017-02-13T13:10:01Z

src/shared/tests.c

 }
+
+const char* get_exe_relative_testdata_dir(void)
+{


Brace wrong place.

keszybz · 2017-02-13T13:12:05Z

Makefile.am

+	    if [ -x .libs/$$f ]; then \
+		install -m 755 .libs/$$f $(DESTDIR)/$(testsdir); \
+	    else \
+		install -m 755 $$f $(DESTDIR)/$(testsdir); \


If -D is used below, it should be used here too.

keszybz · 2017-02-13T13:24:45Z

Makefile.am

 	test-libudev-sym

+.PHONY: install-tests
+install-tests: $(tests) $(TEST_DATA_FILES)


Hm, that's problematic.

We have --enable-tests=unsafe. I think it's reasonable to enable this during package build (since it usually happens in a throw-away environment), but then those tests are installed intermingled with the other ones. Also, the result of install-tests depends on this configure option, but I don't think it should.

Maybe the extra tests should be installed into $(testsdir)/{unsafe,manual}?

I think this PR is safe wrt. that. Regardless of whether you enable or disable unsafe tests, they don't get installed. Check this in Makefile.am:

noinst_PROGRAMS = $(manual_tests) $(tests) $(unsafe_tests) TESTS = $(tests) if ENABLE_UNSAFE_TESTS TESTS += \ ……………………$(unsafe_tests) endif

but the install-tests target only works over $(tests), not $(TESTS).

That said, I'm fine with installing those into $(testsdir)/{unsafe,manual} in addition (or that could be a separate PR).

Oh, right. I think we should install unsafe tests, because they're precisely the kind you want to run in a throw-away build environment, but you are right that this can be a separate PR.

Moe test-resolve's test data from src/resolve/test-data to test/test-resolve/ to be consistent with test/test-{execute,path}/. This will make it easier to make the tests relocatable.

It is useful to package test-* binaries and run them as root under autopkgtest or manually on particular machines. They currently have a built-in hardcoded absolute path to their test data, which does not work when running the test programs from any other path than the original build directory. By default, make the tests look for their data in <test_exe_directory>/testdata/ so that they can be called from any directory (provided that the corresponding test data is installed correctly). As we don't have a fixed static path in the build tree (as build and source tree are independent), set $TEST_DIR with "make check" to point to <srcdir>/test/, as we previously did with an automake variable.

Add a new "install-tests" make target that installs our unit test-* executables and their test data files into /usr/lib/systemd/tests/. This is useful for packaging the tests to run them with root privileges or in CI. Fixes systemd#5257

evverx · 2017-02-14T03:21:02Z

src/shared/tests.c

+        static char testdir[PATH_MAX];
+
+        assert_se(readlink_and_make_absolute("/proc/self/exe", &exedir) >= 0);
+        assert_se(snprintf(testdir, sizeof(testdir), "%s/testdata", dirname(exedir)) > 0);


Not sure I understand how does it work

./libtool --mode=execute ./test-engine ... Assertion 'r == 0' failed at src/test/test-engine.c:62, function main(). Aborting. Aborted (core dumped)

strace shows:

stat("/home/vagrant/systemd/.libs/testdata", 0x7ffd852cece0) = -1 ENOENT (No such file or directory)

i.e. make valgrind-tests is broken

mbiebl · 2017-02-14T04:35:22Z

"make install-tests" fails if srcdir != builddir as the test data files are not found.

install: cannot stat 'test/a.service': No such file or directory
install: cannot stat 'test/basic.target': No such file or directory
install: cannot stat 'test/b.service': No such file or directory
...
install: cannot stat 'test/test-resolve/fake-caa.pkts': No such file or directory
Makefile:26708: recipe for target 'install-tests' failed
make: *** [install-tests] Error 1

martinpitt added the tests label Feb 9, 2017

martinpitt added the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Feb 11, 2017

martinpitt self-assigned this Feb 11, 2017

martinpitt force-pushed the relocatable-tests branch from 381f286 to 925d268 Compare February 12, 2017 22:55

martinpitt changed the title ~~test: make unit tests relocatable~~ test: make unit tests relocatable and add an "install-tests" make target Feb 12, 2017

martinpitt removed the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Feb 12, 2017

martinpitt added the build-system label Feb 12, 2017

keszybz requested changes Feb 13, 2017

View reviewed changes

martinpitt added 3 commits February 13, 2017 22:23

test: move resolved test data into test/

ee3c7ff

Moe test-resolve's test data from src/resolve/test-data to test/test-resolve/ to be consistent with test/test-{execute,path}/. This will make it easier to make the tests relocatable.

buildsys: add "install-tests" target

4f8425b

Add a new "install-tests" make target that installs our unit test-* executables and their test data files into /usr/lib/systemd/tests/. This is useful for packaging the tests to run them with root privileges or in CI. Fixes systemd#5257

martinpitt force-pushed the relocatable-tests branch from 925d268 to 4f8425b Compare February 13, 2017 21:40

martinpitt requested a review from keszybz February 13, 2017 21:46

keszybz approved these changes Feb 14, 2017

View reviewed changes

keszybz merged commit f34182f into systemd:master Feb 14, 2017

evverx reviewed Feb 14, 2017

View reviewed changes

keszybz mentioned this pull request Feb 14, 2017

Revert "test: make unit tests relocatable and add an "install-tests" make target" #5339

Closed

martinpitt deleted the relocatable-tests branch February 14, 2017 06:55

evverx mentioned this pull request Feb 16, 2017

fallback hostname fixes #5366

Merged

Uh oh!

Conversation

martinpitt commented Feb 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

martinpitt commented Feb 9, 2017

Uh oh!

keszybz commented Feb 10, 2017

Uh oh!

martinpitt commented Feb 10, 2017 • edited by keszybz Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mbiebl commented Feb 10, 2017

Uh oh!

keszybz commented Feb 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

martinpitt commented Feb 12, 2017

Uh oh!

keszybz commented Feb 12, 2017

Uh oh!

martinpitt commented Feb 12, 2017

Uh oh!

keszybz commented Feb 12, 2017

Uh oh!

martinpitt commented Feb 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbiebl commented Feb 14, 2017

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

martinpitt commented Feb 9, 2017 •

edited

Loading

martinpitt commented Feb 10, 2017 •

edited by keszybz

Loading

keszybz commented Feb 10, 2017 •

edited

Loading