Merge lp://staging/~nmb/bzr/662509-ignore-blanks into lp://staging/bzr

s/ignore_blanks/ignore_output/ is almost my only comment.

This is exactly how I wanted to implement it.

When we add keywords arguments, we try to also use them as such so:

38 + def run_script(self, script, ignore_blanks=False):
39 + return self.script_runner.run_script(self, script, ignore_blanks)

ignore_output=ignore_output

The idea is that if we add more mandatory arguments the above (erroneous) call *may* fail later, whereas using the keyword form ensures that it fails right there, at the call.

49 + def run_script(self, script, ignore_blanks=False):
50 + return self.script_runner.run_script(self, script, ignore_blanks)

60 + return ScriptRunner().run_script(test_case, script_string, ignore_blanks)

idem

74 +If output occurs and no output is expected, the execution stops and the
75 +test fails. If unexpected output occurs on the standard error, then
76 +execution stops.

Good catch !

So ignore_output=False can even be check_output=True

 +irrelevant, the run_script() method may be passed the keyword argument
101 +``ignore_blanks=True``. For example::
102 +
103 + def test_ignoring_blank_output(self):

Don't forget to rename here too.

I let poolie be the second reviewer here but my 'NeedsFixing' vote is really bb:tweak.

For the record: I reviewed here *before* reading the bug report and its comments.

Not that it matters that much, I still like this proposal :)

No more comments except: well done :)

sent to pqm by email

sent to pqm by email

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

Messages crossed on the wire, this has already landed.

I should add support for the option to the 'run_script' command anyway, so I will take your remarks into account there until nmb beats me to it.

Meh, s/until/unless/...

Shall I submit a follow-on patch re-naming the option? I would also like to clarify the stderr situation (because it does make the test fail) and the documentation should just come out and say that.

I'm not clear about the interaction between PQM and launchpad: can I use this mp to make those changes or should I start a new one once this lands in PQM?

On 26 October 2010 09:41:16 UTC-4, Neil Martinsen-Burrell
<email address hidden> wrote:
> Shall I submit a follow-on patch re-naming the option?  I would also like to
> clarify the stderr situation (because it does make the test fail) and the
> documentation should just come out and say that.

That would be good.

> I'm not clear about the interaction between PQM and launchpad: can I use
> this mp to make those changes or should I start a new one once this lands in
> PQM?

You could reuse this one but I think it would be clearer to start a
new mp. It can come from the same branch.
--
Martin

If you make the changes and push, then it will show up here. Just make sure to make a comment so we know it is ready for review. (Just setting it back to "Needs Review" also works)

I've changed the option name and mentioned that it also ignores unspecified output on standard errror.

+1, that looks reasonable to me.

--
Martin