From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on gnuweeb.org X-Spam-Level: X-Spam-Status: No, score=-0.0 required=5.0 tests=SPF_HELO_PASS,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 Received: from 1wt.eu (wtarreau.pck.nerim.net [62.212.114.60]) by gnuweeb.org (Postfix) with ESMTP id D414D7E312 for ; Wed, 20 Jul 2022 16:21:09 +0000 (UTC) Received: (from willy@localhost) by pcw.home.local (8.15.2/8.15.2/Submit) id 26KGKsKX004192; Wed, 20 Jul 2022 18:20:54 +0200 Date: Wed, 20 Jul 2022 18:20:54 +0200 From: Willy Tarreau To: Ammar Faizi Cc: "Paul E. McKenney" , Pranith Kumar , Alviro Iskandar Setiawan , David Laight , Mark Brown , Linus Torvalds , Shuah Khan , "Fernanda Ma'rouf" , Linux Kselftest Mailing List , Linux Kernel Mailing List , GNU/Weeb Mailing List Subject: Re: [PATCH 00/17] nolibc: add preliminary self tests Message-ID: <20220720162054.GB4159@1wt.eu> References: <20220719214449.2520-1-w@1wt.eu> <67a92005-4e13-909a-1693-dfb86d8114c0@gnuweeb.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <67a92005-4e13-909a-1693-dfb86d8114c0@gnuweeb.org> User-Agent: Mutt/1.10.1 (2018-07-13) List-Id: On Wed, Jul 20, 2022 at 11:03:58PM +0700, Ammar Faizi wrote: > On 7/20/22 4:44 AM, Willy Tarreau wrote: > > I'm obviously interested in comments, but really, I don't want to > > overdesign something for a first step, it remains a very modest test > > program and I'd like that it remains easy to hack on it and to contribute > > new tests that are deemed useful. > > I personally hate how the test framework mandates: > > "There must be exactly one test per line." I know, that's a design choice that makes them trivial to add, because it's the compiler that assigns the test IDs, and it comes with a non negligible benefit. > which makes the test case, for example, one long liner like this: > > if ((p1 = p2 = sbrk(4096)) != (void *)-1) p2 = sbrk(-4096); EXPECT_SYSZR(1, (p2 == (void *)-1) || p2 == p1); break; > > that's ugly and hard to read. Can we get rid of this "one test per line" rule? If you find a better solution, I'm open. What I certainly don't want to do is to have to cross-reference IDs with arrays, nor start to stack endless if/else that are even more painful to deal with, or have to renumber everything by hand once in a while. > It would be great if we followed the documented coding style that says: > > "Statements longer than 80 columns should be broken into sensible chunks, > unless exceeding 80 columns significantly increases readability and does > not hide information." [1] Admittedly this is not core code but debugging code running in userland to help developers spot bugs in their code which is somewhere else and well maintained. I personally think that the tradeoff is positive here, i.e. non-pretty but easily hackable short tests that encourage additions and variations. The ease of adding tests allowed me to create 71 of them in a single afternoon and two of them brought me bugs in existing code, which I think is efficient. But I'm not fond of the approach either, I just couldn't produce anything as efficient that was prettier, but I'm quite open to being proven wrong by an alternate proposal. > What we have here doesn't really increase the readability at all. Maybe > it's too late for 5.20, just for next in case we want to fix it. The goal was not to increase *readability* but *writability*. We're still missing test for most syscalls and I would like them to be added quickly so that we can continue to add tested code. The readability I care about is understanding the output. When I'm seeing: ... 29 execve_root = -1 EACCES [OK] 30 getdents64_root = -1 EBADF [FAIL] 31 getdents64_null = -1 EBADF != (-1 ENOTDIR) [FAIL] 32 gettimeofday_null = 0 [OK] ... on riscv64, I don't have to search long to figure that we did something wrong with getdents64() on this arch and that the error path works differently. Similarly, this on mips: 8 kill_CONT = 0 [OK] 9 kill_BADPID = -1 ESRCH [OK] 10 sbrkdo_page_fault(): sending SIGSEGV to init for invalid read access from 0000000a epc = 0000000a in init[400000+4000] ra = 0000000a in init[400000+4000] Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b tells me that sbrk() definitely doesn't work there. In both cases I know what and where to look without even having to *read* that test. This does matter to me, as a developer of the component subject to the test. But again, I'm open to better proposals. I reached the limits of my imagination there, but I do value the ability to "yyp" one line, change two arguments and gain one extra test for a different combination, and I really do not want to lose that simplicity. Note that for more complex tests, it's trivial to add a dedicated function and that's what was done for getdents64() which also serves as an example. Willy