Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751341AbdIRVgr (ORCPT ); Mon, 18 Sep 2017 17:36:47 -0400 Received: from ec2-52-27-115-49.us-west-2.compute.amazonaws.com ([52.27.115.49]:59914 "EHLO osg.samsung.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1750781AbdIRVgq (ORCPT ); Mon, 18 Sep 2017 17:36:46 -0400 Subject: Re: [PATCH 3/3] selftests: silence test output by default To: Josef Bacik Cc: Shuah Khan , Josef Bacik , "David S. Miller" , linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org, netdev@vger.kernel.org, Shuah Khan References: <1505755982-7855-1-git-send-email-jbacik@fb.com> <1505756224-8187-1-git-send-email-jbacik@fb.com> <20170918175254.lsdzqvjm4uvix4rj@destiny> <20170918182426.cs4wxy3igfx7zyvb@destiny> <1f756821-2737-8982-90d6-b4933e97a3ef@osg.samsung.com> <20170918201905.qtax6kn6iscrebwa@destiny> From: Shuah Khan Message-ID: Date: Mon, 18 Sep 2017 15:36:40 -0600 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: <20170918201905.qtax6kn6iscrebwa@destiny> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6988 Lines: 161 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@toxicpanda.com wrote: >>>>>>> From: Josef Bacik >>>>>>> >>>>>>> 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 >>>>>>> -- >>>>>> >>>>>> 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