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, jo...@toxicpanda.com wrote:
>>>>>>> From: Josef Bacik <jba...@fb.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 <jba...@fb.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
