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

[Neil Martinsen-Burrell]

Add 'ignore_output' parameter to run_script.

This change provides and ignore_blanks argument to run_script to allow for the previous default behavior of unspecified output not being matched.

[Vincent Ladeuil (vila)]

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.

review: Needs Fixing

[Vincent Ladeuil (vila)]

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 :)

[Vincent Ladeuil (vila)]

No more comments except: well done :)

review: Approve

[Vincent Ladeuil (vila)]

sent to pqm by email

[Vincent Ladeuil (vila)]

sent to pqm by email

[Martin Pool (mbp)]

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

[Vincent Ladeuil (vila)]

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.

[Vincent Ladeuil (vila)]

Meh, s/until/unless/...

[Neil Martinsen-Burrell (nmb)]

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?

[Martin Pool (mbp)]

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

[John A Meinel (jameinel)]

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)

[Neil Martinsen-Burrell (nmb)]

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

[Martin Pool (mbp)]

+1, that looks reasonable to me.

--
Martin

=== modified file 'bzrlib/tests/script.py'
--- bzrlib/tests/script.py	2010-10-28 00:06:59 +0000
+++ bzrlib/tests/script.py	2010-11-05 03:06:51 +0000
@@ -196,7 +196,8 @@
         self.output_checker = doctest.OutputChecker()
         self.check_options = doctest.ELLIPSIS
 
-    def run_script(self, test_case, text):
+    def run_script(self, test_case, text, 
+                   blank_output_matches_anything=False):
         """Run a shell-like script as a test.
 
         :param test_case: A TestCase instance that should provide the fail(),
@@ -204,7 +205,12 @@
             attribute used as a jail root.
 
         :param text: A shell-like script (see _script_to_commands for syntax).
+
+        :param blank_output_matches_anything: For commands with no specified
+            output, ignore any output that does happen, including output on
+            standard error.
         """
+        self.blank_output_matches_anything = blank_output_matches_anything
         for cmd, input, output, error in _script_to_commands(text):
             self.run_command(test_case, cmd, input, output, error)
 
@@ -245,6 +251,12 @@
             else:
                 test_case.fail('expected output: %r, but found nothing'
                             % (expected,))
+
+        blank_output_matches_anything = getattr(self, 
+            'blank_output_matches_anything', False)
+        if blank_output_matches_anything and expected is None: 
+            return
+        
         expected = expected or ''
         matching = self.output_checker.check_output(
             expected, actual, self.check_options)
@@ -473,8 +485,9 @@
         super(TestCaseWithMemoryTransportAndScript, self).setUp()
         self.script_runner = ScriptRunner()
 
-    def run_script(self, script):
-        return self.script_runner.run_script(self, script)
+    def run_script(self, script, blank_output_matches_anything=False):
+        return self.script_runner.run_script(self, script, 
+                   blank_output_matches_anything=blank_output_matches_anything)
 
     def run_command(self, cmd, input, output, error):
         return self.script_runner.run_command(self, cmd, input, output, error)
@@ -502,14 +515,16 @@
         super(TestCaseWithTransportAndScript, self).setUp()
         self.script_runner = ScriptRunner()
 
-    def run_script(self, script):
-        return self.script_runner.run_script(self, script)
+    def run_script(self, script, blank_output_matches_anything=False):
+        return self.script_runner.run_script(self, script,
+                   blank_output_matches_anything=blank_output_matches_anything)
 
     def run_command(self, cmd, input, output, error):
         return self.script_runner.run_command(self, cmd, input, output, error)
 
 
-def run_script(test_case, script_string):
+def run_script(test_case, script_string, blank_output_matches_anything=False):
     """Run the given script within a testcase"""
-    return ScriptRunner().run_script(test_case, script_string)
+    return ScriptRunner().run_script(test_case, script_string,
+               blank_output_matches_anything=blank_output_matches_anything)
 

=== modified file 'bzrlib/tests/test_script.py'
--- bzrlib/tests/test_script.py	2010-10-27 23:27:41 +0000
+++ bzrlib/tests/test_script.py	2010-11-05 03:06:51 +0000
@@ -186,6 +186,14 @@
             $ echo foo
             """)
 
+    def test_blank_output_matches_option(self):
+        """If you want blank output to be a wild card, you can pass 
+        blank_output_matches_anything to run_script"""
+        self.run_script(
+            """
+            $ echo foo
+            """, blank_output_matches_anything=True)
+
     def test_ellipsis_everything(self):
         """A simple ellipsis matches everything."""
         self.run_script("""

=== modified file 'doc/developers/testing.txt'
--- doc/developers/testing.txt	2010-10-13 07:55:13 +0000
+++ doc/developers/testing.txt	2010-11-05 03:06:51 +0000
@@ -367,8 +367,9 @@
 The execution stops as soon as an expected output or an expected error is not
 matched.
 
-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 and the test fails.
 
 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
+``blank_output_matches_anything=True``.  For example::
+
+    def test_ignoring_blank_output(self):
+        self.run_script("""
+            $ bzr init
+            $ bzr ci -m 'first revision' --unchanged
+            $ bzr log --line
+            1: ...
+            """, blank_output_matches_anything=True)
+           
+
 Import tariff tests
 -------------------
 

=== modified file 'doc/en/release-notes/bzr-2.3.txt'
--- doc/en/release-notes/bzr-2.3.txt	2010-11-02 22:23:40 +0000
+++ doc/en/release-notes/bzr-2.3.txt	2010-11-05 03:06:51 +0000
@@ -109,6 +109,10 @@
   Instead, use '...' as a wildcard if you don't care about the output.
   (Martin Pool, #637830)
 
+* Add a blank_output_matches_anything 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)