[pulseaudio-discuss] [PATCH] tests: Add tests for alsa-mixer paths

[David Henningsson]

On 03/22/2013 07:00 PM, Tanu Kaskinen wrote:
> On Fri, 2013-03-22 at 15:37 +0100, David Henningsson wrote:
>> It checks all files in the mixer/paths directory and checks
>>   - that the file can be parsed without errors
>>   - that the file is actually shipped in the makefile
>>
>> Signed-off-by: David Henningsson <david.henningsson at canonical.com>
>> ---
>>
>> After I realized that my previous headset-mic patch did not event ship the new file,
>> I also realized I might do the same mistake again.
>>
>> I guess this could be enhanced later with checking that profile-sets do not point
>> to non-existent paths etc, but this will do for now.
>>
>>   src/Makefile.am                  |    7 +++
>>   src/tests/alsa-mixer-path-test.c |  110 ++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 117 insertions(+)
>>   create mode 100644 src/tests/alsa-mixer-path-test.c
>>
>> diff --git a/src/Makefile.am b/src/Makefile.am
>> index 7685e0c..3326a2c 100644
>> --- a/src/Makefile.am
>> +++ b/src/Makefile.am
>> @@ -297,6 +297,8 @@ endif
>>   if HAVE_ALSA
>>   TESTS_norun += \
>>   		alsa-time-test
>> +TESTS_default += \
>> +		alsa-mixer-path-test
>>   endif
>>
>>   if HAVE_TESTS
>> @@ -545,6 +547,11 @@ alsa_time_test_LDADD = $(AM_LDADD) $(ASOUNDLIB_LIBS)
>>   alsa_time_test_CFLAGS = $(AM_CFLAGS) $(ASOUNDLIB_CFLAGS)
>>   alsa_time_test_LDFLAGS = $(AM_LDFLAGS) $(BINLDFLAGS)
>>
>> +alsa_mixer_path_test_SOURCES = tests/alsa-mixer-path-test.c
>> +alsa_mixer_path_test_CFLAGS = $(AM_CFLAGS) $(LIBCHECK_CFLAGS) $(ASOUNDLIB_CFLAGS)
>> +alsa_mixer_path_test_LDADD = $(AM_LDADD) libpulsecore- at PA_MAJORMINOR@.la libpulse.la libpulsecommon- at PA_MAJORMINOR@.la libalsa-util.la
>> +alsa_mixer_path_test_LDFLAGS = $(AM_LDFLAGS) $(BINLDFLAGS) $(LIBCHECK_LIBS)
>> +
>>   usergroup_test_SOURCES = tests/usergroup-test.c
>>   usergroup_test_LDADD = $(AM_LDADD) libpulsecore- at PA_MAJORMINOR@.la libpulse.la libpulsecommon- at PA_MAJORMINOR@.la
>>   usergroup_test_CFLAGS = $(AM_CFLAGS) $(LIBCHECK_CFLAGS)
>> diff --git a/src/tests/alsa-mixer-path-test.c b/src/tests/alsa-mixer-path-test.c
>> new file mode 100644
>> index 0000000..2460c66
>> --- /dev/null
>> +++ b/src/tests/alsa-mixer-path-test.c
>> @@ -0,0 +1,110 @@
>> +#ifdef HAVE_CONFIG_H
>> +#include <config.h>
>> +#endif
>> +
>> +#include <check.h>
>> +#include <dirent.h>
>> +#include <stdbool.h>
>> +#include <stdio.h>
>> +
>> +#include <pulse/pulseaudio.h>
>> +#include <pulsecore/log.h>
>> +#include <pulsecore/core-util.h>
>> +#include <pulsecore/strlist.h>
>> +#include <modules/alsa/alsa-mixer.h>
>> +
>> +
>> +static const char *get_default_paths_dir(void) {
>> +    if (pa_run_from_build_tree())
>> +        return PA_BUILDDIR "/modules/alsa/mixer/paths/";
>
> This doesn't work if the build directory is different from the source
> directory. Instead of PA_BUILDDIR, this should use PA_SRCDIR, which
> doesn't currently exist, but it should be possible to add it.

It is copy-pasted from existing pulseaudio code (in alsa-mixer.c, IIRC). 
If you fix it there, feel free to do so here to.

But I do an out-of-source build normally (according to Colin's old but 
excellent instructions), and my test worked siccessfully. I do remember 
though that Colin's instructions include making some related symlink.

>
>> +    else
>> +        return PA_ALSA_PATHS_DIR;
>> +}
>> +
>> +static pa_strlist *load_makefile() {
>> +    FILE *f;
>> +    bool lookforfiles = false;
>> +    char buf[2048];
>> +    pa_strlist *result = NULL;
>> +    const char *Makefile = PA_BUILDDIR "/Makefile";
>> +
>> +    f = pa_fopen_cloexec(Makefile, "r");
>> +    fail_unless(f != NULL); /* Consider skipping this test instead of failing if Makefile not found? */
>
> If you want my opinion to the question, I think it's fine to assume that
> Makefile is always found.

Ok.

>
>> +    while (!feof(f)) {
>> +        if (!fgets(buf, sizeof(buf), f)) {
>> +            fail_unless(feof(f));
>
> The loop condition is !feof(f), so this will always fail. The if could
> be replaced with fail_unless(fgets(buf, sizeof(buf), f));

Eh, I don't think so?

If you're sure, could you please correct the code in pa_config_parse 
which works the same way?

>> +            break;
>> +        }
>> +        if (strstr(buf, "dist_alsapaths_DATA = \\") != NULL) {
>> +           lookforfiles = true;
>> +           continue;
>> +        }
>> +        if (!lookforfiles)
>> +           continue;
>> +        if (!strstr(buf, "\\"))
>> +           lookforfiles = false;
>> +        else
>> +           strstr(buf, "\\")[0] = '\0';
>> +        pa_strip(buf);
>> +        pa_log_debug("Shipping file '%s'", pa_path_get_filename(buf));
>> +        result = pa_strlist_prepend(result, pa_path_get_filename(buf));
>> +    }
>> +    fclose(f);
>> +    return result;
>> +}
>> +
>> +
>> +START_TEST (mixer_path_test) {
>> +    DIR *dir;
>> +    struct dirent *ent;
>> +    pa_strlist *ship = load_makefile();
>
> This strlist is never freed.

Ok.

>
>> +    const char *pathsdir = get_default_paths_dir();
>> +    pa_log_debug("Analyzing directory: '%s'", pathsdir);
>> +
>> +    dir = opendir(pathsdir);
>> +    fail_unless(dir != NULL);
>> +    while ((ent = readdir(dir)) != NULL) {
>> +        pa_alsa_path *path;
>> +        if (strcmp(ent->d_name, ".") == 0 || strcmp(ent->d_name, "..") == 0)
>
> pa_streq() could be used here.

Ok.

>
>> +            continue;
>> +        pa_log_debug("Analyzing file: '%s'", ent->d_name);
>> +
>> +        /* Can the file be parsed? */
>> +        path = pa_alsa_path_new(pathsdir, ent->d_name, PA_ALSA_DIRECTION_ANY);
>> +        fail_unless(path != NULL);
>> +
>> +        /* Is the file shipped? */
>> +        if (ship) {
>> +            pa_strlist *n;
>> +            bool found = false;
>> +            for (n = ship; n; n = pa_strlist_next(n))
>> +                found |= pa_streq(ent->d_name, pa_strlist_data(n));
>> +            fail_unless(found);
>> +        }
>> +    }
>> +    closedir(dir);
>> +}
>> +END_TEST
>> +
>> +int main(int argc, char *argv[]) {
>> +    int failed = 0;
>> +    Suite *s;
>> +    TCase *tc;
>> +    SRunner *sr;
>> +
>> +    if (!getenv("MAKE_CHECK"))
>> +        pa_log_set_level(PA_LOG_DEBUG);
>> +
>> +    s = suite_create("Alsa-mixer-path");
>> +    tc = tcase_create("alsa-mixer-path");
>> +    tcase_add_test(tc, mixer_path_test);
>> +    tcase_set_timeout(tc, 5 * 60);
>
> 5 minutes seems a bit excessive for this test...

Ok, 30 seconds will do.



-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic