[pacman-dev] [PATCH 2/5] Add unit tests for -Qk and -Qkk with missing files.

Jeremy Heiner scalaprotractor at gmail.com
Mon Dec 16 13:38:11 EST 2013


On Sat, Dec 14, 2013 at 4:03 PM, Andrew Gregory
<andrew.gregory.8 at gmail.com> wrote:
> One of the main reasons I suggested adding install script support for
> this was so that they would work the same as scripts run by pacman and
> therefore be easy to write and understand.  At the very least, they
> should be run in root, not TMPDIR and honor env["scriptlet-shell"].
> This should also be run with fakeroot (use -i and -s to save state
> between invocations) and be run inside a fake chroot.  The only
> downside I see is the need to copy binaries into the test chroot.  It
> looks like your tests would require us to copy at least chown, chmod,
> rm, touch, and ln into the chroot.  Is there any problem with copying
> the additional binaries?

Hey Andrew, thanks for the code review! I obviously wrote these
patches following the "less is more" principle. Every added line of
code is a future potential headache. And if I don't have a solid
use-case argument backing up my edits I'm not going to put my name up
for blame.

I can get my head around the argument that running scripts from the
test root is much less surprising. That gives me what I need to put my
name on that additional bit of code. But I don't have enough yet for
me to feel comfortable taking responsibility for your other
suggestions...

The --scriptlet-shell code makes zero sense to me. The help string in
configure doesn't match the implementation. There is a hard-coded ref
to '/bin/sh' in pmtest.generate, so it is guaranteed that 'bin/sh' is
available under the test root. All --scriptlet-shell does is install a
2nd copy of '/bin/sh' under a different path, and maybe I'm missing
something because that seems to accomplish nothing. What is the point
of telling the test framework to use an alias? Anyway, I would feel
uncomfortable submitting a patch that perpetuates this confusion, and
I am unsure of how to resolve it (which would seem best done in its
own patch set).

I can see what the 'fake' commands accomplish for the binaries, but
can't figure out a scenario where they would be of use for the test
framework. These scripts are run in the phase of a test where the
framework unpacks the 'local' stuff in preparation for eventually
running the binaries. What commands would a test writer invoke during
that phase which would need the 'fake' environment set up? I would
feel weird submitting a patch that adds even the small complexity of
the 'fake' commands without also submitting a positive test case that
demonstrates it does what it is supposed to. And I don't know what
that test case would be.

The downside you mention isn't nothing. Sure, the test filesystem gets
built to RAM (typically), so copying all those extra commands for each
test is pretty fast. But not copying is still faster. And simpler:
there is the ongoing maintenance cost for the list of commands to be
copied as new tests are written. Again, these scripts are explicitly
written to be invoked during a specific phase, where it is known that
a test root is being built, so what advantage is there to having this
one little part of the test framework pretend that it doesn't know
about the real root? Setting up that pretense just amounts to extra
busywork with zero gain.

I'm very willing to be convinced I've got it wrong. My familiarity
with the code is increasing, but still a long way from where you are
at. And if I don't yet know enough to see the use-case justifying the
requested feature then I'm probably not the right one to trust to
implement it correctly. The conservative approach seems to me to add
only what is required by the current tests. It is always easier to add
more complexity as more tests are written than it is to ever yank out
code that doesn't seem to do anything but maybe one day will be used.
Jeremy


More information about the pacman-dev mailing list