[Toybox] [PATCH SET] Fix test harness issues and fix tests

[Rob Landley]

On 03/18/2016 08:24 PM, Andy Chu wrote:
> This is a pretty big cleanup, still in progress, but I think these
> parts are ready to merge.

It's a large lump, some bits of which I disagree with. Should I apply
and then apply an undo patch, or chip off bits?

> I think we should change the test "protocol" to be a subprocess rather
> sourcing the .test file, as you alluded to.  But that can be done in a
> separate step on top of this.

Agreed.

The problem is that when you ctrl-C and it's executing a (subshell),
the host process doesn't get killed. So I tried to do it myself, and
it got... complicated.

When you have multiple shell levels ((subshell) within subshell),
which is easy to do with shell functions calling shell functions,
it's hard to get the PIDs of the various shell levels. (You'd think
$PPID would be the parent of the current process and $$ would be the
current process, but just try it: it doesn't work. And it's hard to
interact with /proc/self without spawning a temporary process to do
so, which gives you the wrong answers...)

Anyway, I've made it work sometimes, by having trap EXIT call this:

https://github.com/landley/aboriginal/blob/master/sources/utility_functions.sh#L93

But sometimes it doesn't work, and I never figured out why. (That's on
the todo list for when I open the toysh can of worms.)

> There's also some cruft like '. testing.sh' at the top of most tests,
> but it doesn't seem to exist.  As long as I'm going in the right
> direction I can fix all this.

Yeah that's historical. Commit 387edf547eb0 moved it.

In theory you used to be able run the blah.test files yourself if you
were willing to set up your $PATH. Then the wrapper grew TEST_HOST,
and single tests vs adding all the commands in the current toybox
binary to the $PATH... And running them by themselves became less
interesting, and having a file in the tests directory that wasn't
a test was kind of awkward...

The test suite isn't 100% tied to toybox. Before my most recent encounter
with the posix mailing list convinced me it's Jorg Schilling's pet
project, I was thinking making an independent command line test would
be a good thing. Now, posix can go hang, the committee is dead and it's
a historical document.

The '. testing.sh' stuff is leftover from wanting to break out the test
suite into its own project someday.

> [PATCH 1/2] Refactor the test harness to triage tests, and fix some
> bugs.
>
> genconfig.sh:
> - Fix a name clash bug where 'make test' would run all tests and then
> try to build the 'test' binary.  'make test_bin' is special cased for now.

In scripts/genconfig.sh there's:

  while IFS=":" read FILE NAME
  do
    [ "$NAME" == help ] && continue
    [ "$NAME" == install ] && continue

I should have added 'test' to that.

Adding the individual command names to the makefile target list is
awkward, and there are multiple conflicts, but I haven't tought of a
better interface yet.

> - Make the test targets .PHONY, since we're not creating files called
> test_sed, etc.

Good.

> - Use here docs to write Makefile fragments, for readability and to be
>  consistent with the rest of the script.  Refactor into functions.

Sure.

(On the one hand, here documents are efficiently collated. On the other
hand, they break indentation.)

> - Add comments.

Often a good thing. :)

> make.sh:
> - Add the output binary name as the first param.

I still want scripts/make.sh to work if run by itself, and you've sort
of broken that unless people know to provide a magic default argument.

The way it used to work is that scripts/single.sh set environment variables
that modified the behavior of scripts/make.sh, so the communication was
sort of out of band.

> This fixes an issue where 'make; make test_sed' would leave you without
> a 'toybox' binary because single.sh created another toybox binary and
> renamed it to sed.

Indeed. This is worth fixing, but I'd prefer to have single.sh set more
environment variables ot modify the behavior further rather than adding
another mechanism to be used on top of the existing mechanisms. (Since
scripts/single.sh is assembling a config file with sed, you can't
easily bypass that script to build your own singleconfig files anyway.)

The design hiccup is that scripts/single.sh can have a $PREFIX on
the start of the filename. Should the _unstripped versions be in that
directory, or stay in the toybox directory? Also, if it's making
ls_unstripped in the toybox directory then I need to teach make clean
to kill *_unstripped.

> runtest.sh:
> - Changed the 'testing' function to return 0.  This fixes a bug where
>   the return value of the script depends on the return code of the
>   last command -- NOT whether it passed or failed.  For example,
>   'make test_test' would fail with code 1, even though all tests
>   passed (because 'test' exits 1 validly).

The pass/fail stuff written to stdout was the result, the return
code never meant anything. (There was some attempt to return the number
of pass/fail but the "who is calling us" question kinda screwed that
up.)

Still... sure?

>  On the other hand, tests with failures would still return 0 if the last
>  command didn't fail.
> - Use local variables instead of globals since we are sharing globals
> with the tests themselves.

Some of the variables were meant to be shared with the tests, but ok.

(This is more leftover from the days when you ran the scripts directly,
transitioning to it being run by a wrapper. There's dangling design
elements I never got back to...)

> - Add a common function to skip tests if not running as root,
> and keep track of skipped commands.

Ok.

> [PATCH 2/2] Various test fixes and cleanup.
>
> bzcat: Fix a test that was succeeding for the wrong reason.  The test
> was looking for a 1 exit code in the case of bad data, but there was a
> typo in the input path, so it failed 1 for the wrong reason.
>
> bzcat, xzcat, zcat: Use $HOST_BIN_TAR to fix the issue where they fail
> under 'make test' but not 'make test_bzcat'.

tar is in pending for a reason.

"make test" tests the current toybox binary, built with the current .config.
If you don't select a command in menuconfig, it doesn't get built into
the toybox binary, so it tests the host version. So you could test
individual versions before I made scripts/single.sh, I just did that
so you could build binaries without the multiplexier (because people
wanted that).

> hostname: Use $HOST_BIN_HOSTNAME, and skip one test if not root.

No, we should fix toybox hostname.

> chgrp, groupadd, groupdel, ifconfig, losetup, mount, useradd: Skip if
> not root.

Good. (I've got a couple of those locally, but swapping out for the
function is good.)

> pgrep, pkill: style cleanup and speedup of the tests (change sleep 1 to
> sleep 0.1).  No fixes yet.

I've been using sleep .25 because I've had .1 hiccup testing under qemu
on a slow box when the host system isn't idle, but yes. :)

> renice: Fix some quoting problems (revealing further errors, not fixed)
> 
> tar: Style cleanup in preparation to fix failures under 'make test' but
> not 'make test_tar' (due to host/toybox discrepancy)

Then we should fix the host/toybox discrepancies.

(That said, both the tar.test and tar.c files were contributed and I've
only glanced at them a bit.)

> touch: Use $HOST_BIN_DATE because toybox doesn't support %N nanoseconds

No, the correct solution is to fix toybox date. We can test using the
host date by switching our date to N in menuconfig.

Rob

 1458533323.0