[Toybox] avoiding testing shell builtins

[enh]

On Wed, Jun 26, 2019 at 9:45 PM Rob Landley <rob at landley.net> wrote:
>
> On 6/26/19 6:49 PM, enh wrote:
> > No, this is on Android. I get an error from /system/bin/mksh from the first
> > test.
>
> I mean when there _is_ a shell builtin, TEST_HOST should _not_ test it. This
> would be a design change. Is this the correct behavior? I'd rather not special
> case this, I want to figure out what the right semantics are so they can be
> consistent and documented.
>
> Hmmm... I think the right semantics are that TEST_HOST shouldn't test the shell
> builtin _if_ there's a version in the $PATH. So call which and see if it returns
> something, and if it doesn't use the pathless version...
>
> Something like:
>
> --- a/scripts/test.sh
> +++ b/scripts/test.sh
> @@ -34,11 +34,13 @@ do_test()
>    cd "$TESTDIR" && rm -rf testdir && mkdir testdir && cd testdir || exit 1
>    CMDNAME="${1##*/}"
>    CMDNAME="${CMDNAME%.test}"
> -  C="$CMDNAME"
>    if [ -z "$TEST_HOST" ]
>    then
>      C="$TESTDIR/$CMDNAME"
>      [ ! -e "$C" ] && echo "$CMDNAME disabled" && return
> +  else
> +    C="$(which $CMDNAME 2>/dev/null)"
> +    [ -z "$C" ] && "C=$CMDNAME"
>    fi
>
>    . "$1"
>
> Which is hard to test because what do we have a toybox command for that_doesn't_
> have $PATH entry in debian but has a shell builtin... ah, ulimit. Except I
> haven't written a ulimit.test yet because todo item...
>
> This todo item is "I shouldn't source tests, I should run them as indpendent
> processes so they don't have to clean up their environment", but that means
> making sure I'm exporting the variables I set and auditing all the tests. (Not a
> hard todo item, just time consuming...)

hmm. i already _do_ run each test independently (and in a relatively
clean environment)... and that's the problem here. i did this to
myself:

  adb shell -t "export FILES=/data/local/tmp/toybox-tests/tests/files/ ; \
                export VERBOSE=1 ; \
                export CMDNAME=$toy; \
                export C=$toy; \
                export LANG=en_US.UTF-8; \
                mkdir $tmp_dir/$toy && cd $tmp_dir/$toy ; \
                source /data/local/tmp/toybox-tests/runtest.sh ; \
                source /data/local/tmp/toybox-tests/tests/$toy.test ; \
                if [ "\$FAILCOUNT" -ne 0 ]; then exit 1; fi; \
                cd .. && rm -rf $toy"

fixed by

-                export C=$toy; \
+                export C=\"$(which $toy)\"; \

in my runner. sorry about that!

> Anyway, not a regression, so I checked it in.
>
> (Well I say it's not a regression. Why are two sed host tests failing now? Is
> this an ubuntu->devuan thing? Sigh...)

the sed loop test hangs with TEST_HOST=1 for you too now? haven't we
had this before where i reported that when google switched from ubuntu
to debian?

> Rob
>
>
> > On Wed, Jun 26, 2019, 16:42 Rob Landley <rob at landley.net
> > <mailto:rob at landley.net>> wrote:
> >
> >     On 6/26/19 3:42 PM, enh wrote:
> >     > your patch doesn't fix this for me. even with testcmd i'm still seeing
> >     > the mksh builtin kill get run rather than the toybox kill...
> >
> >     With TEST_HOST it tests just the command name, however the shell decides to
> >     resolve that. Without that it tests the path to the toybox command.
> >
> >     Instead you want the TEST_HOST version also to not test the shell builtin? (Some
> >     host systems don't have non-builtin versions of things like "echo". It's a
> >     design question of what counts as the "host version"...)
> >
> >     Rob
> >
> >
> >     > On Sat, Jun 22, 2019 at 7:35 PM Rob Landley <rob at landley.net
> >     <mailto:rob at landley.net>> wrote:
> >     >>
> >     >> On 6/22/19 11:26 AM, enh via Toybox wrote:
> >     >>> i realized today that the kill tests are testing the shell's builtin
> >     >>> kill, not the toybox kill. one solution would be to use env, but
> >     >>> presumably that would be better in the infrastructure rather than
> >     >>> having to remember to do it in each test that might be subverted by a
> >     >>> builtin?
> >     >>
> >     >> This is why I added testcmd and $C a while back. (Although currently it won't
> >     >> work if the path to the testing directory has a space in it, because I hadn't
> >     >> quoted $C. Just fixed that...)
> >     >>
> >     >> Rob
> >     >
> >