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

[Andrew Gregory]

On 12/16/13 at 01:38pm, Jeremy Heiner wrote:
> 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).

The alias isn't for the test runner, it's for pacman.  If it didn't
exist, pacman wouldn't be able to find the configured SCRIPTLET_SHELL.
I was slightly mistaken in my original comment; which path should be
used for local scripts actually depends on whether or not they end up
being run in a chroot.  Running in a chroot, they would use
--scriptlet-shell because we know it will be suitable to use for
scripts because pacman itself needs it.  Outside a chroot, they should
continue using '/bin/sh' because that's what the sync scripts in the
tests currently get regardless of what scriptlet-shell refers to.

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

fakeroot will let you call chown in querycheck008.

Using a chroot allows us to use --scriptlet-shell, which we know will
be suitable for running scripts, instead of 'bin/sh' and improves
compatibility with the scripts run by pacman, which can contain
absolute paths.  Though, paths in tests should generally be relative
anyway and I doubt we'll be moving away from using 'bin/sh' for sync
scripts anytime soon.  The gains are fairly minor, so I'm not terribly
concerned about using a chroot.

> 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