[Toybox] avoiding testing shell builtins

[enh]

On Wed, Jun 26, 2019 at 10:33 PM enh <enh at google.com> wrote:
>
> 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)\"; \

well, okay, maybe there should be a \ before the first $ there :-)

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