Code review comment for lp:~nmb/bzr/662509-ignore-blanks

Thanks for adding this.

btw my lp userid is 'mbp' not poolie; it's a bit of a usability bug
you could make that error.

To me 'ignore_output' means "don't pay attention to the output at
all", which is not I think what you're doing here. We want something
that says that blank expected output matches anything. Maybe
'blank_output_matches_anything'?

The number of places that need to be updated in script.py suggests
there may be too much repetition there already.

>
> -When no output is specified, any ouput from the command is accepted
> -and execution continue.
> +If output occurs and no output is expected, the execution stops and the
> +test fails.  If unexpected output occurs on the standard error, then
> +execution stops.

Thanks for fixing the typo. I'm not sure if you're specifically
trying to draw a distinction between unexpected text on stdout and
stderr, and if so whether it's true. Are you saying that unexpected
stderr makes execution stop but the test doesn't fail?

>
>  If an error occurs and no expected error is specified, the execution stops.
>
> @@ -447,11 +448,11 @@
>     def test_unshelve_keep(self):
>         # some setup here
>         script.run_script(self, '''
> -            $ bzr add file
> -            $ bzr shelve --all -m Foo
> +            $ bzr add -q file
> +            $ bzr shelve -q --all -m Foo
>             $ bzr shelve --list
>             1: Foo
> -            $ bzr unshelve --keep
> +            $ bzr unshelve -q --keep
>             $ bzr shelve --list
>             1: Foo
>             $ cat file
> @@ -471,6 +472,19 @@
>             yes
>             """)
>
> +To avoid having to specify "-q" for all commands whose output is
> +irrelevant, the run_script() method may be passed the keyword argument
> +``ignore_blanks=True``.  For example::
> +
> +    def test_ignoring_blank_output(self):
> +        self.run_script("""
> +            $ bzr init
> +            $ bzr ci -m 'first revision' --unchanged
> +            $ bzr log --line
> +            1: ...
> +            """
> +
> +

but you don't seem to actually pass this option?

Can you also just make clear if how this handles stderr output?

It seems like we should doctest testing.txt, but doing so may take
some larger changes.

>  Import tariff tests
>  -------------------
>
>
> === modified file 'doc/en/release-notes/bzr-2.3.txt'
> --- doc/en/release-notes/bzr-2.3.txt    2010-10-18 21:34:05 +0000
> +++ doc/en/release-notes/bzr-2.3.txt    2010-10-19 20:54:48 +0000
> @@ -99,6 +99,10 @@
>   Instead, use '...' as a wildcard if you don't care about the output.
>   (Martin Pool, #637830)
>
> +* Add an ignore_blanks keyword argument with default False to
> +  bzrlib.tests.script.ScriptRunner.run_script to specify if missing
> +  output does not need to be matched.  (Neil Martinsen-Burrell, #662509)
> +
>  * ``bzr test-script script`` is a new command that runs a shell-like script
>   from an the ``script`` file. (Vincent Ladeuil)
>
>
>
>

--
Martin