On Fri, Apr 10, 2020 at 08:25:28AM +0800, Qu Wenruo wrote:
>
>
> On 2020/4/9 下午11:52, David Sterba wrote:
> > On Tue, Apr 07, 2020 at 03:18:44PM +0800, Qu Wenruo wrote:
> >> Since run_check_stdout() can insert INSTRUMENT for all btrfs related
> >> programs, which could easily pollute the stdout, any caller of
> >> run_check_stdout() should do proper filter.
> >>
> >> The following callers are affected:
> >> - misc/004
> >> Filter the output of "btrfs ins min-dev-size"
> >>
> >> - misc/009
> >> - misc/013
> >> - misc/024
> >> They are all calling "btrfs ins rootid", so introduce get_subvolid()
> >> function to grab the subvolid properly.
> >>
> >> - misc/031
> >> Loose the filter for "btrfs qgroup show". No need for "tail -n 1".
> >>
> >> So we still have the same coverage, but now these tests won't cause
> >> false alert if we insert INSTRUMENT for all btrfs commands.
> >>
> >> Signed-off-by: Qu Wenruo <wqu@xxxxxxxx>
> >> ---
> >> tests/common | 13 ++++++++++++
> >> tests/misc-tests/004-shrink-fs/test.sh | 11 ++++++++--
> >> .../009-subvolume-sync-must-wait/test.sh | 2 +-
> >> .../013-subvolume-sync-crash/test.sh | 2 +-
> >> .../024-inspect-internal-rootid/test.sh | 21 +++++++------------
> >> .../031-qgroup-parent-child-relation/test.sh | 4 ++--
> >> 6 files changed, 33 insertions(+), 20 deletions(-)
> >>
> >> diff --git a/tests/common b/tests/common
> >> index 71661e950db0..f8fc3cfd8b4f 100644
> >> --- a/tests/common
> >> +++ b/tests/common
> >> @@ -169,6 +169,9 @@ run_check()
> >>
> >> # same as run_check but the stderr+stdout output is duplicated on stdout and
> >> # can be processed further
> >> +#
> >> +# NOTE: If this function is called on btrfs related command, caller should
> >> +# filter the output, as INSTRUMENT can easily pollute the output.
> >> run_check_stdout()
> >> {
> >> local spec
> >> @@ -636,6 +639,16 @@ check_min_kernel_version()
> >> return 0
> >> }
> >>
> >> +# Get subvolume id for specified path
> >> +get_subvolid()
> >> +{
> >> + # run_check_stdout may have INSTRUMENT pollating the output,
> >> + # need to filter the output.
> >> + run_check_stdout "$TOP/btrfs" inspect-internal rootid "$1" | \
> >> + grep -e "^[[:digit:]]\+$"
> >
> > This does not seem much better, now it's specific to the commands and
> > calling the commands directly in new tests will make it fail.
> >
> > If we find another command where the extra output must be filtered,
> > another helper. The instrument-tool specific filtering is IMHO fixed in
> > one place and future proof.
>
> I get your point and concern, and kinda support your point.
>
> >
> > I want to avoid adding yet another test coding style exception like "for
> > inspect-internal you must use this helper", we have already enough like
> > new tests not using the mount/umount helpers or opencoding other
> > existing helpers.
>
> However the problem only happens for command with one line output.
> Like the min-dev-size and rootid of inspect group.
>
> All other common tools will need filtering anyway, like qgroup show or
> dump-tree.
>
> So just several exception would still be acceptable.
> >
> > My idea is to let people write tests in a natural way and adapt the
> > instrumentation tools as we know what problems they could cause.
> >
> I used to support valgrind specific options to solve the problem, until
> knowing you're using a simple wrapper to test.
>
> Yes, we're only using valgrind anyway, but I'm not so sure for other
> guys, maybe one day some new tool developer would use their own fancy
> tool to check btrfs-progs.
>
> And if they find that we're using valgrind specific option just to make
> all tests pass, and their tool fails due to that reason, they may just
> exclude btrfs-progs and express their disappointment against btrfs.
>
> Considering the exception is really not that many, the trade at least
> looks good enough to me.
Ok, I understand, let's do it your way.
