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

Zorro Lang
2022-05-18T02:23:30Z

On Mon, May 16, 2022 at 03:26:04PM -0700, Darrick J. Wong wrote:
> 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

Sorry, I'll try to leave more time to you next time, if a patch touches the code
related with you.

> > > 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.

Sorry Darrick, my general regression test[1] didn't cover your usage, cause
I didn't hit this "broken" error... Actually after talking with you last week,
I'm planning to change the ways I use fstests, especially for those groups
not belong to "auto". That might help to find issues like this in time.

Furthermore, I don't know why no one found this regression either in the past
one week after for-next branch been updated. I'm thinking about extend the
interval between for-next with master update, change from 1 week to 2 weeks.
As 1 week "soak period" looks like a little short.

Feel free to ping me, if you have more suggestions.

Thanks,
Zorro

[1]
./check -g auto
./check -g all -x auto

>
> 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
>