Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Leonard Crestez <cdleonard@gmail.com>
Subject: Re: [PATCH 08/11] selftests: net/fcnal: Replace sleep after server
 start with -k
To:     David Ahern <dsahern@gmail.com>, Shuah Khan <shuah@kernel.org>
Cc:     "David S. Miller" <davem@davemloft.net>,
        Ido Schimmel <idosch@nvidia.com>,
        Seth David Schoen <schoen@loyalty.org>,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        Jakub Kicinski <kuba@kernel.org>,
        David Ahern <dsahern@kernel.org>,
        linux-kselftest@vger.kernel.org
References: <cover.1633520807.git.cdleonard@gmail.com>
 <ec40bd7128a30e93b90ba888f3468f394617a010.1633520807.git.cdleonard@gmail.com>
 <43210038-b04b-3726-1355-d5f132f6c64e@gmail.com>
 <d6882c3f-4ecf-4b4e-c20e-09b88da4fbd6@gmail.com>
 <888962dc-8d55-4875-cf44-c0b8ebaa1978@gmail.com>
Message-ID: <dfe10fec-b2d8-16c0-ae20-6839f47b2809@gmail.com>
Date:   Wed, 10 Nov 2021 15:54:56 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.13.0
MIME-Version: 1.0
In-Reply-To: <888962dc-8d55-4875-cf44-c0b8ebaa1978@gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 10/7/21 4:22 AM, David Ahern wrote:
> On 10/6/21 3:35 PM, Leonard Crestez wrote:
>>
>> I counted the [FAIL] or [ OK ] markers but not the output of nettest
>> itself. I don't know what to look for, I guess I could diff the outputs?
>>
>> Shouldn't it be sufficient to compare the exit codes of the nettest client?
> 
> mistakes happen. The 700+ tests that exist were verified by me when I
> submitted the script - that each test passes when it should and fails
> when it should. "FAIL" has many reasons. I tried to have separate exit
> codes for nettest.c to capture the timeouts vs ECONNREFUSED, etc., but I
> could easily have made a mistake. scanning the output is the best way.
> Most of the 'supposed to fail' tests have a HINT saying why it should fail.

It is not good to have a test for which correctness is ambiguous to such 
an extent, it makes reliable future changes difficult. In theory an 
uniform TAP format is supposed to solve this but it is not applied 
inside selftests/net.

I attempted to write a script to compare two logs in their current 
format: https://gitlab.com/cdleonard/kselftest-parse-nettest-fcnal

It does a bunch of nasty scrubbing of irrelevant behavior and got it to 
the point where no diffs are found between repeated runs on the same 
machine.

One nasty issue was that many tests kill processes inside log_test so 
relevant output may be shown either before or after the "TEST: " result 
line. This was solved by associating output until the next ### with 
previous test.

>> The output is also modified by a previous change to not capture server
>> output separately and instead let it be combined with that of the
>> client. That change is required for this one, doing out=$(nettest -k)
>> does not return on fork unless the pipe is also closed.
>>
>> I did not look at your change, mine is relatively minimal because it
>> only changes who decide when the server goes into the background: the
>> shell script or the server itself. This makes it work very easily even
>> for tests with multiple server instances.
> 
> The logging issue is why I went with 1 binary do both server and client
> after nettest.c got support for changing namespaces.

It's possible to just compare the "client" and "server" logs separately 
by sorting them on their prefix.

I think a decent approach would be to do a bulk replace for all 
"run_cmd{,_nsb,_nsc} nettest" with a new "run_nettest" function that 
passes all arguments to nettest itself. That run_nettest function could 
include a leading common "-t" arg that is parsed at the top of 
fcnal-test.sh.

--
Regards,
Leonard