Code review comment for lp:~lool/linaro-image-tools/testsuite-when-installed-v2

On Thu, Mar 24, 2011, Guilherme Salgado wrote:
> The code shuffling is most welcome, but given the size of the diff, it's
> very tricky to find out what are the exact bits that fix bug 711312.
> Care to point them out for me?

 It's the very first revision of the series which has the lp:xyz tag,
 r302 in this branch; it moves from hwpack's find_script to
 media_create's find_command().

> > I'm aware of some minor issues with this patchset:
> > - there is one class of change I didn't finish: converting the "%s foo
> > bar" % sudo_args calls to SUDO_ARGS + ["foo", "bar"]; I only did half
> > of the changes -- I'm happy to finish it if reviewers think that's the
> > right way to pursue the change
>
> Well, for that you reverted my recent changes to use the
> mock's .commands_executed instead of .calls. The former makes for more
> readable tests and makes it easier to spot differences when tests fail,
> so I'm against your changes here. If all you want is to use the
> SUDO_ARGS constant from cmd_runner.py you could just define sudo_args at
> the top of test_media_create.py as
>
> sudo_args = " ".join(SUDO_ARGS)
>
> instead of the current definition. That would allow the tests to
> continue using .commands_executed instead of .calls

 Ok; this is exactly what I guessed might be the case, and the reason I
 stopped mass-converting commands; I actually added a sudo_args
 definition as you suggest. I'll revert the changs from string to list
 in the tests.

> These changes can be saved for later, though, if you'd prefer.

 I don't mind deferring this to a later submission; I'll open a bug to
 not forget about it.

> > I did run the testsuite both from the source tree and from an
> > installed version; I did the later with:
> > sudo ./setup.py install
> > python -m subunit.run linaro_image_tools.tests.test_suite|grep -v
> > ^successful:|grep -v ^test:
>
> Does that actually use the system installed version or would it prefer
> the one in the current directory if you had one? IOW, is '.' prepended
> or appended to sys.path?

 It would probably pick the local version instead of the system one by
 default, but I had taken care to change to another directory before
 running the system-wide tests.

--
Loïc Minier