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