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

[enh]

On Wed, Sep 7, 2022 at 5:14 AM Rob Landley <rob at landley.net> wrote:

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

sorry, my intended referent wasn't even in your block --- i was grumbling
about "mac only supports `make tests` all at once". specifically "it's
annoying that we can't `make test_foo` on macOS [though i do have a
workaround so it's not a priority, and i don't have a good alternative to
realpath on macOS so it's not trivial]".


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

grouping as an idea is fine, but this implementation just seems hard to
write _and_ hard to read (and non-obvious to know whether you're even
within one).

my preference for "grouping" was just to factor out the condition and then
mark each test. so instead of

skipnot
some-complicated-condition-that-tells-us-whether-we-have-sparse-support-say
test ...
skipnot
some-complicated-condition-that-tells-us-whether-we-have-sparse-support-say
test ...
skipnot
some-complicated-condition-that-tells-us-whether-we-have-sparse-support-say
test ...

we'd have

HAS_SPARSE=some-complicated-condition-that-tells-us-whether-we-have-sparse-support-say
skipnot HAS_SPARSE
test ...
skipnot HAS_SPARSE
test ...
skipnot HAS_SPARSE
test ...

it's mildly repetitive still, but it's more intention revealing (modulo our
ability to come up with a decent name), and it's super easy to read and you
never need a lookbehind of more than one line to know what the line you're
looking at will do.

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

at the very least, if we do keep something like the current implementation
of grouping, i'd recommend we start indenting the tests that are in a group
for readability.

(but then at that part i start to wonder even more what exactly this is
buying us? the .test files are already shell scripts, so isn't `if ... fi`
the better way to do this --- everyone already needs to know that.)


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

(i still think that's a _feature_. see above.)


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

but we could have them "stack" instead, no? if skipnot never _clears_ the
"should i skip?" flag, you can have

skipnot condition
toyonly
testcmd blah...

anyway, right? or, if you really wanted consistency, you could replace
toyonly with a variable that could be used with skipnot for something like

skipnot not-toy     # it's cases like this that really make me want a
skip_if!
skipnot condition


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

i think our biggest disagreement is that i don't think the *many forms are
a good idea.


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

we wouldn't even have to have this discussion if we just said "a little bit
of repetition is helpfully intention-revealing, and there's always `if ...
fi` if there's too much"...


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

btw, i spoke too soon when i said that it works on Android ... i hadn't run
the chattr tests :-(

looks like there are still .test files with SKIPNEXT references but
SKIPNEXT is broken now?

that causes trouble in chattr.test for the reason outlined in the existing
comment there:

# For the rest, just toggle the bits back and forth (where supported).
# Note that some file system/kernel combinations do return success but
# silently ignore your request: +T on 4.19 f2fs, or +F on 5.2 ext4,
# for example, so we're deliberately a bit selective here.
# f2fs in 5.6+ kernels supports compression, but you can only enable
# compression on a file while it's still empty, so we skip +c too.

Rob
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.landley.net/pipermail/toybox-landley.net/attachments/20220907/acdb29ac/attachment.htm>