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@xxxxxxxxxx> > > > --- > > > 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 _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel