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

["Aníbal Limón" <anibal.limon@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 9940 bytes --]

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?

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
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]