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
