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          chvt            cksum*          clear
>       cmp             cols            comm            cp
>       cron            ctrlaltdel      cut             date
>       dd              df              diff            dmesg
>       du              ed              eject           env
>       expr            fallocate       find            flock
>       fold            free            freeramdisk     fsfreeze
>       getconf         getty           grep            halt
>       head            hostname        hwclock         id
>       insmod          install         join            kill
>       killall5        last            lastlog         ln
>       logger          login           logname         ls
>       lsmod           lsusb           md5sum*         mesg
>       mkdir           mkfifo          mknod           mkswap
>       mktemp          mount           mountpoint      mv
>       nice            nl              nohup           nologin*
>       od              pagesize        passwd          paste
>       patch*          pathchk         pidof           pivot_root
>       printf          ps              pwd             pwdx
>       readahead       readlink        renice          respawn
>       rev             rm              rmdir           rmmod
>       sed             seq*            setsid          sha1sum*
>       sha224sum*      sha256sum*      sha384sum*      sha512-224sum*
>       sha512-256sum*  sha512sum*      sort            split
>       sponge          stat            strings         stty
>       su              swaplabel       swapoff         swapon
>       switch_root     sync            sysctl          tail
>       tar             tee             tftp            touch
>       tr              truncate        tsort           umount
>       uniq*           unshare         uptime          uudecode
>       uuencode        vtallow         watch           wc
>       which           who             xargs
> 
> I think should look into what their tests will require, maybe
> even write some tests with optional test framework, especially
> for the tests that cannot be trivially tested in sh. We should
> also identify which of them cannot be automatically tested
> without a virtual machine, and write them down in the README,
> and possibly list of features to test. Once this is done, we
> can make a more informed discussion.
> 
> For the moment I see two options: 1) write the tests mostly
> in sh and write special utilities in C where required, and
> 2) use the C code, but look for ways to improve it.

I feel like 1) is the way to go if we want to keep things as simple
as possible. The rest are special cases which can be handled in a
special way.

Would you be open for working on some (portable) shell code for the most
common tools?


> 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.


Cheers,

Silvan

Attachment: signature.asc
Description: PGP signature

Reply via email to