[MERGE] cleanup blackbox tests

[Martin Pool]

On 6/27/07, Vincent Ladeuil <v.ladeuil+lp at free.fr> wrote:
> opportunity to look around the test suite with a modification to
> implement :)
>
> >>>>> "mbp" == Martin Pool <mbp at sourcefrog.net> writes:
>
> <snip/>
>
>     mbp> @@ -1280,16 +1298,34 @@
>     mbp>      def run_bzr(self, *args, **kwargs):
>     mbp>          """Invoke bzr, as if it were run from the command line.
>
>     mbp> +        The argument list should not include the bzr program name - the
>     mbp> +        first argument is normally the bzr command.  Arguments may be
>     mbp> +        passed in three ways:
>     mbp> +
>     mbp> +        1- A list of strings, eg ["commit", "a"].  This is recommended
>     mbp> +        when the command contains whitespace or metacharacters, or
>     mbp> +        is built up at run time.
>     mbp> +
>     mbp> +        2- A single string, eg "add a".  This is the most convenient
>     mbp> +        for hardcoded commands.
>     mbp> +
>     mbp> +        3- Several varargs parameters, eg run_bzr("add", "a").
>     mbp> +        This is not recommended for new code.
>     mbp> +
>
> It looks like you still use form (1) in the cleaned code when
> form (2) was... more convenient.
>
> Also if (2) if the most convenient, why not make it (1) so that
> people will use the list notation only when they need to ?

Oh, listing them in that order didn't really mean to imply an order of
preference.  Robert and I have different preferences as to which is
most reasonable.

> More surprisingly here (and nearly everywhere 'unknowns' is used).
>
> I suppose you make that patch in several passes and may have
> change your mind, but as the aim is to clean the code, it will be
> better if the rules presented in run_bzr doc string were enforced
> in the same patch.

Yes, I was originally going to change it to all use lists as Robert
suggested.  But changing them all was a bit of work (it's hard to do
it right with a regexp) and I realized that it was actually making
those call sites worse not better.

> Summary, except for commit where we want to include spaces in the
> message, the list notation is rarely needed.

It's also good when we need to build up a command from constituent
parts - I would say it's safer never to be doing a string join but
always a list join:

not  run_bzr("commit " + filename)
but  run_bzr(["commit", filename])

> I was curious, so I had to try it.
>
> Bingo, it occurs that the vast majority of calls can just be
> changed to use the string syntax (making them more readable and
> saving space) and when the list notation is needed it makes
> things even clearer by separating the command itself from the
> additional parameters. Amazing :)
>
> So here is a another patch, built on top of the first one,
> activating the warning but passing the test suite without
> warnings.
>
> Note: using commit messages without embedded spaces is the most
> useful trick-of-the-day :-)

I don't think we should change them to not use spaces in the test case
as part of this patch, if that's what you're doing.  If you just mean
that when you're interactively committing you can leave off the quotes
then yes, that's true.

> It looks like some tests stayed empty in test_upgrade.py...

 91
 92     def test_upgrade_checkout(self):
 93         # upgrading a checkout should work
 94         pass
 95

That's a bit naughty.  We should change them to the proposed "this
test doesn't exist yet" failure.

> Activating that warning make bzrtools quite un-happy... I have a
> patch available when this one is merged, I don't attached it here
> as I fear that BB may be quite un-happy too ;-)

Because bzrtools makes use of the varargs syntax?  We can always merge
the updates and not yet the deprecation.

-- 
Martin