[Spice-devel] [PATCH 4/4] stat: add test for statistic functions
Jonathon Jongsma
jjongsma at redhat.com
Thu Dec 3 09:40:10 PST 2015
On Thu, 2015-12-03 at 12:05 -0500, Frediano Ziglio wrote:
> >
> > On Wed, 2015-12-02 at 11:43 +0000, Frediano Ziglio wrote:
> > > Make sure code compile with and without statistics enabled (beside
> > > printing functions).
> > >
> > > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > > ---
> > > server/tests/.gitignore | 5 +++
> > > server/tests/Makefile.am | 35 ++++++++++++++++++++
> > > server/tests/stat-main.c | 49 ++++++++++++++++++++++++++++
> > > server/tests/stat-test.c | 83
> > > ++++++++++++++++++++++++++++++++++++++++++++++++
> > > 4 files changed, 172 insertions(+)
> > > create mode 100644 server/tests/stat-main.c
> > > create mode 100644 server/tests/stat-test.c
> > >
> > > diff --git a/server/tests/.gitignore b/server/tests/.gitignore
> > > index 1059247..35b9624 100644
> > > --- a/server/tests/.gitignore
> > > +++ b/server/tests/.gitignore
> > > @@ -9,3 +9,8 @@ spice-server-replay
> > > test_display_width_stride
> > > test_two_servers
> > > test_vdagent
> > > +stat_test
> > > +libtest1.a
> > > +libtest2.a
> > > +libtest3.a
> > > +libtest4.a
> >
> > this should be a little less generically-named. at least libstat_test1 or
> > something
> >
> >
> > > diff --git a/server/tests/Makefile.am b/server/tests/Makefile.am
> > > index 96eb39c..f015281 100644
> > > --- a/server/tests/Makefile.am
> > > +++ b/server/tests/Makefile.am
> > > @@ -44,6 +44,20 @@ noinst_PROGRAMS = \
> > > test_vdagent \
> > > test_display_width_stride \
> > > spice-server-replay \
> > > + stat_test \
> > > + $(NULL)
> > > +
> > > +TESTS = \
> > > + stat_test \
> > > + $(NULL)
> > > +
> > > +check_PROGRAMS = $(TESTS)
> > > +
> > > +noinst_LIBRARIES = \
> > > + libtest1.a \
> > > + libtest2.a \
> > > + libtest3.a \
> > > + libtest4.a \
> > > $(NULL)
> > >
> > > test_vdagent_SOURCES = \
> > > @@ -113,3 +127,24 @@ spice_server_replay_SOURCES =
> > > \
> > > basic_event_loop.h \
> > > test_util.h \
> > > $(NULL)
> > > +
> > > +stat_test_SOURCES = stat-main.c
> > > +stat_test_LDADD = \
> > > + $(LDADD) \
> > > + libtest1.a \
> > > + libtest2.a \
> > > + libtest3.a \
> > > + libtest4.a \
> > > + $(NULL)
> > > +
> > > +libtest1_a_SOURCES = stat-test.c
> > > +libtest1_a_CPPFLAGS = $(AM_CPPFLAGS) -DCOMP=0 -DWORKER=0
> > > -DTEST_NAME=test1
> > > +
> > > +libtest2_a_SOURCES = stat-test.c
> > > +libtest2_a_CPPFLAGS = $(AM_CPPFLAGS) -DCOMP=0 -DWORKER=1
> > > -DTEST_NAME=test2
> > > +
> > > +libtest3_a_SOURCES = stat-test.c
> > > +libtest3_a_CPPFLAGS = $(AM_CPPFLAGS) -DCOMP=1 -DWORKER=0
> > > -DTEST_NAME=test3
> > > +
> > > +libtest4_a_SOURCES = stat-test.c
> > > +libtest4_a_CPPFLAGS = $(AM_CPPFLAGS) -DCOMP=1 -DWORKER=1
> > > -DTEST_NAME=test4
> >
> > I assume that you didn't just use the actual names (COMPRESS_STAT) to make
> > sure
> > the test still works even when the user adds these symbols to their CFLAGS.
> > But
> > I'd prefer slightly more descriptive names for these defines. Perhaps
> > TEST_COMPRESS_STAT or similar instead of COMP? Same comment for the generic
> > TEST_NAME values (perhaps stat_test1 instead of test1, etc)
> >
> > > diff --git a/server/tests/stat-main.c b/server/tests/stat-main.c
> > > new file mode 100644
> > > index 0000000..e7a7df2
> > > --- /dev/null
> > > +++ b/server/tests/stat-main.c
> > > @@ -0,0 +1,49 @@
> > > +/* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */
> > > +/*
> > > + Copyright (C) 2015 Red Hat, Inc.
> > > +
> > > + This library is free software; you can redistribute it and/or
> > > + modify it under the terms of the GNU Lesser General Public
> > > + License as published by the Free Software Foundation; either
> > > + version 2.1 of the License, or (at your option) any later version.
> > > +
> > > + This library is distributed in the hope that it will be useful,
> > > + but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> > > + Lesser General Public License for more details.
> > > +
> > > + You should have received a copy of the GNU Lesser General Public
> > > + License along with this library; if not, see
> > > <http://www.gnu.org/licenses/
> > > > .
> > > +*/
> > > +
> > > +/* check statistics
> > > + */
> > > +
> > > +#include <config.h>
> > > +#include <glib.h>
> > > +
> > > +typedef void test_func_t(void);
> > > +
> > > +test_func_t test1;
> > > +test_func_t test2;
> > > +test_func_t test3;
> > > +test_func_t test4;
> > > +
> > > +static test_func_t *test_funcs[] = {
> > > + test1,
> > > + test2,
> > > + test3,
> > > + test4,
> > > + NULL
> > > +};
> > > +
> > > +int main(void)
> > > +{
> > > + test_func_t **func_p;
> > > +
> > > + for (func_p = test_funcs; *func_p; ++func_p) {
> > > + (*func_p)();
> > > + }
> > > + return 0;
> > > +}
> > > +
> > > diff --git a/server/tests/stat-test.c b/server/tests/stat-test.c
> > > new file mode 100644
> > > index 0000000..db267e7
> > > --- /dev/null
> > > +++ b/server/tests/stat-test.c
> > > @@ -0,0 +1,83 @@
> > > +/* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */
> > > +/*
> > > + Copyright (C) 2015 Red Hat, Inc.
> > > +
> > > + This library is free software; you can redistribute it and/or
> > > + modify it under the terms of the GNU Lesser General Public
> > > + License as published by the Free Software Foundation; either
> > > + version 2.1 of the License, or (at your option) any later version.
> > > +
> > > + This library is distributed in the hope that it will be useful,
> > > + but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> > > + Lesser General Public License for more details.
> > > +
> > > + You should have received a copy of the GNU Lesser General Public
> > > + License along with this library; if not, see
> > > <http://www.gnu.org/licenses/
> > > > .
> > > +*/
> > > +
> > > +/* check statistics
> > > + */
> > > +
> > > +#include <config.h>
> > > +
> > > +#undef COMPRESS_STAT
> > > +#undef RED_WORKER_STAT
> > > +
> > > +#if COMP
> > > +#define COMPRESS_STAT
> > > +#endif
> > > +
> > > +#if WORKER
> > > +#define RED_WORKER_STAT
> > > +#endif
> > > +
> > > +#include <unistd.h>
> > > +#include <glib.h>
> > > +#include "../stat.h"
> > > +
> > > +#ifndef TEST_NAME
> > > +#error TEST_NAME must be defined!
> > > +#endif
> > > +
> > > +void TEST_NAME(void)
> > > +{
> > > + stat_info_t info;
> > > + stat_start_time_t start_time;
> > > +
> > > +#if !defined(COMPRESS_STAT) && !defined(RED_WORKER_STAT)
> > > + G_STATIC_ASSERT(sizeof(start_time) == 0);
> > > + G_STATIC_ASSERT(sizeof(info) == 0);
> > > +#endif
> > > +
> > > + stat_init(&info, "test", CLOCK_MONOTONIC);
> > > + stat_start_time_init(&start_time, &info);
> > > + usleep(2);
> > > + stat_add(&info, start_time);
> > > +
> > > +#ifdef RED_WORKER_STAT
> > > + g_assert_cmpuint(info.count, ==, 1);
> > > + g_assert_cmpuint(info.min, ==, info.max);
> > > + g_assert_cmpuint(info.min, >=, 2000);
> > > + g_assert_cmpuint(info.min, <, 100000000);
> > > +#endif
> > > +
> > > + stat_reset(&info);
> > > +
> > > + stat_compress_init(&info, "test", CLOCK_MONOTONIC);
> > > + stat_start_time_init(&start_time, &info);
> > > + usleep(2);
> > > + stat_compress_add(&info, start_time, 100, 50);
> > > + usleep(1);
> > > + stat_compress_add(&info, start_time, 1000, 500);
> > > +
> > > +#ifdef COMPRESS_STAT
> > > + g_assert_cmpuint(info.count, ==, 2);
> > > + g_assert_cmpuint(info.min, !=, info.max);
> > > + g_assert_cmpuint(info.min, >=, 2000);
> >
> > It seems strange to me that stat_compress_init() above does not reset the
> > start
> > time when it sets a new clock.
>
> stat_compress_init just initialize the structure that holds the statistics,
> the structure has no timing information beside elapsed statistics.
>
> > I would have expected this value to be 1000,
> > not
> > 2000...
> >
>
> First stat_compress_add see 2000 nanoseconds at least while
> second stat_compress_add see 3000 nanoseconds at least.
> When you add statistics timer is not reset.
>
> > > + g_assert_cmpuint(info.min, <, 100000000);
> > > + g_assert_cmpuint(info.total, >=, 5000);
> >
> > ...and I would have expected this value to be 3000, not 5000.
> >
>
> That's why we write tests :)
Yes, I understand. I'm just wondering if this is actually the desired behavior.
As far as I know, nobody has really used this stats stuff for a while, so it
might be doing the wrong thing (?)
>
> > > + g_assert_cmpuint(info.orig_size, ==, 1100);
> > > + g_assert_cmpuint(info.comp_size, ==, 550);
> > > +#endif
> > > +}
> >
>
> Frediano
More information about the Spice-devel
mailing list