Re: [LTP] [PATCH 1/2] metadata: add tests grouping support

[Andrea Cervesato via ltp <ltp@lists.linux.it>]

Hi Cyril,

> Hi!
> > 1. Source file path - the two nearest parent directories (immediate
> >    parent first), skipping 'kernel' as too generic. For example:
> >    - testcases/kernel/syscalls/clone/clone01.c -> clone, syscalls
> >    - testcases/kernel/kvm/kvm_pagefault01.c -> kvm
> >    - testcases/cve/cve-2017-16939.c -> cve
> > 
> > 2. @group tags in the doc comment block, e.g.:
> >    /*    * Test description.
> >     *
> >     * @group stress
> 
> Maybe call it groups and allow white-space separated list?
> 
> I'm not really sure about the syntax, but I guess that @groups foo bar
> will do.

I was thinking that having `@group foo` is cleaner. When we start to have
multiple groups it can be really to handle multiple groups: for instance,
we might end up having one line over 80 chars in the description.

@groups foo1 bar1 foo2 bar2 foo3 bar3 ..

Instead, we might have

@group foo1
@group bar1
@group foo2
@group bar2
@group foo3
@group bar3

These strings are short, but if we have long names it can be weird
to read having all of them on a single line.

> 
> >     */
> > 
> > Add test case for @group tag parsing.
> > 
> > Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
> > ---
> >  metadata/metaparse.c         | 88 ++++++++++++++++++++++++++++++++++++++++++++
> >  metadata/tests/groups.c      | 11 ++++++
> >  metadata/tests/groups.c.json | 13 +++++++
> >  3 files changed, 112 insertions(+)
> > 
> > diff --git a/metadata/metaparse.c b/metadata/metaparse.c
> > index 561cbb9d2d54689988c9aa49d591628696bcf847..6bc4b7af60c7449d4b60a1252fa58fed77e03066 100644
> > --- a/metadata/metaparse.c
> > +++ b/metadata/metaparse.c
> > @@ -1168,6 +1168,92 @@ static void print_help(const char *prgname)
> >  	exit(0);
> >  }
> >  
> > +/*
> > + * Add groups derived from the source file path.
> > + *
> > + * Groups are the two nearest parent directories (immediate parent
> > + * first), skipping 'kernel' as it's too generic:
> > + *
> > + *   testcases/kernel/syscalls/clone/clone01.c  -> clone, syscalls
> > + *   testcases/kernel/kvm/kvm_pagefault01.c     -> kvm
> > + *   testcases/cve/cve-2017-16939.c             -> cve
> > + */
> > +static void add_path_groups(struct data_node *groups, const char *fname)
> > +{
> > +	char buf[256];
> > +	int offsets[8];
> > +	int ndirs = 0;
> > +	int ngroups = 0;
> > +	char *p;
> > +
> > +	if (strncmp(fname, "testcases/", 10))
> > +		return;
> > +
> > +	snprintf(buf, sizeof(buf), "%s", fname + 10);
> 
> Maybe avoid static buffers here with:
> 
> 	buf = strdup(fname + 10);
> 

we can eventually just loop on file name and using offsets, but a
small static buffer makes things clear. I will try to handle it
differently.

> > +	p = strtok(buf, "/");
> > +	while (p && ndirs < 8) {
> > +		offsets[ndirs++] = p - buf;
> 
> Why do we store offset rather than the pointer to the string?
> 
> 		dirs[ndirs++] = p;

Leftover of experiments I guess..

> 
> > +		p = strtok(NULL, "/");
> > +	}
> > +
> > +	/* Last element is the filename, skip it */
> > +	ndirs--;
> > +
> > +	for (int j = ndirs - 1; j >= 0 && ngroups < 2; j--) {
> > +		if (!strcmp(buf + offsets[j], "kernel"))
> > +			continue;
> > +
> > +		data_node_array_add(groups, data_node_string(buf + offsets[j]));
> > +		ngroups++;
> > +	}
> 
> We can avoid the complex loop by adding a function to add the group with
> the "kernel" filter and just calling it twice here:
> 
> 	add_group(groups, dirs[ndirs-1]);
> 	add_group(groups, dirs[ndirs-2]);

This makes sense yes, we are not gonna add any folder after kernel anyway.

> 
> > +}
> > +
> > +/*
> > + * Add groups from @group tags in the doc comment block.
> > + */
> > +static void add_doc_groups(struct data_node *groups, struct data_node *doc)
> > +{
> > +	if (!doc || doc->type != DATA_ARRAY)
> > +		return;
> > +
> > +	for (unsigned int i = 0; i < data_node_array_len(doc); i++) {
> > +		struct data_node *line = doc->array.array[i];
> > +		const char *s;
> > +
> > +		if (line->type != DATA_STRING)
> > +			continue;
> > +
> > +		s = line->string.val;
> > +
> > +		while (*s && (*s == ' ' || *s == '\t'))
> > +			s++;
> > +
> > +		if (strncmp(s, "@group ", 7))
> > +			continue;
> > +
> > +		s += 7;
> > +		while (*s && (*s == ' ' || *s == '\t'))
> > +			s++;
> > +
> > +		if (*s)
> > +			data_node_array_add(groups, data_node_string(s));
> > +	}
> 
> If we hook up into the multiline_comment() function we can consume the
> line as well so that it does not appear in the parsed doc string. We
> would have to pass the groups node from the main all the way to
> multiline_comment() something as (uncomplete):

Will take a look into it.

--
Andrea Cervesato
SUSE QE Automation Engineer Linux
andrea.cervesato@suse.com

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp