Re: [PATCH 1/4] fstests: fix group list generation for whacky test names

["Darrick J. Wong" <djwong@kernel.org>]

On Tue, May 17, 2022 at 07:35:17AM +1000, Dave Chinner wrote:
> On Mon, May 16, 2022 at 09:20:26AM -0700, Darrick J. Wong wrote:
> > On Mon, May 16, 2022 at 06:59:19PM +1000, Dave Chinner wrote:
> > > From: Dave Chinner <dchinner@redhat.com>
> > > 
> > > Darrick noticed that tests/xfs/191-input-validation didn't get
> > > generated properly. Fix the regex to handle this.
> > > 
> > > $ grep -I -R "^_begin_fstest" tests/xfs | \
> > >   sed -e 's/^.*\/\([0-9]*\):_begin_fstest/\1/' |grep 191
> > > tests/xfs/191-input-validation:_begin_fstest auto quick mkfs realtime
> > > $
> > > $ grep -I -R "^_begin_fstest" tests/xfs | \
> > >   sed -e 's/^.*\/\([0-9]*\).*:_begin_fstest/\1/ ' |grep 191
> > > 191 auto quick mkfs realtime
> > > $
> > > 
> > > Long term, we should rename that test to '191' and rip out all that
> > > unused and unnecessary complexity for matching ascii test names
> > > because we just don't use it. Numbers for tests are still working
> > > just fine.
> > > 
> > > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > > ---
> > >  tools/mkgroupfile | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/tools/mkgroupfile b/tools/mkgroupfile
> > > index 24435898..958d4e2f 100755
> > > --- a/tools/mkgroupfile
> > > +++ b/tools/mkgroupfile
> > > @@ -60,7 +60,7 @@ ENDL
> > >  
> > >  	# Aggregate the groups each test belongs to for the group file
> > >  	grep -I -R "^_begin_fstest" $test_dir/ | \
> > > -		sed -e 's/^.*\/\([0-9]*\):_begin_fstest/\1/' >> $new_groups
> > > +		sed -e 's/^.*\/\([0-9]*\).*:_begin_fstest/\1/' >> $new_groups
> > 
> > Sorry I didn't get a chance to review this patch before it went in, but
> > this string parsing gets tripped up by things that the old code handled
> > just fine.  Back when we'd run _begin_fstest as a real bash subroutine
> > to print the group name arguments, a line like this:
> > 
> > _begin_fstest deprecated # log logprint quota
> 
> And I just don't care about that.

I *do*, because my entire testing dashboard turned red overnight because
these changes broke the group list file generation and pulled in tests
that normally get excluded from my nightly test runs.

Did _anyone_ actually compare the group.list output before and after
applying the patch?

$ diff -U 1 -r build-old/tests/ build-x86_64/tests/
diff -U 1 -r build-old/tests/xfs/group.list
build-x86_64/tests/xfs/group.list
--- build-old/tests/xfs/group.list      2022-05-16 15:05:55.655374923 -0700
+++ build-x86_64/tests/xfs/group.list   2022-05-16 15:07:12.562086066 -0700
@@ -3,2 +3,3 @@
 
+/home/djwong/cdev/work/fstests/build-x86_64/tests/xfs/191-input-validation:_begin_fstest
auto quick mkfs realtime broken
 001 db auto quick
@@ -20,3 +21,3 @@
 017 mount auto quick stress
-018 deprecated
+018 deprecated # log logprint v2log
 019 mkfs auto quick
@@ -83,4 +84,4 @@
 080 rw ioctl
-081 deprecated
-082 deprecated
+081 deprecated # log logprint quota
+082 deprecated # log logprint v2log
 083 dangerous_fuzzers punch
@@ -191,3 +192,2 @@
 190 rw auto quick
-191-input-validation auto quick mkfs realtime broken
 192 auto quick clone


> Handling these whacky corner cases is the *wrong solution* - we're
> just creating more technical debt for ourselves going down that
> path.

Frankly, I think this whole change *introduces* technical debt.

Test files used to be bash scripts that have /all/ the standard
behaviors of bash scripts -- commands can have comments at the end of
the line, continuations are supported, etc.  Test authors merely have to
ensure that the script parses correctly, and that the groups for each
test are specified as options to _begin_fstest.

Now you've effectively made it so that there's this one weird line that
looks like any other piece of shell script, but it isn't.  You can't
have comments, you can't use line continuations, and if you
accidentally drop something like a colon in the line, there's a risk
that a regular expression will do the wrong thing and explode.

In other words, test scripts aren't purely bash scripts anymore even
though they sure look like pure bash.  Every author must now be aware
that there's a behavioral quirk pertaining to exactly one required line
in every test script.

> $ git grep _begin_fstest tests/ |grep "#"
> tests/xfs/018:_begin_fstest deprecated # log logprint v2log
> tests/xfs/081:_begin_fstest deprecated # log logprint quota
> tests/xfs/082:_begin_fstest deprecated # log logprint v2log
> $
> 
> There are the only 3 tests marked deprecated. *Nobody runs them* and
> if they did, they all fail on a current kernel:
> 
> ....
> Running: MOUNT_OPTIONS= ./check -R xunit -b -s xfs xfs/018 xfs/081 xfs/082
> ....
> Failures: xfs/018 xfs/081 xfs/082
> Failed 3 of 3 tests
> 
> We don't need to support this whacky corner case - just remove
> the three deprecated tests completely. If anyone needs them, they
> are still in the git history. But the right thing to do is to remove
> the corner case altogether, not add complexity to handle it
> needlessly.

Then why didn't you propose remove them?  I agree that these three are
hopelessly broken and I've never gotten x/191 to pass, and fully support
removing them.

> > Instead, that above output (which I harvested from xfs/081) now
> > becomes:
> > 
> > 081 deprecated # log logprint quota
> > 
> > The first grepsed blobule should do more if it's going to
> > performance-optimize bash:
> 
> We don't need to "performance-optimize bash" - we've already fixed
> the problem by addressing the low hanging fruit (35s down to less
> than 0.4s, and less than 0.1s with make -j8).

That's nice that it's faster to build, but IMHO it's *broken*.  The
original patch was a performance improvement that submarined in a change
in how we have to write tests.

The comments for _begin_fstest don't make any mention at all of the new
requirements for the _begin_fstest callsite.

> Beyond where we iare now is really "don't care" territory because
> nobody is going to notice it going 0.1s faster now. OTOH, we are sure
> as anything going to notice the complexity of the regexes in a
> year's time when we have to work out what it does from first
> principles again....

I already had to do that, and it's barely been 10 days since this was
committed.

> > grep -I -R "^_begin_fstest" -Z $test_dir/ | \
> > 	sed -e 's/#.*$//g' \
> 
> This is the only bit that is needed to handle the commented out
> group case, everythign else is just complexity that comes from
> over-optimisation. And even handling comments is largely unnecessary
> because removing deprecated tests is the right way to fix this...

...until the next person trips over this.

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com