Re: [dpdk-ci] [dpdk-web] [PATCH] add general testing guidelines

[Thomas Monjalon <thomas@monjalon.net>]

16/03/2020 10:43, Richardson, Bruce:
> From: web <web-bounces@dpdk.org> On Behalf Of Thomas Monjalon
> > 
> > The community lab page is replaced with a more general page about testing,
> > where the community lab is part of the big picture. It includes:
> > 	- testing goals
> > 	- CI workflow
> > 	- tools
> 
> General question - is this meant to describe the ideal state or the current state of testing?

Yes it is about goals and workflow details, plus reference the tools which are used.
I think these three items give a big picture.

> It starts off largely inspirational, but then ends with lots of details on the current
> setup. Do we want to have that mix in the one page?

I plan to add one more page to list what is currently done in which lab.
So the split is:
	- one guideline page to setup a lab
	- one status page

I understand your question about splitting the guidelines in several pages
but I don't whether it would help reading, or the opposite, makes harder
to get all informations about what is a lab?

> I've also put some stylistic comments inline below too.
[...]
> > +There are various ways of contributing to the CI effort:
> > +
> > +- suggest improvement
> > +- write a new test
> > +- improve infrastucture scripts
> 
> Typo: infrastRucture

OK thanks

> > +- run a lab
> > +
> > +A specific effort is done
> > +for the [community lab](//lab.dpdk.org/results/dashboard/about)
> > +hosted at [UNH-IOL](//www.iol.unh.edu).
> > +
> 
> Not sure about the phrase "specific effort". How about "One specific lab instance has been set up for the community lab..."

What about this?
One specific lab instance, named community lab, is hosted at UNH-IOL.

[...]
> > +Ideally, the CI infrastructure should have these properties:
> > +
> > +- reliable
> > +- neutral
> 
> Neutral in what way? Presumably vendor or HW neutral?

Neutral is a property which can apply to vendors, HW or SW (OS).
Do you think "vendor neutral" is better?

> > +- transparent visibility
> 
> What is implied by this? It is "clear methodology" or "transparent testing methods"?

I mean we should be able to see which tests are run,
what is the environment, how tests are configured and what is the result.
The property to measure is really the visibility.

> > +- coverage summary
> > +- trending graphs
> > +- reports to author and
> > +[test-report@dpdk.org](//inbox.dpdk.org/test-report/)
> > +- easy to add a test
> > +- easy to reproduce a test locally
> > +
> 
> The first three are what I'd consider properties,
> but the rest are more functions that should be included in the CI, rather than properties of it.

I don't agree:
	- coverage apply to all tests: what is the coverage of each test?
	- trending graphs can be done for any test or measure
	- reporting must be done for all tests
	- ability to add test is an infrastructure property
	- ability to reproduce is a test property

[...]
> > +The tests should cover these categories:
> > +
> > +- compilation
> > +- static analysis
> > +- functional
> > +- performance
> > +- interoperability
> > +- portability
> > +- compatibility
> > +
> 
> While compilation and static analysis are ok, the rest need "testing" after them since they are not nouns. Alternatively drop "static analysis" from the list and then change compilation to "compile" to use only adjectives.

I don't want to drop static analysis, I think it is a test category.
We could add "testing" after each item, but I feel it is just an overhead.
I don't know what to do here.

[...]
> > +### Patch Testing
> > +
> > +When a patch is submitted, it is received via the mailing list
> > +[dpdk-dev](//inbox.dpdk.org/dev/).
> > +For simple tests,
> > +like
> > +[checkpatch](//git.dpdk.org/tools/dpdk-ci/tree/tests/checkpatch.sh),
> > +it can be run directly on email receiving.
> 
> s/email receiving/the received email/

I wanted to emphasize on the trigger which "receiving" instead of "polling".
But "on the received email" is also OK. Is it a better English?

Thanks for the review.