[systemd-devel] Fwd: [PATCH v3] tests: added tests for unit_file_get_{state, list}

[Martin Pitt]

Hello Ken, David,

sorry for breaking the thread.

Thanks for adding these tests! This looks good to me, I just have some
nitpicks. NB that I'm not really familiar with the systemd code, so my
comments are certainly not complete.

David Timothy Strauss [2014-10-17 13:42 +0200]:
> diff --git a/Makefile.am b/Makefile.am
> index e52db17..7d4f2f5 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -1358,7 +1358,8 @@ tests += \
>         test-ratelimit \
>         test-condition-util \
>         test-uid-range \
> -       test-bus-policy
> +       test-bus-policy \
> +       test-enabled

Nitpick: Can we end lists with a $(NULL) line so that patches don't
always have to change the previous last line?

>  EXTRA_DIST += \
>         test/a.service \
> @@ -1394,8 +1395,36 @@ EXTRA_DIST += \
>         test/bus-policy/hello.conf \
>         test/bus-policy/methods.conf \
>         test/bus-policy/ownerships.conf \
> -       test/bus-policy/signals.conf
> -
> +       test/bus-policy/signals.conf \
> +       test/test-enabled-root/usr/lib/systemd/system/masked.service \

[...] Personally I find it a lot easier to have a test with this
style:

  unit = create_test_unit(contents);
  assert(something_about(unit));

as you keep the data what you test and what you expect together in one
spot. Also it keeps clutter out of the tree and it's easier to
factorize/mass-change test units.

But this is again just nitpicking, and actually also a question to the
systemd developers which style they prefer.

> +static void test_enabled(int argc, char* argv[]) {
> [...]
> +        h = hashmap_new(&string_hash_ops);
> +        r = unit_file_get_list(UNIT_FILE_SYSTEM, root_dir, h);
> +        assert_se(r >= 0);

I'm a bit confused here -- is this supposed to have a side effect and
the condition was rewritten later?

> +        HASHMAP_FOREACH(p, h, i) {
> +                UnitFileState s;
> +
> +                s = unit_file_get_state(UNIT_FILE_SYSTEM, root_dir,
> +                                        basename(p->path));
> +
> +                /* unit_file_get_list and unit_file_get_state are
> +                 * a little different in some cases.  Handle these
> +                 * cases here ...
> +                 */
> +                switch ((int)s) {
> +                case UNIT_FILE_ENABLED_RUNTIME:

Meh type safety (enum → int → back to enum), but this looks
safe/correct anyway. Handling both errno and enum value in the same
switch isn't otherwise possible, it'd need to be moved into an if/else
ladder.

Martin
-- 
Martin Pitt                        | http://www.piware.de
Ubuntu Developer (www.ubuntu.com)  | Debian Developer  (www.debian.org)