Re: [XFSTESTS v4 0/4] Richacl tests

Re: [XFSTESTS v4 0/4] Richacl tests

[Dave Chinner]

On Wed, Mar 09, 2016 at 01:06:34PM +0100, Andreas Gruenbacher wrote:
> Hello,
> 
> here is a new version of the richacl tests.

xfstests patches need to be sent to fstests@vger.kernel.org (added
to CC list), not xfs@oss.sgi.com.

> According to feedback from the
> previous posting (http://oss.sgi.com/archives/xfs/2015-12/msg00316.html), each
> of the richacl tests is not run separately, on a new scratch filesystem.

Oh, my. So, you've taken this one comment:

	"The rule of thumb is that there should be one xfs test per
	individual regression test. You've got at least 10 separate
	regression tests there, so there should be at least 10
	xfstests.  They should not be aggregated into a single test
	- if you need to run them all at once, then that is what the
	richacl test group is for..."

And then *implemented your own execution infrastructure* so that the
tests are /listed/ as separate tests in a group file but you still
/run them/ as one test?

I'm almost lost for words.

It seems to me that you've ignored all the comments Eric and I have
made to you about properly integrating the tests into xfstests so
that they are able to be maintained by anyone who works with
xfstests. Instead, you've kept most of the wacky stuff and instead
made the richacl tests even more of a special snowflake than they
were before.

This is not rocket science, Andreas. Both Eric and I have spelt out
exactly how to convert the richacl test scripts to use xfstests
scripts and infrastructure (e.g.
http://oss.sgi.com/archives/xfs/2015-11/msg00506.html), but you seem
to be willfully ignoring the feedback you are being given. i.e.

	- the separation of tests between richacl/<test> and
	  tests/<fs>/<test number> is wrong. Implement the
	  tests directly inside tests/<fs>/<test number>, using
	  xfstests infrastructure, please.

	- still not using .out files and instead are using your own
	  internal frankenstein output matching to determine success
	  or failure. Use the xfstests infrastructure for golden
	  output matching, please.

	- now has weird-ass richacl test execution from generic/338
	  and execute the tests correctly from the test harness
	  itself. Again, use the xfstests infrastructure correctly
	  rather than reinventing your own, please.

Most of this is as simple as copying the execution parts of your
scripts to the xfstests test scripts, and the output parts of the
test scripts into the test.out file. There's no new infrastructure
needed for running tests, no separate richacl/ script directory,
etc.

-Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [XFSTESTS v4 0/4] Richacl tests

[Andreas Gruenbacher]

On Mon, Mar 14, 2016 at 11:24 PM, Dave Chinner <david@fromorbit.com> wrote:
> On Wed, Mar 09, 2016 at 01:06:34PM +0100, Andreas Gruenbacher wrote:
>> Hello,
>>
>> here is a new version of the richacl tests.
>
> xfstests patches need to be sent to fstests@vger.kernel.org (added
> to CC list), not xfs@oss.sgi.com.

Sorry for that.

>> According to feedback from the
>> previous posting (http://oss.sgi.com/archives/xfs/2015-12/msg00316.html), each
>> of the richacl tests is not run separately, on a new scratch filesystem.
>
> Oh, my. So, you've taken this one comment:
>
>         "The rule of thumb is that there should be one xfs test per
>         individual regression test. You've got at least 10 separate
>         regression tests there, so there should be at least 10
>         xfstests.  They should not be aggregated into a single test
>         - if you need to run them all at once, then that is what the
>         richacl test group is for..."
>
> And then *implemented your own execution infrastructure* so that the
> tests are /listed/ as separate tests in a group file but you still
> /run them/ as one test?
>
> I'm almost lost for words.

What? Each test runs separately; not sure what makes you think otherwise.

> It seems to me that you've ignored all the comments Eric and I have
> made to you about properly integrating the tests into xfstests so
> that they are able to be maintained by anyone who works with
> xfstests. Instead, you've kept most of the wacky stuff and instead
> made the richacl tests even more of a special snowflake than they
> were before.
>
> This is not rocket science, Andreas. Both Eric and I have spelt out
> exactly how to convert the richacl test scripts to use xfstests
> scripts and infrastructure (e.g.
> http://oss.sgi.com/archives/xfs/2015-11/msg00506.html), but you seem
> to be willfully ignoring the feedback you are being given. i.e.
>
>         - the separation of tests between richacl/<test> and
>           tests/<fs>/<test number> is wrong. Implement the
>           tests directly inside tests/<fs>/<test number>, using
>           xfstests infrastructure, please.
>
>         - still not using .out files and instead are using your own
>           internal frankenstein output matching to determine success
>           or failure. Use the xfstests infrastructure for golden
>           output matching, please.
>
>         - now has weird-ass richacl test execution from generic/338
>           and execute the tests correctly from the test harness
>           itself. Again, use the xfstests infrastructure correctly
>           rather than reinventing your own, please.
>
> Most of this is as simple as copying the execution parts of your
> scripts to the xfstests test scripts, and the output parts of the
> test scripts into the test.out file. There's no new infrastructure
> needed for running tests, no separate richacl/ script directory,
> etc.

I've said again and again that maintaining one set of richacl tests in
xfstests and another in the richacl package is going to really
painful, and that because of that, I'm trying to find a way of using
the same test scripts in both places. That message obviously didn't
get through at all though. That is just sad.

Andreas

Re: [XFSTESTS v4 0/4] Richacl tests

[Eric Sandeen]

On 3/14/16 6:23 PM, Andreas Gruenbacher wrote:
> On Mon, Mar 14, 2016 at 11:24 PM, Dave Chinner <david@fromorbit.com> wrote:

<snip>

>> Most of this is as simple as copying the execution parts of your
>> scripts to the xfstests test scripts, and the output parts of the
>> test scripts into the test.out file. There's no new infrastructure
>> needed for running tests, no separate richacl/ script directory,
>> etc.
> 
> I've said again and again that maintaining one set of richacl tests in
> xfstests and another in the richacl package is going to really
> painful, and that because of that, I'm trying to find a way of using
> the same test scripts in both places. That message obviously didn't
> get through at all though. That is just sad.

Andreas, the issue as I see it is that xfstests is a large community
project, with many users who have come to understand its implementation
and its quirks^Wconventions.  It is run and maintained by many people;
over 150 authors have committed patches to the codebase.  They understand
how it all works together; it is a common language.

The way you are proposing your richacl tests integration is unlike any
other tests in the codebase; you have, to some degree, spliced your own
test harness into xfstests, rather than following the advice of
"When in Rome, do as the Romans do."

This might make your life a little easier if you plan to maintain
a separate repo of tests; in the meantime it makes life harder for the
150+ people who will be running and maintaining xfstests, not your
test repo.

The advantage of xfstests is that is is a common language, warts
and all.  It is run by developers and qa groups all over the world.
There is a learning curve, but many people have learned it.
Implementing your tests in this way adds a new and unique learning
curve for all those people, and will make it less likely that others
will share in the maintenance burden for these new tests.

What I would suggest is that if you have tests which test only
richacl userspace functionality, then perhaps you should keep
them private to the richacl package.  For tests which test kernel
functionality, make them native to xfstests.  This way you will get
good coverage and maintenance help from all the people testing
kernelspace with xfstests, and those hacking on richacls can run the
tests local to it.

This is more or less what e2fsprogs has done, and it seems to work
out ok.

-Eric

Re: [XFSTESTS v4 0/4] Richacl tests

[Dave Chinner]

On Tue, Mar 15, 2016 at 12:23:57AM +0100, Andreas Gruenbacher wrote:
> On Mon, Mar 14, 2016 at 11:24 PM, Dave Chinner <david@fromorbit.com> wrote:
> > On Wed, Mar 09, 2016 at 01:06:34PM +0100, Andreas Gruenbacher wrote:
> >> Hello,
> >>
> >> here is a new version of the richacl tests.
> >
> > xfstests patches need to be sent to fstests@vger.kernel.org (added
> > to CC list), not xfs@oss.sgi.com.
> 
> Sorry for that.
> 
> >> According to feedback from the
> >> previous posting (http://oss.sgi.com/archives/xfs/2015-12/msg00316.html), each
> >> of the richacl tests is not run separately, on a new scratch filesystem.
> >
> > Oh, my. So, you've taken this one comment:
> >
> >         "The rule of thumb is that there should be one xfs test per
> >         individual regression test. You've got at least 10 separate
> >         regression tests there, so there should be at least 10
> >         xfstests.  They should not be aggregated into a single test
> >         - if you need to run them all at once, then that is what the
> >         richacl test group is for..."
> >
> > And then *implemented your own execution infrastructure* so that the
> > tests are /listed/ as separate tests in a group file but you still
> > /run them/ as one test?
> >
> > I'm almost lost for words.
> 
> What? Each test runs separately; not sure what makes you think otherwise.

You added test generic/338, which is *not included in the group
file* and so is not run by the test harness. then you added
tests generic/339-348, which have a name suffix that matches the
script they are supposed to run located in the richacl/ directory.

tests generic/339-348 do one thing: they execute generic/338,
which then looks at the calling program name (e.g. 339-apply-masks)
strips out the test number to get the script it's supposed to run.
then it runs $here/richacl/apply-masks, captures all the
output and only if the test fails (based on the return value of
the script being run) does it dump the output of the test.


If I renumber tests on commit (which I do regularly), this
magical "run test generic/338" mechanism breaks completely. It's
fragile, unmaintainable and *completely unnecessary*.

> I've said again and again that maintaining one set of richacl tests in
> xfstests and another in the richacl package is going to really
> painful, and that because of that, I'm trying to find a way of using
> the same test scripts in both places. That message obviously didn't
> get through at all though. That is just sad.

Yes, I fully understand that's what you are trying to do. I've
already explained to you why your approached doesn't work for
xfstests, which Eric has further explained in his response.

Once we add a test to xfstests, the author *loses control* of the
test. Nobody "owns" a set of tests - anyone can modify them and we
expect that *everyone* will modify them. e.g. if someone is changing
some generic infrastructure, we don't expect them to have to
understand that there are these magical richacl tests that you can't
move or filter or change in the same way all the other hundreds of
tests can be changed. All the tests need to look the same, run the
same way, and be structured in the same way. Otherwise we are simply
setting ourselve up with a long term maintenance nightmare.

If you really need to keep an identical copy of the tests somewhere
else, then make the xfstests versions the master copy and write your
own harness wrapper around the outside of those scripts. Keeping
tests in standard format in xfstests is far more important to me
than whether they get kept in sync with some other external
package...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com