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

[Tanu Kaskinen]

On Fri, 2013-03-22 at 19:43 +0100, David Henningsson wrote:
> 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.

OK.

> 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.

I tried this:

mkdir build
cd build
../configure
make check

This test failed.

> >
> >> +    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?

fgets() returns NULL in case there's an error or f is at EOF. The while
condition just checked that f is not at EOF, therefore an error must
have happened. 

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

Done.

-- 
Tanu