[Toybox] [PATCH] tar.test: fix the tests when run as root.

[Rob Landley]

On 9/6/22 19:09, enh wrote:
>     I can force the time format with --full-time, although does the mac ls command
>     have that...
> 
>     https://apple.stackexchange.com/questions/15170/how-do-i-change-the-time-format-used-by-ls-command-line-on-osx
>     <https://apple.stackexchange.com/questions/15170/how-do-i-change-the-time-format-used-by-ls-command-line-on-osx>
> 
>     No it does  not. Nor does it provide an obvious alternative, they recommend
>     "brew install coreutils". Right, I'm pretty happy saying mac only supports "make
>     tests" all at once with macos_defconfig, and if you want more than that you can
>     set up the $PATH to appropriate commands yourself. (Comparing toybox, busybox,
>     and debian makes sense. Comparing with macosx is out of scope.)
> 
> it's also useful for debugging specific mac issues... i currently have to `rm
> tests/*.test` and then `git checkout -- tests/tar.test` to test tar on macos in
> a reasonable amount of time.

Um, _what_ is useful? (Happy to make that easier for you, but what specifically
are you recommending I do? Lots of things that first "it" could apply to in the
previous paragraph...)
>     The problem is, if you're still seeing the OLD hash, I can't just change it to
>     the one I'm seeing. If you are, could you send me the tarball? (You saw the
>     "save all tarballs" hack a couple emails back, right? I should work the test
>     name in there or something, although "sha1sum *" should let you know which one
>     has the matching hash...)
>  
> i saw it, but didn't you see my reply saying that it didn't work?

Not ringing a bell, but it's been a week... (dig dig dig...)

Found it. Ah, I remember the _start_ of that message. I opened a reply window,
got halfway through, my laptop battery died between sessions and lost my open
reply windows (thanks thunderbird) and the original message was marked as read. :P

Got it open in another window and responding to it again.

>     I checked in the rest of it, lemme know what I broke?
> 
> it doesn't seem to work at all?
> 
> you got the test the wrong way round (it should have been `&& SKIP=1` not `||
> SKIP=1`), but then it only seems to last for one test anyway?

Sigh. I changed those tests manually to make sure the plumbing was skipping
them, and the "check for darwin" version I reverted to after wasn't right...

> oh, because you're supposed to count the number of tests to skip? i'm not sure i
> have the words to express how terrible of an idea this seems to me? that's so
> unreadable an idea even its own author got it wrong? :-P
> 
> i still don't understand why there isn't just a "positive" version of skipnot?

There can be, but skipnot increments skip by 1 so would still have to be
repeated before each test instead of grouped.

These test suite changes open new semantic possibilities, you can now skip
GROUPS of tests. The interesting cases are probably just "skip next" and "skip
until told to stop", but a generic mechanism was easier to implement.

The new API for USING the new infrastructure was quick and dirty "get something
checked in". It could definitely use more polishing. I got distracted by "huh,
'optional' has almost entirely gone away hasn't it" and "the documentation at
the start of runtest.sh is completely out of date" and testing every change I
made under mksh to make sure it supports ((++blah)) and such...

> personally i think the only thing wrong with skipnot is that it's backwards,
> which makes the tests hard to read.

Semantics: it skips ONE test.

skipnot()
{
...
  [ $? -eq 0 ] || { ((++SKIP)); return 1; }
}

It does not start a group of skipped tests.

Also, "toyonly testcmd blah..." is one line, where:

  skipnot condition
  testcmd blah...

Is two and they can even have arbitrary extra stuff _between_ them (comments or
mkdir etc)... not consistent semantics there.

I was reluctant to expand that without giving it more thought. Sure I can add
four "skipif", "skipnot", "skipifmany", "skipnotmany" but I'm not sure that's
the right way to go vs just "here's a flexible mechanism"...

toyonly encapsulates a non-obvious test (are we running under toybox), and using
((++SKIP)) instead of SKIP=1 is subtly important to have an individual test skip
_inside_ a larger block that could be skipped for other reasons:

  [ thingy ] || SKIP=1

Is a sharp edge because it terminates the skip block you're in after this test...

> i actually _like_ having a `skip_if` on
> tests; each test is understandable in its own right, without you needing to read
> any context. (that's actually part of why _i_ wasn't offended by the repetition
> of all the uname checks in my version, but i'd much rather factor out a DARWIN=1
> or something if you want something brief like `skip_if DARWIN` instead.)

I agree it needs more design work. And documentation. And a walkthrough video.
It's on the todo heap with making the test suite run under mkroot and the whole
"running root tests" vs "running non-root tests" (which test different things,
for example in tests/test.test where the access tests punch right through the
permissions as root so aren't really _testing_ as much. This means I probably
have to have mkroot run the test suite TWICE, both as root and as non-root...)

> but expecting humans to _count_ the number of tests to skip (and to know, when
> adding/removing/changing existing tests, that they might invisibly be in such a
> "block") sounds like the kind of thing that even a FORTRAN IV programmer might
> have said "dude, are you sure?" to...

That's why the new "optional" sets it to 99999.

Having it count down and expire means the same mechanism can handle "skip the
next test" and "skip all tests until we switch this back off". That's basically
an implementation detail. I exposed it in tar.test because I hadn't worked out
what a wrapper should look like yet (or if there should be one).

(The current answer to the question "ah but can different skip blocks nest?" is
"not going there". Yes I _could_ verbose_has the SKIP plumbing but I really
wouldn't want to DOCUMENT that...)

> from this line:
> 
> SKIP=0 # End of tests that don't work on MacOS X
> 
> i don't think you even believed in it yourself :-) did you accidentally check in
> something that was between two changes?

The semantics changed a couple times while I was thinking it through and the
LOCAL test I used to make sure skipping them worked wasn't "darwin", and then I
used "undo" to put the darwin tests back and it reverted too far. :P

Rob