[PATCH weston v6 2/4] Enables output in the JUnit XML format.
Jon A. Cruz
jonc at osg.samsung.com
Wed Jul 15 17:21:16 PDT 2015
On 07/07/2015 04:49 AM, Pekka Paalanen wrote:
> On Thu, 2 Jul 2015 23:36:45 -0700
> "Jon A. Cruz" <jonc at osg.samsung.com> wrote:
>
>> Adds basic support for optionally outputting in the XML format
>> commonly used by JUnit compatible tools.
>>
>> This format is supported by default by many tools, including
>> the Jenkins build system. It also is more detailed and
>> captures more information than the more simplistic TAP
>> format.
>>
>> Signed-off-by: Jon A. Cruz <jonc at osg.samsung.com>
>> ---
>> Makefile.am | 9 +
>> configure.ac | 26 ++
>> tools/zunitc/src/zuc_junit_reporter.c | 488 ++++++++++++++++++++++++++++++++++
>> tools/zunitc/src/zuc_junit_reporter.h | 38 +++
>> tools/zunitc/src/zunitc_impl.c | 17 ++
>> 5 files changed, 578 insertions(+)
>> create mode 100644 tools/zunitc/src/zuc_junit_reporter.c
>> create mode 100644 tools/zunitc/src/zuc_junit_reporter.h
>>
>> diff --git a/Makefile.am b/Makefile.am
>> index 4f0a450..89d9e4c 100644
>> --- a/Makefile.am
>> +++ b/Makefile.am
>> @@ -982,6 +982,8 @@ libzunitc_la_SOURCES = \
>> tools/zunitc/src/zuc_context.h \
>> tools/zunitc/src/zuc_event.h \
>> tools/zunitc/src/zuc_event_listener.h \
>> + tools/zunitc/src/zuc_junit_reporter.c \
>> + tools/zunitc/src/zuc_junit_reporter.h \
>> tools/zunitc/src/zuc_types.h \
>> tools/zunitc/src/zunitc_impl.c \
>> shared/helpers.h
>> @@ -993,6 +995,13 @@ libzunitc_la_CFLAGS = \
>> libzunitc_la_LIBADD = \
>> libshared.la
>>
>> +if ENABLE_JUNIT_XML
>> +libzunitc_la_CFLAGS += \
>> + $(LIBXML2_CFLAGS)
>> +libzunitc_la_LIBADD += \
>> + $(LIBXML2_LIBS)
>> +endif
>> +
>> libzunitcmain_la_SOURCES = \
>> tools/zunitc/src/main.c
>>
>> diff --git a/configure.ac b/configure.ac
>> index 404418e..b67df39 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -426,6 +426,31 @@ if test "x$enable_dbus" != "xno"; then
>> fi
>> AM_CONDITIONAL(ENABLE_DBUS, test "x$enable_dbus" = "xyes")
>>
>> +# Note that other features might want libxml2, or this feature might use
>> +# alternative xml libraries at some point. Therefore the feature and
>> +# pre-requisite concepts are split.
>> +AC_ARG_ENABLE(junit_xml,
>> + AS_HELP_STRING([--disable-junit-xml],
>> + [do not build with JUnit XML output]),,
>> + enable_junit_xml=auto)
>> +if test "x$enable_junit_xml" != "xno"; then
>> + PKG_CHECK_MODULES(LIBXML2,
>> + [libxml-2.0 >= 2.6],
>> + have_libxml2=yes,
>> + have_libxml2=no)
>> + if test "x$have_libxml2" = "xno" -a "x$enable_junit_xml" = "xyes"; then
>> + AC_MSG_ERROR([JUnit XML support explicitly requested, but libxml2 couldn't be found])
>> + fi
>> + if test "x$have_libxml2" = "xyes"; then
>> + enable_junit_xml=yes
>> + AC_DEFINE(HAVE_LIBXML2, [1], [Define if libxml2 is available])
>
> Hi Jon,
>
> HAVE_LIBXML2 is never used, so not needed here.
>
Removed. I was actually going to use it to guard the #include of libxml
stuff so as to allow for other libs to write XML. But as per the general
rules of refactoring that conditional should not be used until there is
actual code that uses an alternative.
>> + AC_DEFINE(ENABLE_JUNIT_XML, [1], [Build Weston with JUnit output support])
>> + else
>> + enable_junit_xml=no
>> + fi
>> +fi
>> +AM_CONDITIONAL(ENABLE_JUNIT_XML, test "x$enable_junit_xml" = "xyes")
>> +
>> # ivi-shell support
>> AC_ARG_ENABLE(ivi-shell,
>> AS_HELP_STRING([--disable-ivi-shell],
>> @@ -537,6 +562,7 @@ AC_MSG_RESULT([
>> FBDEV Compositor ${enable_fbdev_compositor}
>> RDP Compositor ${enable_rdp_compositor}
>> Screen Sharing ${enable_screen_sharing}
>> + JUnit XML output ${enable_junit_xml}
>>
>> Raspberry Pi BCM headers ${have_bcm_host}
>>
>> diff --git a/tools/zunitc/src/zuc_junit_reporter.c b/tools/zunitc/src/zuc_junit_reporter.c
>> new file mode 100644
>> index 0000000..7e05fbf
>> --- /dev/null
>> +++ b/tools/zunitc/src/zuc_junit_reporter.c
>> @@ -0,0 +1,488 @@
>> +/*
>> + * Copyright © 2015 Samsung Electronics Co., Ltd
>
>> + */
>> +
>> +#include "config.h"
>> +
>> +#include "zuc_junit_reporter.h"
>> +
>> +#if ENABLE_JUNIT_XML
>> +
>> +#include <fcntl.h>
>> +#include <libxml/parser.h>
>> +#include <memory.h>
>> +#include <stdio.h>
>> +#include <sys/stat.h>
>> +#include <sys/types.h>
>> +#include <time.h>
>> +#include <unistd.h>
>> +
>> +#include "zuc_event_listener.h"
>> +#include "zuc_types.h"
>> +
>> +#include "shared/zalloc.h"
>> +
>> +#define XML_FNAME "test_detail.xml"
>> +
>> +#define ISO_8601_FORMAT "%Y-%m-%dT%H:%M:%SZ"
>> +
>> +/**
>> + * Internal data.
>> + */
>> +struct junit_data
>> +{
>> + int fd;
>> + time_t begin;
>> +};
>> +
>> +/**
>> + * Formats a time in milliseconds to the normal JUnit elapsed form, or
>> + * NULL if there is a problem.
>> + * The caller should release this with free()
>> + *
>> + * @return the formatted time string upon success, NULL otherwise.
>> + */
>> +static char *
>> +as_duration(long ms);
>> +
>> +/**
>> + * Formats a time in milliseconds to the full ISO-8601 date/time string
>> + * format, or NULL if there is a problem.
>> + * The caller should release this with free()
>> + *
>> + * @return the formatted time string upon success, NULL otherwise.
>> + */
>> +static char *
>> +as_iso_8601(time_t const *t);
>> +
>> +/**
>> + * Returns the status string for the tests (run/notrun).
>> + *
>> + * @param test the test to check status of.
>> + * @return the status string.
>> + */
>> +static char const *
>> +get_test_status(struct zuc_test *test);
>> +
>> +/**
>> + * Output the given event.
>> + *
>> + * @param parent the parent node to add new content to.
>> + * @param event the event to write out.
>> + */
>> +static void
>> +emit_event(xmlNodePtr parent, struct zuc_event *event);
>> +
>> +/**
>> + * Output the given test.
>> + *
>> + * @param parent the parent node to add new content to.
>> + * @param test the test to write out.
>> + */
>> +static void
>> +emit_test(xmlNodePtr parent, struct zuc_test *test);
>> +
>> +/**
>> + * Output the given test case.
>> + *
>> + * @param parent the parent node to add new content to.
>> + * @param test_case the test case to write out.
>> + */
>> +static void
>> +emit_case(xmlNodePtr parent, struct zuc_case *test_case);
>> +
>> +static void
>> +destroy(void *data);
>> +
>> +static void
>> +run_started(void *data, int live_case_count, int live_test_count,
>> + int disabled_count);
>> +
>> +static void
>> +run_ended(void *data, int case_count, struct zuc_case **cases,
>> + int live_case_count, int live_test_count, int total_passed,
>> + int total_failed, int total_disabled, long total_elapsed);
>
> Lots of static functions declared instead of defined. Do we really need
> these declarations separate from the definition?
>
File had escaped my previous cleanup on this point. Should be clear of
forward declarations now.
>> +
>> +struct zuc_event_listener *
>> +zuc_junit_reporter_create(void)
>> +{
>> + struct zuc_event_listener *listener =
>> + zalloc(sizeof(struct zuc_event_listener));
>> +
>> + struct junit_data *data = zalloc(sizeof(struct junit_data));
>
> Empty line between decls and code.
>
>> + data->fd = -1;
>> +
>> + listener->data = data;
>> + listener->destroy = destroy;
>> + listener->run_started = run_started;
>> + listener->run_ended = run_ended;
>> +
>> + return listener;
>> +}
>> +
>> +void
>> +destroy(void *data)
>> +{
>> + xmlCleanupParser();
>> +
>> + free(data);
>> +}
>
> When I read this definition, it makes me think this is not static. A
> non-static function with such a very generic name raises an eye-brow.
> (An example of how the declarations confuse me here.)
>
Figured out that this is another habit from working in mixed C + C++
codebases. If something has already been declared elsewhere C is happy
with or without 'static', but C++ prohibits its use. Now that I know why
I had been leaving it off it should be easier for me to catch and
correct before submitting.
>> +
>> +void
>> +run_started(void *data, int live_case_count, int live_test_count,
>> + int disabled_count)
>> +{
>> + struct junit_data *jdata = data;
>> + jdata->begin = time(NULL);
>> + jdata->fd = open(XML_FNAME, O_WRONLY | O_CLOEXEC | O_CREAT | O_TRUNC,
>> + S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH);
>
> Does it not matter whether open fails? Does something else catch it?
>
> Btw. since this is always creating and truncating the same file, how
> does this work when we execute several test programs
> one-after-the-other or even in parallel?
This actually mirrors the behavior of some test frameworks, but you have
correctly identified this as sub-optimal. The intention was to follow up
with adding a good mechanism for overriding the name. (It was delayed
due to limitations in weston's current options parser)
Marked explicitly as a @todo for follow-up fixing.
>> +}
>> +
>> +char const *
>> +get_test_status(struct zuc_test *test)
>> +{
>> + if (test->disabled || test->skipped)
>> + return "notrun";
>> + else
>> + return "run";
>> +}
>> +
>> +#define MAX_64BIT_STRLEN 20
>> +
>> +static void
>> +set_attribute(xmlNodePtr node, const char *name, int value)
>> +{
>> + xmlChar scratch[MAX_64BIT_STRLEN + 1] = {};
>> + xmlStrPrintf(scratch, sizeof(scratch), BAD_CAST "%d", value);
>> + xmlSetProp(node, BAD_CAST name, scratch);
>> +}
>
>> +char *
>> +as_duration(long ms)
>> +{
>> + char *str = NULL;
>> + if (asprintf(&str, "%1.3f", ms / 1000.0) < 0) {
>> + str = NULL;
>> + } else {
>> + if (!strcmp("0.000", str)) {
>> + free(str);
>> + str = strdup("0");
>
> That's a funny sequence. Why is 0.000 bad?
Comment added in code for explanation.
"0.000" is bad since existing test frameworks avoid it. To keep safely
compatible with various tools that read JUnit style output, what is
written should be kept 100% identical where possible.
>
>> + }
>> + }
>> + return str;
>> +}
>> +
>> +char *
>> +as_iso_8601(time_t const *t)
>> +{
>> + char *result = NULL;
>> + char buf[32] = {};
>> + struct tm when;
>> + if (gmtime_r(t, &when) != NULL)
>> + if (strftime(buf, sizeof(buf), ISO_8601_FORMAT, &when))
>> + result = strdup(buf);
>> +
>> + return result;
>> +}
>> +
>> +#else /* ENABLE_JUNIT_XML */
>> +
>> +#include "shared/zalloc.h"
>> +#include "zuc_event_listener.h"
>> +
>> +/*
>> + * Simple stub version if junit output (including libxml2 support) has
>> + * been disabled.
>> + */
>> +
>> +struct zuc_event_listener *
>> +zuc_junit_reporter_create(void)
>> +{
>> + struct zuc_event_listener *listener = zalloc(sizeof(*listener));
>> +
>> + /* Leaving all to default 0/NULL */
>> +
>> + return listener;
>
> Should this rather fail than silently not do anything?
Yes.
>> +}
>> +
>> +#endif /* ENABLE_JUNIT_XML */
>> diff --git a/tools/zunitc/src/zuc_junit_reporter.h b/tools/zunitc/src/zuc_junit_reporter.h
>> new file mode 100644
>> index 0000000..66f3c7b
>> --- /dev/null
>> +++ b/tools/zunitc/src/zuc_junit_reporter.h
>> @@ -0,0 +1,38 @@
>> +/*
>> + * Copyright © 2015 Samsung Electronics Co., Ltd
>
>> + */
>> +
>> +#ifndef ZUC_JUNIT_REPORTER_H
>> +#define ZUC_JUNIT_REPORTER_H
>> +
>> +struct zuc_event_listener;
>> +
>> +/**
>> + * Creates an instance of a reporter that will write data in the JUnit
>> + * XML format.
>> + */
>> +struct zuc_event_listener *
>> +zuc_junit_reporter_create(void);
>> +
>> +#endif /* ZUC_JUNIT_REPORTER_H */
>> diff --git a/tools/zunitc/src/zunitc_impl.c b/tools/zunitc/src/zunitc_impl.c
>> index b8ab0b2..6f591f0 100644
>> --- a/tools/zunitc/src/zunitc_impl.c
>> +++ b/tools/zunitc/src/zunitc_impl.c
>
>
> Otherwise looks good to me.
>
>
> Thanks,
> pq
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
--
Jon A. Cruz - Senior Open Source Developer
Samsung Open Source Group
jonc at osg.samsung.com
More information about the wayland-devel
mailing list