Message-ID: <20170918201905.qtax6kn6iscrebwa@destiny>
Date:   Mon, 18 Sep 2017 16:19:06 -0400
From:   Josef Bacik <josef@...icpanda.com>
To:     Shuah Khan <shuahkh@....samsung.com>
Cc:     Josef Bacik <josef@...icpanda.com>, Shuah Khan <shuah@...nel.org>,
        Josef Bacik <jbacik@...com>,
        "David S. Miller" <davem@...emloft.net>,
        linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org,
        netdev@...r.kernel.org
Subject: Re: [PATCH 3/3] selftests: silence test output by default

On Mon, Sep 18, 2017 at 01:48:31PM -0600, Shuah Khan wrote:
> On 09/18/2017 12:24 PM, Josef Bacik wrote:
> > On Mon, Sep 18, 2017 at 12:13:40PM -0600, Shuah Khan wrote:
> >> On 09/18/2017 11:52 AM, Josef Bacik wrote:
> >>> On Mon, Sep 18, 2017 at 11:46:18AM -0600, Shuah Khan wrote:
> >>>> On 09/18/2017 11:37 AM, josef@...icpanda.com wrote:
> >>>>> From: Josef Bacik <jbacik@...com>
> >>>>>
> >>>>> Some of the networking tests are very noisy and make it impossible to
> >>>>> see if we actually passed the tests as they run.  Default to suppressing
> >>>>> the output from any tests run in order to make it easier to track what
> >>>>> failed.
> >>>>>
> >>>>> Signed-off-by: Josef Bacik <jbacik@...com>
> >>>>> --
> >>>>
> >>>> This change suppresses pass/fail wrapper output for all tests, not just the
> >>>> networking tests.
> >>>>
> >>>> Could you please send me before and after results for what you are trying
> >>>> to fix.
> >>>>
> >>>
> >>> Yeah I wanted to suppress extraneous output from everybody, I just happened to
> >>> notice it because I was testing net.  The default thing already spits out what
> >>> it's running and pass/fail, there's no need to include all of the random output
> >>> unless the user wants to go and run the test manually.  As it is now it's
> >>> _impossible_ to tell what ran and what passed/failed because of all the random
> >>> output.
> >>
> >> Unfortunately kselftests have lots of users that want different things. A recent
> >> request is to use TAP13 format for output for external parsers to be able to parse.
> >> That is what this change to add TAP13 header does.
> >>
> >> The output you are seeing is the TAP 13 format to indicate the test has passed.
> >>
> >> The right fix would be to suppress the Pass/Fail from the individual shell script
> >> and have the shell script exit with error code. kselftest lib.mk will handle the
> >> error code and print out pass/fail like it is doing now.
> >>
> >> Using the common logic will help avoid duplicate code in tests/test scripts and
> >> also makes the pass/fail messages consistent.
> >>
> >> In the following output the individual test output can be eliminated since lib.mk
> >> run_tests does that for you. In addition, you will also get a count of tests at
> >> the end of the run of all tests in a test directory.
> >>
> >> TAP version 13
> >> selftests: run_netsocktests
> >> ========================================
> >> --------------------
> >> running socket test
> >> --------------------
> >> [PASS]
> >> ok 1..1 selftests: run_netsocktests [PASS]
> >> selftests: run_afpackettests
> >> ========================================
> >> must be run as root
> >> ok 1..2 selftests: run_afpackettests [PASS]
> >> selftests: test_bpf.sh
> >> ========================================
> >> test_bpf: [FAIL]
> >> not ok 1..3 selftests:  test_bpf.sh [FAIL]
> >> selftests: netdevice.sh
> >> ========================================
> >> SKIP: Need root privileges
> >> ok 1..4 selftests: netdevice.sh [PASS]
> >>
> >> If you eliminate that you will just see the common lib.mk results.
> >>
> >> TAP version 13
> >> selftests: run_netsocktests
> >> ========================================
> >> ok 1..1 selftests: run_netsocktests [PASS]
> >> selftests: run_afpackettests
> >> ========================================
> >> must be run as root
> >> ok 1..2 selftests: run_afpackettests [PASS]
> >> ========================================
> >> selftests: test_bpf.sh
> >> ========================================
> >> not ok 1..3 selftests:  test_bpf.sh [FAIL]
> >> selftests: netdevice.sh
> >> ========================================
> >> SKIP: Need root privileges
> >> ok 1..4 selftests: netdevice.sh [PASS]
> >>
> >>
> >> If you would like to fix the duplicate output, please send me patches
> >> to remove pass/fail output strings from tests instead. It is on my
> >> todo to do that this release.
> >>
> > 
> > I'm confused, this is exactly what my patch does, it strips all of the
> > extraneous output and leaves only the TAP13 output.  Here is the output without
> > my suppression patch
> > 
> > https://da.gd/pup0
> > 
> > and here is the output with my suppression patch
> > 
> > https://da.gd/3olKj
> > 
> > Unless I'm missing something subtle it appears to be exactly the output you
> > want, without the random crap from the other tests.  The only thing I'm
> > redirecting is the output of the _test_ itself, $$BASENAME_TEST from what I can
> > tell is the actual test we're running, not the wrapper, so everything is as it
> > should be.  Thanks,
> > 
> > Josef
> > 
> 
> Yes you are right. Yes I see that you are redirecting all
> output from the test with (./$$BASENAME_TEST > /dev/null 2>&1)
> My bad.
> 
> cd `dirname $$TEST` > /dev/null; (./$$BASENAME_TEST > /dev/null 2>&1 && echo "ok 1..$$test_num selftests: $$BASENAME_TEST [PASS]") || echo "not ok 1..$$test_num selftests:  $$BASENAME_TEST [FAIL]"; cd - > /dev/null;\
> 
> However, it also suppresses important error messages from individual tests
> like in the case of rtnetlink.sh from your before and after output.
> 
> User knows the test failed, but doesn't know why. One way to solve
> the problem is creating a temp file with the output for reference.
> Something like:
> 
> (./$$BASENAME_TEST > /tmp/kselftest.out 2>&1
> 
> 

So I didn't do a global file because we'd have to append to it to make sure we
didn't lose any history.  If this doesn't bother you I can do that instead, and
then just remove the file whenever we start running things.  Let me know if
that's what you would prefer.  Thanks,

Josef

Powered by blists - more mailing lists