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

[Thomas Monjalon <thomas@monjalon.net>]

16/03/2020 13:17, Richardson, Bruce:
> From: 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
> > 
> Do we anticipate others setting up public labs? I suppose it doesn't hurt to
> document this even if no other labs are set up.

I cannot speak for others, but we can already document the community lab,
the Intel lab and the smaller free "labs", i.e. services, from Coverity,
Travis and OBS.
I will work on it as a second step.


> > 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?
> > 
> Ok, happy to keep it all in one place.
> 
> > > 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?
> > 
> We just need to clarify what type(s) of neutrality is in mind. Vendor neutral is probably what people most look for, so I suggest changing it to that - it implies HW neutral I think.

OK

> > > > +- 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.
> > 
> I think just "transparency" is probably the best title then.

OK for transparency.

> However, to clarify things, I think it might be good to have a one-line explanation of each bullet point to allow additional clarity. For example:
> 
> - reliable: users need to know that any failures are genuine, and not the result of intermittent issues in the test infrastructure
> - neutral: the lab should not focus on one HW or SW platform to the exclusion or detriment of others
> - transparent: the details of what tests have been run and the result of those tests should be visible to all users.
> 
> > > > +- 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
> > 
> Yes, I agree they are needed, they just don't fit in the same list as reliability, transparency and neutrality, which are abstract concepts vs coverage reports, trend graphs etc. which are concrete outputs. Therefore I suggest they be put in a separate list on the page, not dropped altogether.

Transparency is also achieved through some outputs.
And the last two ones (add and reproduce) are not outputs.

I understand your concern but I don't know how to separate lists.


> > [...]
> > > > +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.
> 
> Maybe split it into two levels of bullets:
> 
> - compilation
> - static analysis
> - test case runs, including
>   - functional tests
>   - performance tests
>   - interoperability tests
>   - ....
> 
> If that doesn't work for you, it's ok to leave as-is. Everyone will understand it, it just reads a little weird to me. 😊

OK for the nested bullet for runtime tests.

> > [...]
> > > > +### 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?
> > 
> Ah, ok, I understand a little better. To emphasise the triggering rather than what the scan runs on, I suggest "on email reception", or change "run" to "triggered" as in - "triggered on receipt of the patch email, and run on the email itself."

OK thanks.
Note: I tried to understand the difference between receiving, reception and receipt before sending this patch.
Looks like I failed to pick the best English (or Irish) form ;)

Thanks for the good review.