On Mon, Apr 18, 2022 at 08:56:43AM +0300, Amir Goldstein wrote:
> On Mon, Apr 18, 2022 at 8:05 AM Zorro Lang <zlang@xxxxxxxxxx> wrote:
> >
> > On Sun, Apr 17, 2022 at 08:40:23PM +0300, Amir Goldstein wrote:
> > > Some tests are written before a fix is merged upstream and some tests
> > > are written without a known available fix at all. Such is the case with
> > > test overlay/061.
> > >
> > > Introduce a helper _known_issues_on_fs() which can be used to document
> > > this situation and print a hint on failure.
> > > The helper supports specifying specifc fs with the known issue for
> > > generic tests as well as the notation ^FS1 ^FS2 to indicate a known issue
> > > on all filesystems expect for FS1 FS2.
> > >
> > > Setting the variable SKIP_KNOWN_ISSUES=yes, will cause all tests
> > > annotated as known issues for the tested fs to be skipped.
> > >
> > > A future improvement may provide a run option to skip tests based on
> > > _known_issues_before_kernel when running the tests on an older kernel
> > > version.
> > >
> > > Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
> > > ---
> > > README | 2 ++
> > > common/rc | 16 +++++++++++++++-
> > > tests/overlay/061 | 2 ++
> > > 3 files changed, 19 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/README b/README
> > > index 7da66cb6..8abc3840 100644
> > > --- a/README
> > > +++ b/README
> > > @@ -241,6 +241,8 @@ Misc:
> > > this option is supported for all filesystems currently only -overlay is
> > > expected to run without issues. For other filesystems additional patches
> > > and fixes to the test suite might be needed.
> > > + - set SKIP_KNOWN_ISSUES=yes to skip tests for bug without a known fix.
> > > + Those tests are annotated with _known_issue_on_fs helper.
> > >
> > > ______________________
> > > USING THE FSQA SUITE
> > > diff --git a/common/rc b/common/rc
> > > index 2e9dc408..3cf60a7e 100644
> > > --- a/common/rc
> > > +++ b/common/rc
> > > @@ -1597,8 +1597,23 @@ _supported_fs()
> > > _notrun "not suitable for this filesystem type: $FSTYP"
> > > }
> > >
> > > +_known_issue_on_fs()
> > > +{
> > > + # Only "supported" fs have the known issue
> > > + _check_supported_fs $* || return
> > > +
> > > + if [ "$SKIP_KNOWN_ISSUES" = "yes" ]; then
> > > + _notrun "known issue for this filesystem type: $FSTYP"
> > > + fi
> > > +
> > > + echo "HINT: You _MAY_ be hit by a known issue for filesystem type $FSTYP." >> $seqres.hints
> > > + echo >> $seqres.hints
> > > +}
> > > +
> > > _known_issue_before_kernel()
> > > {
> > > + # TODO: don't run if $SKIP_KNOWN_ISSUES and kernel version < $1
> > > +
> >
> > As I said in last patch review, it's only for one kernel base line, it might cause
> > downstream kernel testing can't run a case, even if it had backported the patches.
> >
>
> I agree. This may be useful to some but a bit controversial.
> Anyway, I shall remove this TODO.
>
>
> > > echo "HINT: You _MAY_ be hit by a known issue on kernel version < $1." >> $seqres.hints
> > > echo >> $seqres.hints
> > > }
> > > @@ -4929,7 +4944,6 @@ _require_kernel_config()
> > > _has_kernel_config $1 || _notrun "Installed kernel not built with $1"
> > > }
> > >
> > > -
> > > init_rc
> > >
> > > ################################################################################
> > > diff --git a/tests/overlay/061 b/tests/overlay/061
> > > index b80cf5a0..36be3391 100755
> > > --- a/tests/overlay/061
> > > +++ b/tests/overlay/061
> > > @@ -22,6 +22,8 @@ _begin_fstest posix copyup
> > >
> > > # real QA test starts here
> > > _supported_fs overlay
> > > +_known_issue_on_fs overlay
> >
> > If it's a overlay case, it's absolutely cover an overlay known issue at first.
> > Even if it hit a mm or underlying fs bug, the _known_issue_on_fs is helpless
> > for that.
>
> I disagree.
> There is a subtle difference.
> Most of the tests in fstests are regression tests meaning that they
> cover an issue that is supposed to be fixed in the tested kernel.
>
> In case of overlayfs, I took "test driven development" a bit further and
> wrote fstests to cover non-standard behavior that we want to fix and then
> wrote a development roadmap [*] that took more that 10 kernel releases
> to complete.
> Test overlay/061 is the last of those tests, whose issue was never addressed
> and there is no prospect as to when it is going to be addressed.
>
> This is the reason that previous patch removed all those lines from comments:
> -# This simple test demonstrates a known issue with overlayfs:
>
> [*] https://github.com/amir73il/overlayfs/wiki/Overlayfs-TODO#Compliance
>
> As a tester, how would you know if something is wrong with your kernel
> when a test is failing unless you had an annotation that tells you that the
> test is expected to fail?
>
> SKIP_KNOWN_ISSUES takes this one step further and says don't bother
> me with tests that are expected to fail.
>
> >
> > I can understand above 2 patches, but this patch looks weird to me. We can
> > give a hint to downstream testing (from upstream commit side). But deal with
> > known issues, that's another story. I'd like to let testers from different
> > downstream kernels to deal with their known issues checking by themselves,
> > don't depend on upstream fstests for all.
> >
>
> Not sure I understand your point.
Oh, sorry I didn't explain that clearly. I mean patch [1/3] and [2/3] can be improved
by more talking, but [3/3] is not reasonable for me.
Let's see what's the purpose of _known_issue_on_fs():
1) If it trys to work for upstream mainline:
As a hint, it can be replaced by known commit output [2/3]. As a forcibly "_notrun" to a fs,
it can be replaced by _supported_fs ^$someonefs with a proper comment [1/3]. As a test driven
tool... no, I don't like to make xfstests to be place which developers used to record/update/
remind their "planning to do/fix", they can record them in other place :)
2) If it trys to help downstream testing:
It's really helpless for downstream testing, except downstream testers maintain their
own xfstests, and change `_known_issue_on_fs xxx` for themselves. But as a downstream
tester, I'd like to maintain our known issues (known failures and skip list) by
ourselves in another place, not in xfstests inside.
Anyway, let's see reviewpoint from others too :)
Thanks,
Zorro
>
> The intention of this helper was to address the debate over two of Darrick's
> new tests. I mentioned them in the cover letter but forgot to mention here:
>
> [1] https://lore.kernel.org/fstests/20220412172853.GG16799@magnolia/
> [2] https://lore.kernel.org/fstests/20220414155007.GC17014@magnolia/
>
> There is a difference between:
> _supported_fs ext4 xfs btrfs
>
> And:
>
> _supported_fs generic
> # https://lore.kernel.org/fstests/20220414155007.GC17014@magnolia/
> _known_issue_on_fs ^ext4 ^xfs ^btrfs
>
> The main difference is that the test WILL run and fail on all other fs
> and both me and Eryu indicated that should happen.
>
> The difference from only using _supported_fs generic
> is that giving more information causes less surprises and less confusion
> to other fs developers that get a surprise new failure when pulling fstests
> upstream updates.
>
> In the end, that what this series is all about - being more considerate
> of other testers and making the information known to test author known
> to a much wider audience that does not need to dig in test comments
> and long mailing list discussions or overlayfs roadmap wikis to find that
> information.
>
> Thanks,
> Amir.
>
