Re: [ptest-runner][PATCH] feat: generate xml-file

["Esquivel, Benjamin" <benjamin.esquivel@intel.com>]

Sure Anibal, see my response inline.

> -----Original Message-----
> From: Aníbal Limón [mailto:anibal.limon@linux.intel.com]
> Sent: Thursday, January 19, 2017 11:42 AM
> To: Pascal Bach <pascal.bach@siemens.com>; yocto@yoctoproject.org;
> Christian Schuler <schuler.christian@siemens.com>; Esquivel, Benjamin
> <benjamin.esquivel@intel.com>
> Subject: Re: [yocto] [ptest-runner][PATCH] feat: generate xml-file
> 
> Hi Pascal and Christian,
> 
> Your patch looks good at first look, only a few comments about the code
> inline.
> 
> We are trying to standardize the results output format across our testing
> components in order to build up-on a Test reporting tool, so let us review the
> XML format.
> 
> Benjamin do you have a wiki page link with test result format information?
https://wiki.yoctoproject.org/wiki/QA/xUnit_XML_Template
> 
> Cheers,
> alimon
> 
> On 01/19/2017 07:33 AM, Pascal Bach wrote:
> > From: Christian Schuler <schuler.christian@siemens.com>
> >
> > add a feature that allows the user to generate an XUnit xml-file.
> > This file shows if the ptests run correctly or not. e.g. bash, zlib.
> > It doesn't list the underlaying tests of the package.
> > The feature is activated by console input -x <filepath>.
> > Unit-tests of the feature are included.
> >
> > Signed-off-by: Christian Schuler <schuler.christian@siemens.com>
> > Signed-off-by: Pascal Bach <pascal.bach@siemens.com>
> > ---
> >  README.md     |  1 +
> >  main.c        | 15 +++++++++++----
> >  tests/utils.c | 46 +++++++++++++++++++++++++++++++++++++++++++--
> >  utils.c       | 60
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >  utils.h       |  7 ++++++-
> >  5 files changed, 121 insertions(+), 8 deletions(-)
> >
> > diff --git a/README.md b/README.md
> > index ed7a589..fedab04 100644
> > --- a/README.md
> > +++ b/README.md
> > @@ -14,6 +14,7 @@ Now the ptest-runner2 support the next features:
> >  - List available ptests.
> >  - Specify the timeout for avoid blocking indefinetly.
> >  - Only run certain ptests.
> > +- XML-ouput
> >
> >  Proposed features:
> >
> > diff --git a/main.c b/main.c
> > index 31bf3b5..8dfdad6 100644
> > --- a/main.c
> > +++ b/main.c
> > @@ -41,7 +41,7 @@
> >  static inline void
> >  print_usage(FILE *stream, char *progname)  {
> > -	fprintf(stream, "Usage: %s [-d directory] [-l list] [-t timeout] "
> > +	fprintf(stream, "Usage: %s [-d directory] [-l list] [-t timeout] [-x xml-
> filename]"
> >  			"[-h] [ptest1 ptest2 ...]\n", progname);  }
> >
> > @@ -50,6 +50,7 @@ static struct {
> >  	int list;
> >  	int timeout;
> >  	char **ptests;
> > +	char * xml_filename;
> >  } opts;
> >
> >  int
> > @@ -70,8 +71,9 @@ main(int argc, char *argv[])
> >  	opts.list = 0;
> >  	opts.timeout = DEFAULT_TIMEOUT;
> >  	opts.ptests = NULL;
> > +	opts.xml_filename = NULL;
> >
> > -	while ((opt = getopt(argc, argv, "d:lt:h")) != -1) {
> > +	while ((opt = getopt(argc, argv, "d:ltx:h")) != -1) {
> >  		switch (opt) {
> >  			case 'd':
> >  				free(opts.directory);
> > @@ -88,6 +90,11 @@ main(int argc, char *argv[])
> >  				print_usage(stdout, argv[0]);
> >  				exit(0);
> >  			break;
> > +			case 'x':
> > +				free(opts.xml_filename);
> > +				opts.xml_filename = strdup(optarg);
> > +				CHECK_ALLOCATION(opts.xml_filename, 1,
> 1);
> > +			break;
> >  			default:
> >  				print_usage(stdout, argv[0]);
> >  				exit(1);
> > @@ -131,9 +138,9 @@ main(int argc, char *argv[])
> >  		run = filter_ptests(head, opts.ptests, ptest_num);
> >  		CHECK_ALLOCATION(run, ptest_num, 1);
> >  		ptest_list_free_all(head);
> > -	}
> > +	}
> >
> > -	rc = run_ptests(run, opts.timeout, argv[0], stdout, stderr);
> > +	rc = run_ptests(run, opts.timeout, argv[0], stdout, stderr,
> > +opts.xml_filename);
> 
> Will be a good idea modify the run_ptest signature to pass the whole options
> to it.
> 
> I mean:
> 
> run_ptests(run, opts, argv[0], stdout, stderr)
> 
> If you could do a first patch with this change and then add your functionality
> realted to xml_filename will be good.
> 
> >
> >  	ptest_list_free_all(run);
> >
> > diff --git a/tests/utils.c b/tests/utils.c index 1332798..dd869e9
> > 100644
> > --- a/tests/utils.c
> > +++ b/tests/utils.c
> > @@ -173,7 +173,7 @@ START_TEST(test_run_ptests)
> >  	ptest_list_remove(head, "hang", 1);
> >  	ptest_list_remove(head, "fail", 1);
> >
> > -	rc = run_ptests(head, timeout, "test_run_ptests", fp_stdout,
> fp_stderr);
> > +	rc = run_ptests(head, timeout, "test_run_ptests", fp_stdout,
> > +fp_stderr, NULL);
> >  	ck_assert(rc == 0);
> >  	ptest_list_free_all(head);
> >
> > @@ -234,6 +234,46 @@ START_TEST(test_run_fail_ptest)
> >  	ptest_list_free_all(head);
> >  END_TEST
> >
> > +START_TEST(test_xml_pass)
> > +	char ref[] = "<?xml version='1.0' encoding='UTF-8'?>\n<testsuite
> name='ptests' tests='2'>\n"
> > +			"<testcase name='.'>\n<nodes expected='/'
> result='/'>\n"
> > +			"<nodes expected='0' result='0'>\n"
> > +			"</nodes>\n"
> > +			"</nodes>\n"
> > +			"</testcase>\n"
> > +			"<testcase name='.'>\n"
> > +			"<nodes expected='/' result='/'>\n"
> > +			"<nodes expected='0' result='1'>\n"
> > +			"<error type='Timeout/Unexpected error'>\n"
> > +			"A Timeout has occured or more tests than expected
> failed.\n"
> > +			"</error>\n"
> > +			"</nodes>\n"
> > +			"</nodes>\n"
> > +			"</testcase>\n"
> > +			"</testsuite>";
> > +	char buffer[strlen(ref)];
> > +
> > +	FILE *fp;
> > +	fp = xml_create(2, "./test_xml");
> > +	xml_add_case(fp, 0,".");
> > +	xml_add_case(fp, 1,".");
> > +	xml_finish(fp);
> > +
> > +	fp = fopen("./test_xml", "a+");
> > +	fseek(fp, 0, SEEK_SET);
> > +
> > +	fread(buffer, strlen(ref), 1, fp);
> > +	ck_assert(strncmp(ref, buffer, strlen(ref)) == 0);
> > +	fclose(fp);
> > +	unlink("./test_xml");
> > +
> > +END_TEST
> > +
> > +START_TEST(test_xml_fail)
> > +	ck_assert(xml_create(2, "./") == NULL);
> > +
> > +END_TEST
> > +
> >  Suite *
> >  utils_suite()
> >  {
> > @@ -249,6 +289,8 @@ utils_suite()
> >  	tcase_add_test(tc_core, test_run_ptests);
> >  	tcase_add_test(tc_core, test_run_timeout_ptest);
> >  	tcase_add_test(tc_core, test_run_fail_ptest);
> > +	tcase_add_test(tc_core, test_xml_pass);
> > +	tcase_add_test(tc_core, test_xml_fail);
> >
> >  	suite_add_tcase(s, tc_core);
> >
> > @@ -276,7 +318,7 @@ test_ptest_expected_failure(struct ptest_list
> *head, const int timeout, char *pr
> >  		ck_assert(ptest_list_length(filtered) == 1);
> >
> >  		h_analizer(
> > -			run_ptests(filtered, timeout, progname, fp_stdout,
> fp_stderr),
> > +			run_ptests(filtered, timeout, progname, fp_stdout,
> fp_stderr,
> > +NULL),
> >  			fp_stdout, fp_stderr
> >  		);
> >
> > diff --git a/utils.c b/utils.c
> > index 77427e0..430fb7a 100644
> > --- a/utils.c
> > +++ b/utils.c
> > @@ -303,9 +303,10 @@ wait_child(const char *ptest_dir, const char
> > *run_ptest, pid_t pid,
> >
> >  int
> >  run_ptests(struct ptest_list *head, int timeout, const char *progname,
> > -		FILE *fp, FILE *fp_stderr)
> > +		FILE *fp, FILE *fp_stderr, char *xml_filename)
> >  {
> >  	int rc = 0;
> > +	XML *xh;
> >
> >  	struct ptest_list *p;
> >  	char stime[GET_STIME_BUF_SIZE];
> > @@ -314,6 +315,12 @@ run_ptests(struct ptest_list *head, int timeout,
> const char *progname,
> >  	int pipefd_stdout[2];
> >  	int pipefd_stderr[2];
> >
> > +	if(xml_filename) {
> > +		xh = xml_create(ptest_list_length(head), xml_filename);
> > +		if(!xh)
> > +			exit(EXIT_FAILURE);
> > +	}
> > +
> >  	do
> >  	{
> >  		if ((rc = pipe2(pipefd_stdout, O_NONBLOCK)) == -1) @@ -
> 355,6 +362,9
> > @@ run_ptests(struct ptest_list *head, int timeout, const char
> *progname,
> >  					fprintf(fp, "ERROR: Exit status is
> %d\n", status);
> >  					rc += 1;
> >  				}
> > +				if (xml_filename)
> > +					xml_add_case(xh, status, ptest_dir);
> > +
> >  				fprintf(fp, "END: %s\n", ptest_dir);
> >  				fprintf(fp, "%s\n", get_stime(stime,
> GET_STIME_BUF_SIZE));
> >  			}
> > @@ -368,5 +378,53 @@ run_ptests(struct ptest_list *head, int timeout,
> const char *progname,
> >  	if (rc == -1)
> >  		fprintf(fp_stderr, "run_ptests fails: %s", strerror(errno));
> >
> > +	if (xml_filename)
> > +		xml_finish(xh);
> > +
> >  	return rc;
> >  }
> > +
> > +FILE *
> > +xml_create(int test_count, char *xml_filename) {
> > +	FILE *xh;
> > +
> > +	if ((xh = fopen(xml_filename, "w"))) {
> > +		fprintf(xh, "<?xml version='1.0' encoding='UTF-8'?>\n");
> > +		fprintf(xh, "<testsuite name='ptests' tests='%d'>\n",
> test_count);
> > +	}
> > +	else {
> > +		fprintf(stderr, "File could not be created. Make sure the path
> is correct and you have the right permissions.");
> > +		return NULL;
> > +	}
> > +
> > +	return xh;
> > +}
> > +
> > +void
> > +xml_add_case(FILE *xh, int status, const char *ptest_dir) {
> > +	fprintf(xh, "<testcase name='%s'>\n", ptest_dir);
> > +	fprintf(xh, "<nodes expected='/' result='/'>\n");
> > +
> > +	if(status == 0){
> > +		fprintf(xh, "<nodes expected='0' result='0'>\n");
> > +		fprintf(xh, "</nodes>\n");
> > +	}
> > +	else{
> > +		fprintf(xh, "<nodes expected='0' result='%d'>\n", status);
> > +		fprintf(xh, "<error type='Timeout/Unexpected error'>\n");
> > +		fprintf(xh, "A Timeout has occured or more tests than
> expected failed.\n");
> > +		fprintf(xh, "</error>\n");
> > +		fprintf(xh, "</nodes>\n");
> > +	}
> > +	fprintf(xh, "</nodes>\n");
> > +	fprintf(xh, "</testcase>\n");
> > +}
> > +
> > +void
> > +xml_finish(FILE *xh)
> > +{
> > +	fprintf(xh, "</testsuite>");
> > +	fclose(xh);
> > +}
> > diff --git a/utils.h b/utils.h
> > index 4dc23ef..99f6100 100644
> > --- a/utils.h
> > +++ b/utils.h
> > @@ -30,10 +30,15 @@
> >  #define CHECK_ALLOCATION(p, size, exit_on_null) \
> >  	check_allocation1(p, size, __FILE__, __LINE__, exit_on_null)
> >
> > +typedef FILE XML;
> > +
> >  extern void check_allocation1(void *, size_t, char *, int, int);
> > extern struct ptest_list *get_available_ptests(const char *);  extern
> > int print_ptests(struct ptest_list *, FILE *);  extern struct
> > ptest_list *filter_ptests(struct ptest_list *, char **, int); -extern
> > int run_ptests(struct ptest_list *, int, const char *progname, FILE *,
> > FILE *);
> > +extern int run_ptests(struct ptest_list *, int, const char *progname,
> > +FILE *, FILE *, char *xml_filename); extern XML * xml_create(int
> > +test_count, char *filename); extern void xml_add_case(XML *, int
> > +status, const char *ptest_dir); extern void xml_finish(XML *);
> 
> I'm trying to follow BSD code-style so please remove variable names from
> prototypes. Also i make a mistake with progname that was before on the
> run_ptests prototype.
> 
> Also i don't wrote about code style into README, i will put a section into it,
> sorry for that.
> 
> Best regards,
> alimon
> 
> >
> >  #endif
> >