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

[Andy Chu]

Responses inline:

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

By host process do you mean parent process?  I thought this is only a
problem if you do 'foo.test &' which detaches from the terminal (I
think), which we're not doing.  I think this is what session groups in
Unix are for -- if you Ctrl-C then SIGINT is sent to every process in
the same session (roughly).

We can talk about this later but I don't think it should be a problem,
at least if you're running tests serially, as we are.

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

(I happen to know a lot of trivia about killing untrusted processes,
but that's probably not what you're going for here...)

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

OK thanks for the background.

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

That's essentially what I did in patches 1-3, but as mentioned in
patch #4 I moved single command binaries to generated/single/test.  I
suggest getting rid of the top level aliases, but I didn't do that (in
order not to stomp all over your project :) )

I like to think of the arguments to make as *data*, not *actions*.
Data can't collide with data; only actions can collide with data.
It's convenient that data lives on the file system, which already has
namespaces :)


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

scripts/make.sh without arguments still works... the default is 'toybox'.


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

I can change it to an env variable if you want, or if you'd rather go
through everything yourself that's fine too.  I guess that makes sense
since it's optional and not required, and positional arguments are
usually required.  Right now I see that single.sh sets KCONFIG_CONFIG
for make.sh.

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

With patch #4, you get

toybox
toybox_unstripped
generated/
  single/
    sed
    sed_unstripped

I got rid of PREFIX in single.sh.  I think make PREFIX=foo install
should work, because that's a Unix UI convention.  But I don't think I
broke that (I didn't touch install.sh).


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

I think this was bad because you get an error from make:

$ make test_test
... EVERYTHING PASSES
PASS: test -lt
PASS: test -le
make: *** [test_test] Error 1

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

We already agreed that a subprocess is better, but I really think the
globals used for communication should be the ONLY globals.  Right now
the tests can change internal state in the harness and totally screw
it up.  They can't do that if the test harness uses local variables.

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

So for these two issues, I agree the tests aren't in good shape and
there's still work to do on the commands.  But I'm trying not to fix
the entire world in one patch -- I'm just preserving the intent of the
existing tests.  I think the tests need a lot of work, and shouldn't
even use host binaries at all, because that makes them flaky and
unreproducible.

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

Yeah same response... these are obviously not permanent fixes, but
they're much better than what's there!  I'm making more tests green
(at least under some environment; see the stats in my first message).

Now these tests can EASILY be detected by setting HOST_BIN_* to empty
strings.  Before this patch there's no way to detect them, other than
through some tedious labor... I basically triaged the non-hermetic
tests for us, and they can be improved later.

Let me know what you think of the bigger patch.  Hopefully it is in
line philosophically with what you are thinking... I'm game to change
details.  The immediate goal is just to have a test harness that can
prevent trivial regressions like the 'factor' thing, which saves time
and makes it easier to contribute.

Larger goals were on the testing thread from a week ago ... though I
think everything discussed there has this kind of stuff (basic smoke
tests and a green build) as prerequisites.

Andy