Message-ID: <e0286045-62fd-50aa-0f39-f9c816eebea2@osg.samsung.com>
Date:   Mon, 18 Sep 2017 15:36:40 -0600
From:   Shuah Khan <shuahkh@....samsung.com>
To:     Josef Bacik <josef@...icpanda.com>
Cc:     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, Shuah Khan <shuahkh@....samsung.com>
Subject: Re: [PATCH 3/3] selftests: silence test output by default

On 09/18/2017 02:19 PM, Josef Bacik wrote:
> 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
> 

Right. Let's not worry about global file. Let's just use /tmp/$$BASENAME_TEST
for now. Something like the below diff:


diff --git a/tools/testing/selftests/lib.mk b/tools/testing/selftests/lib.mk
index 223234cd98e9..317393e430c9 100644
--- a/tools/testing/selftests/lib.mk
+++ b/tools/testing/selftests/lib.mk
@@ -24,7 +24,8 @@ define RUN_TESTS
                        echo "selftests: Warning: file $$BASENAME_TEST is not executable, correct this.";\
                        echo "not ok 1..$$test_num selftests: $$BASENAME_TEST [FAIL]"; \
                else                                    \
-                       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;\
+                       echo "Please check /tmp/$$BASENAME_TEST for full test output";                                                  \
+                       cd `dirname $$TEST` > /dev/null; (./$$BASENAME_TEST > /tmp/$$BASENAME_TEST && echo "ok 1..$$test_num selftests: $$BASENAME_TEST [PASS]") || echo "not ok 1..$$test_num selftests:  $$BASENAME_TEST [FAIL]"; cd - > /dev/null;\
                fi;                                     \
        done;
 endef



thanks,
-- Shuah

Powered by blists - more mailing lists