Re: [hackers] [sbase][PATCH v2] Add tests for some utilities
On Wed, Aug 01, 2018 at 10:12:54PM +0200, Mattias Andrée wrote: > On Wed, 1 Aug 2018 22:07:33 +0200 > Mattias Andrée wrote: > > [...] > > > On Wed, 1 Aug 2018 21:16:26 +0200 > > Silvan Jegen wrote: > > > > > On Wed, Aug 01, 2018 at 07:53:18PM +0200, Mattias Andrée wrote: > > > > Thank you for your time! > > > > > > Would you be open for working on some (portable) shell code for the most > > > common tools? > > > > I am open to it, however I fear that it is not the simpler solution > > as it may require two implementations of the same functionality. > > Sorry, I'm mixing things up, the idea is to completely replace > the C version of test-common, not just add another implementation. That is the plan but the C code that has to be written for the tools with "special" testing requirements will probably have to check some stdout/stderr output too. Not having too much duplication between the C and the shell code would be nice... > > Have a nice evening, You too! Cheers, Silvan
Re: [hackers] [sbase][PATCH v2] Add tests for some utilities
On Wed, Aug 01, 2018 at 10:07:33PM +0200, Mattias Andrée wrote: > On Wed, 1 Aug 2018 21:16:26 +0200 > Silvan Jegen wrote: > > [...] > > > On Wed, Aug 01, 2018 at 07:53:18PM +0200, Mattias Andrée wrote: > > > Thank you for your time! > > > * uname: > > > Most of uname can be tested in ed, however, the output > > s/ed/sh/ # I guess you understood that, but I cannot stand not correcting it. I think there was just a lifted eyebrow that is all... > > > > > > [...] > > > > > > As I see it, the most complex parts of the C code are: > > > > > > * start_process: > > > It's probably enough to split out some code to > > > separate functions: the `if (in->flags & DATA)`, > > > the dup2–close loops at the end. > > > > > > * wait_process: > > > Instead of ready all file descriptors as fast as > > > possible, the they could probably be read in order. > > > > > > * check_output_test: > > > It's probably enough to add a few short comments > > > and improve variable names. > > > > > > * print_failure: > > > It's probably enough to add some empty lines add a > > > few short comments. > > > > > > The other parts are pretty straight forward. > > > > If we go with option 1) I would like to wait to see which C functionality > > we would end up needing in the end. Looking at the C code I would postpone > > until after that decision has been made. > > I will make a sh reimplementation, but since I'm back at work > now, it will take some time. In the meanwhile, why not enjoy > my new painting. Haha, the painting is a thing of beauty! It taking time is not an issue. The earlier you send any code, the faster everyone will be able to give feedback though! Cheers, Silvan signature.asc Description: PGP signature
Re: [hackers] [sbase][PATCH v2] Add tests for some utilities
On Wed, 1 Aug 2018 22:07:33 +0200 Mattias Andrée wrote: > On Wed, 1 Aug 2018 21:16:26 +0200 > Silvan Jegen wrote: > > > On Wed, Aug 01, 2018 at 07:53:18PM +0200, Mattias Andrée wrote: > > > Thank you for your time! > > > > Thank you for all your work! :P > > Hi again Silvan, > > > > > > > > The common code is 590 lines of code, including: > > > > > > * 102 lines of code related to identifying the error when the > > > test fails. > > > > > > * 14 lines of code for properly killing processes on failure, > > > abortion, and when a test case hangs. > > > > > > * 32 lines of code, plus 13 of the lines counted above, > > > for supporting concurrent tests. > > > > > > This leaves 442 lines for the fundamental stuff and a few > > > lines to support printing all errors rather than just the > > > first error. > > > > > > Some note on your sh code (since you wrote “crappy and > > > probably non-portable”, you are probably aware of this, > > > but just in case): > > > > > > * `source` is a Bash synonym for the portable `.` command. > > > > Yeah, that sounds familiar. > > > > > > > * `echo` is unportable and `printf` should be used instead. > > > > Didn't know that echo was not portable. Thought it was just a builtin > > that should work the same everywhere. It's probably the flags that are > > the issue... > > It's worse than that, nothing about echo is portable: > > - You don't know how backslashes are interpreted. > > - You don't know whether you get a new line at the end. > > - You don't know if -e is supported. > > - You don't know if -n is supported. > > > > > > > > * I have never seen `&>` before, so I my guess is that it > > > is a Bashism. > > > > Yeah, seems likely. It's supposed to redirect both stderr and stdout. "sh" > > did not complain about it but that doesn't mean much... > > > > > > > * It looks like whichever file descriptor is not being > > > inspected by `check_output` is printed the inherited > > > file descriptor rather than to /dev/null. > > > > Printing behaviour of the tests should looked at and fixed for sure. > > > > > > > * I think it would be better to have something like: > > > > > > set -e > > > > > > run "test name" "./dirname //" > > > check_stderr "" > > > check_stdout / || check_stdout // > > > check_exit 0 > > > > > > Your sh code, with check_exit added, covers most current tests. > > > However, it every time the usage output is modified it has to > > > be change in the test case, which I guess is acceptable but > > > undesirable. The tests that are currently implemented > > > > I think that is working as intended. It's a behaviour change in the code > > and should result in an error (unless we decide that we don't want to > > test the usage output of course). > > > > > > > and which need to be handled specially are: > > > > > > * sleep: > > > This can be done with sh. With some adaption to the sh > > > code, tests can also be done in parallel as it is in > > > the C code. > > > > > > * test: > > > test takes a lot of time to test, which is why multiple > > > tests are run in parallel in the C code. Like tty(1), > > > this test also requires the creation of terminals, but > > > it also requires the creation of sockets. > > > > > > * time: > > > Requires setitimer(3) and pause(3). > > > > > > * tty: > > > This test requires the creation of terminals. > > > > > > * uname: > > > Most of uname can be tested in ed, however, the output > > s/ed/sh/ # I guess you understood that, but I cannot stand not correcting it. > > > > of uname with only one flag applied requires the uname > > > system call. > > > > > > * whoami: > > > The user name can be retrieved via $LOGNAME according > > > to POSIX, however this requires that your login program > > > actually sets it. Additionally (and this should be added > > > to the test) when whoami is called from a program with > > > setuid the owner of the program should be printed (i.e. > > > the effective user), not real user which is stored in > > > $LOGNAME. > > > > > > Additionally env, time, and yes requires identifying which signal > > > the processed was killed by. I have never done this in sh, but I > > > understand that it should be doable. time and env also requires a > > > way to kill the process it runs with a specific signal. Furthermore > > > the test for tty should include a case where stdin does not exist, > > > which for the moment is not done. All tests excepts test and sleep > > > fundamentally requires support for stdout, but they also use stderr > > > for checking error support. > > > > > > These tests require all the features in the C code, except the > > > extra functionally enumerated at the beginning of this mail and > > > the support for binary data. These are tests we should focus on, > > > whichever solution is found for them should be applied to the > > > rest
Re: [hackers] [sbase][PATCH v2] Add tests for some utilities
On Wed, Aug 01, 2018 at 07:53:18PM +0200, Mattias Andrée wrote: > Thank you for your time! Thank you for all your work! :P > The common code is 590 lines of code, including: > > * 102 lines of code related to identifying the error when the > test fails. > > * 14 lines of code for properly killing processes on failure, > abortion, and when a test case hangs. > > * 32 lines of code, plus 13 of the lines counted above, > for supporting concurrent tests. > > This leaves 442 lines for the fundamental stuff and a few > lines to support printing all errors rather than just the > first error. > > Some note on your sh code (since you wrote “crappy and > probably non-portable”, you are probably aware of this, > but just in case): > > * `source` is a Bash synonym for the portable `.` command. Yeah, that sounds familiar. > * `echo` is unportable and `printf` should be used instead. Didn't know that echo was not portable. Thought it was just a builtin that should work the same everywhere. It's probably the flags that are the issue... > * I have never seen `&>` before, so I my guess is that it > is a Bashism. Yeah, seems likely. It's supposed to redirect both stderr and stdout. "sh" did not complain about it but that doesn't mean much... > * It looks like whichever file descriptor is not being > inspected by `check_output` is printed the inherited > file descriptor rather than to /dev/null. Printing behaviour of the tests should looked at and fixed for sure. > * I think it would be better to have something like: > > set -e > > run "test name" "./dirname //" > check_stderr "" > check_stdout / || check_stdout // > check_exit 0 > > Your sh code, with check_exit added, covers most current tests. > However, it every time the usage output is modified it has to > be change in the test case, which I guess is acceptable but > undesirable. The tests that are currently implemented I think that is working as intended. It's a behaviour change in the code and should result in an error (unless we decide that we don't want to test the usage output of course). > and which need to be handled specially are: > > * sleep: > This can be done with sh. With some adaption to the sh > code, tests can also be done in parallel as it is in > the C code. > > * test: > test takes a lot of time to test, which is why multiple > tests are run in parallel in the C code. Like tty(1), > this test also requires the creation of terminals, but > it also requires the creation of sockets. > > * time: > Requires setitimer(3) and pause(3). > > * tty: > This test requires the creation of terminals. > > * uname: > Most of uname can be tested in ed, however, the output > of uname with only one flag applied requires the uname > system call. > > * whoami: > The user name can be retrieved via $LOGNAME according > to POSIX, however this requires that your login program > actually sets it. Additionally (and this should be added > to the test) when whoami is called from a program with > setuid the owner of the program should be printed (i.e. > the effective user), not real user which is stored in > $LOGNAME. > > Additionally env, time, and yes requires identifying which signal > the processed was killed by. I have never done this in sh, but I > understand that it should be doable. time and env also requires a > way to kill the process it runs with a specific signal. Furthermore > the test for tty should include a case where stdin does not exist, > which for the moment is not done. All tests excepts test and sleep > fundamentally requires support for stdout, but they also use stderr > for checking error support. > > These tests require all the features in the C code, except the > extra functionally enumerated at the beginning of this mail and > the support for binary data. These are tests we should focus on, > whichever solution is found for them should be applied to the > rest tests since those require almost only subset of the > functionality. The only extra functionality other tests require Thanks for compiling the list of tools to be handled in a special way. I do feel that it would be better to find the simplest solution that works for most of the tests. For the rest we can then decide how to handle them in a minimal way (most likely some custom C code for each one. Common parts should still be shared of course). > is support for binary data; which I really don't think warrant > a separate solution. > > The following utilities (from both sbase and ubase) still need > tests, as well as the utilities listed under ubase's TODO. I > have most of the work already done for the utilities marked > with an asterisk. > > at awk bc cal > cat chgrp chmod chown > chroot chvtcksum
Re: [hackers] [sbase][PATCH v2] Add tests for some utilities
Hi Silvan, Thank you for your time! The common code is 590 lines of code, including: * 102 lines of code related to identifying the error when the test fails. * 14 lines of code for properly killing processes on failure, abortion, and when a test case hangs. * 32 lines of code, plus 13 of the lines counted above, for supporting concurrent tests. This leaves 442 lines for the fundamental stuff and a few lines to support printing all errors rather than just the first error. Some note on your sh code (since you wrote “crappy and probably non-portable”, you are probably aware of this, but just in case): * `source` is a Bash synonym for the portable `.` command. * `echo` is unportable and `printf` should be used instead. * I have never seen `&>` before, so I my guess is that it is a Bashism. * It looks like whichever file descriptor is not being inspected by `check_output` is printed the inherited file descriptor rather than to /dev/null. * I think it would be better to have something like: set -e run "test name" "./dirname //" check_stderr "" check_stdout / || check_stdout // check_exit 0 Your sh code, with check_exit added, covers most current tests. However, it every time the usage output is modified it has to be change in the test case, which I guess is acceptable but undesirable. The tests that are currently implemented and which need to be handled specially are: * sleep: This can be done with sh. With some adaption to the sh code, tests can also be done in parallel as it is in the C code. * test: test takes a lot of time to test, which is why multiple tests are run in parallel in the C code. Like tty(1), this test also requires the creation of terminals, but it also requires the creation of sockets. * time: Requires setitimer(3) and pause(3). * tty: This test requires the creation of terminals. * uname: Most of uname can be tested in ed, however, the output of uname with only one flag applied requires the uname system call. * whoami: The user name can be retrieved via $LOGNAME according to POSIX, however this requires that your login program actually sets it. Additionally (and this should be added to the test) when whoami is called from a program with setuid the owner of the program should be printed (i.e. the effective user), not real user which is stored in $LOGNAME. Additionally env, time, and yes requires identifying which signal the processed was killed by. I have never done this in sh, but I understand that it should be doable. time and env also requires a way to kill the process it runs with a specific signal. Furthermore the test for tty should include a case where stdin does not exist, which for the moment is not done. All tests excepts test and sleep fundamentally requires support for stdout, but they also use stderr for checking error support. These tests require all the features in the C code, except the extra functionally enumerated at the beginning of this mail and the support for binary data. These are tests we should focus on, whichever solution is found for them should be applied to the rest tests since those require almost only subset of the functionality. The only extra functionality other tests require is support for binary data; which I really don't think warrant a separate solution. The following utilities (from both sbase and ubase) still need tests, as well as the utilities listed under ubase's TODO. I have most of the work already done for the utilities marked with an asterisk. at awk bc cal cat chgrp chmod chown chroot chvtcksum* clear cmp colscommcp cronctrlaltdel cut date dd df diffdmesg du ed eject env exprfallocate findflock foldfreefreeramdisk fsfreeze getconf getty grephalt headhostnamehwclock id insmod install joinkill killall5lastlastlog ln logger login logname ls lsmod lsusb md5sum* mesg mkdir mkfifo mknod mkswap mktemp mount mountpoint mv nicenl nohup nologin* od pagesizepasswd paste patch* pathchk pidof pivot_root printf ps pwd pwdx reada
Re: [hackers] [sbase][PATCH v2] Add tests for some utilities
Hi Mattias!
On Wed, Jul 11, 2018 at 09:39:23PM +0200, Mattias Andrée wrote:
> The following utilities are tested:
> - basename(1)
> - dirname(1)
> - echo(1)
> - false(1)
> - link(1)
> - printenv(1)
> - sleep(1)
> - test(1)
> - time(1)
> - true(1)
> - tty(1)
> - uname(1)
> - unexpand(1)
> - unlink(1)
> - whoami(1)
> - yes(1)
>
> Some tests contain "#ifdef TODO", these tests current
> fail, but there are patches submitted for most of them.
> There are not patches submitted for fixing the
> "#ifdef TODO"s in expand.test.c and unexpand.test.c.
>
> Signed-off-by: Mattias Andrée
Sorry for not getting around to looking at this earlier.
I definitely think we should have unit tests for sbase (and other
projects?) as soon as possible. What concerns me with your approach is
that we have about 700 lines of C code in testing-common.{c,h} of which
I feel quite a bit could be dropped.
I have written some (crappy and probably non-portable) shell script
functions to check the stdout and stderr of a process. It's about 40
lines. I also converted your tests for dirname to use these functions
(both files attached. The test coverage is not exactly the same but
relatively similar).
I wonder if we couldn't use some cleaned-up version of the shell script
functions for the easy test cases that only check stdout and stderr
output and your custom C code for the more specialised test cases (like
'tty').
What do you think?
Cheers,
Silvan
> ---
> Makefile| 45 -
> basename.test.c | 68 +++
> dirname.test.c | 55 ++
> echo.test.c | 51 ++
> expand.test.c | 92 ++
> false.test.c| 32
> link.test.c | 58 ++
> printenv.test.c | 79
> sleep.test.c| 53 ++
> test-common.c | 560
>
> test-common.h | 219 ++
> test.test.c | 408 +
> time.test.c | 218 ++
> true.test.c | 31
> tty.test.c | 44 +
> uname.test.c| 283
> unexpand.test.c | 97 ++
> unlink.test.c | 56 ++
> whoami.test.c | 38
> yes.test.c | 131 +
> 20 files changed, 2614 insertions(+), 4 deletions(-)
>
> [snip]
test-common.sh
Description: Bourne shell script
#! /bin/sh
source ./test-common.sh
#"test name" "command to execute" "expected output"
check_stdout "dirname-noarg" "./dirname" "" && \
check_stderr "dirname-noarg-stderr" "./dirname" "usage: ./dirname path\n" && \
check_stdout "dirname-non-existing" "./dirname a b c" "" && \
check_stdout "dirname-slash" "./dirname /" "/\n" && \
check_stdout "dirname-dashes-slash" "./dirname -- /" "/\n" && \
check_stdout "dirname-dashes-slash-a" "./dirname -- /a" "/\n" && \
check_stdout "dirname-doublequotes" "./dirname \"\"" ".\n" && \
check_stdout "dirname-slashes" "./dirname ///" "/\n" && \
check_stdout "dirname-a/b" "./dirname a/b" "a\n" && \
check_stdout "dirname-a/b/" "./dirname a/b/" "a\n" && \
check_stdout "dirname-a/b//" "./dirname a/b//" "a\n" && \
check_stdout "dirname-a" "./dirname a" ".\n" && \
check_stdout "dirname-a/" "./dirname a/" ".\n" && \
check_stdout "dirname-/a/b/c" "./dirname /a/b/c" "/a/b\n" && \
check_stdout "dirname-//a/b/c" "./dirname //a/b/c" "//a/b\n"
