Merge lp:~nmb/bzr/662509-ignore-blanks into lp:bzr
- 662509-ignore-blanks
- Merge into bzr.dev
Status: | Merged | ||||
---|---|---|---|---|---|
Merged at revision: | 5532 | ||||
Proposed branch: | lp:~nmb/bzr/662509-ignore-blanks | ||||
Merge into: | lp:bzr | ||||
Diff against target: |
156 lines (+53/-12) 4 files modified
bzrlib/tests/script.py (+22/-7) bzrlib/tests/test_script.py (+8/-0) doc/developers/testing.txt (+19/-5) doc/en/release-notes/bzr-2.3.txt (+4/-0) |
||||
To merge this branch: | bzr merge lp:~nmb/bzr/662509-ignore-blanks | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Vincent Ladeuil | Approve | ||
Martin Pool | Pending | ||
Poolie | Pending | ||
Review via email: mp+38889@code.launchpad.net |
Commit message
Add 'ignore_output' parameter to run_script.
Description of the change
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) wrote : | # |
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) wrote : | # |
No more comments except: well done :)
Vincent Ladeuil (vila) wrote : | # |
sent to pqm by email
Vincent Ladeuil (vila) wrote : | # |
sent to pqm by email
Martin Pool (mbp) wrote : | # |
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_
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_
> # some setup here
> script.
> - $ 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_
> +
> + def test_ignoring_
> + 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/
> --- doc/en/
> +++ doc/en/
> @@ -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.
> + 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) wrote : | # |
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) wrote : | # |
Meh, s/until/unless/...
Neil Martinsen-Burrell (nmb) 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.
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) wrote : | # |
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) wrote : | # |
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) wrote : | # |
I've changed the option name and mentioned that it also ignores unspecified output on standard errror.
Martin Pool (mbp) wrote : | # |
+1, that looks reasonable to me.
--
Martin
Preview Diff
1 | === modified file 'bzrlib/tests/script.py' | |||
2 | --- bzrlib/tests/script.py 2010-10-28 00:06:59 +0000 | |||
3 | +++ bzrlib/tests/script.py 2010-11-05 03:06:51 +0000 | |||
4 | @@ -196,7 +196,8 @@ | |||
5 | 196 | self.output_checker = doctest.OutputChecker() | 196 | self.output_checker = doctest.OutputChecker() |
6 | 197 | self.check_options = doctest.ELLIPSIS | 197 | self.check_options = doctest.ELLIPSIS |
7 | 198 | 198 | ||
9 | 199 | def run_script(self, test_case, text): | 199 | def run_script(self, test_case, text, |
10 | 200 | blank_output_matches_anything=False): | ||
11 | 200 | """Run a shell-like script as a test. | 201 | """Run a shell-like script as a test. |
12 | 201 | 202 | ||
13 | 202 | :param test_case: A TestCase instance that should provide the fail(), | 203 | :param test_case: A TestCase instance that should provide the fail(), |
14 | @@ -204,7 +205,12 @@ | |||
15 | 204 | attribute used as a jail root. | 205 | attribute used as a jail root. |
16 | 205 | 206 | ||
17 | 206 | :param text: A shell-like script (see _script_to_commands for syntax). | 207 | :param text: A shell-like script (see _script_to_commands for syntax). |
18 | 208 | |||
19 | 209 | :param blank_output_matches_anything: For commands with no specified | ||
20 | 210 | output, ignore any output that does happen, including output on | ||
21 | 211 | standard error. | ||
22 | 207 | """ | 212 | """ |
23 | 213 | self.blank_output_matches_anything = blank_output_matches_anything | ||
24 | 208 | for cmd, input, output, error in _script_to_commands(text): | 214 | for cmd, input, output, error in _script_to_commands(text): |
25 | 209 | self.run_command(test_case, cmd, input, output, error) | 215 | self.run_command(test_case, cmd, input, output, error) |
26 | 210 | 216 | ||
27 | @@ -245,6 +251,12 @@ | |||
28 | 245 | else: | 251 | else: |
29 | 246 | test_case.fail('expected output: %r, but found nothing' | 252 | test_case.fail('expected output: %r, but found nothing' |
30 | 247 | % (expected,)) | 253 | % (expected,)) |
31 | 254 | |||
32 | 255 | blank_output_matches_anything = getattr(self, | ||
33 | 256 | 'blank_output_matches_anything', False) | ||
34 | 257 | if blank_output_matches_anything and expected is None: | ||
35 | 258 | return | ||
36 | 259 | |||
37 | 248 | expected = expected or '' | 260 | expected = expected or '' |
38 | 249 | matching = self.output_checker.check_output( | 261 | matching = self.output_checker.check_output( |
39 | 250 | expected, actual, self.check_options) | 262 | expected, actual, self.check_options) |
40 | @@ -473,8 +485,9 @@ | |||
41 | 473 | super(TestCaseWithMemoryTransportAndScript, self).setUp() | 485 | super(TestCaseWithMemoryTransportAndScript, self).setUp() |
42 | 474 | self.script_runner = ScriptRunner() | 486 | self.script_runner = ScriptRunner() |
43 | 475 | 487 | ||
46 | 476 | def run_script(self, script): | 488 | def run_script(self, script, blank_output_matches_anything=False): |
47 | 477 | return self.script_runner.run_script(self, script) | 489 | return self.script_runner.run_script(self, script, |
48 | 490 | blank_output_matches_anything=blank_output_matches_anything) | ||
49 | 478 | 491 | ||
50 | 479 | def run_command(self, cmd, input, output, error): | 492 | def run_command(self, cmd, input, output, error): |
51 | 480 | return self.script_runner.run_command(self, cmd, input, output, error) | 493 | return self.script_runner.run_command(self, cmd, input, output, error) |
52 | @@ -502,14 +515,16 @@ | |||
53 | 502 | super(TestCaseWithTransportAndScript, self).setUp() | 515 | super(TestCaseWithTransportAndScript, self).setUp() |
54 | 503 | self.script_runner = ScriptRunner() | 516 | self.script_runner = ScriptRunner() |
55 | 504 | 517 | ||
58 | 505 | def run_script(self, script): | 518 | def run_script(self, script, blank_output_matches_anything=False): |
59 | 506 | return self.script_runner.run_script(self, script) | 519 | return self.script_runner.run_script(self, script, |
60 | 520 | blank_output_matches_anything=blank_output_matches_anything) | ||
61 | 507 | 521 | ||
62 | 508 | def run_command(self, cmd, input, output, error): | 522 | def run_command(self, cmd, input, output, error): |
63 | 509 | return self.script_runner.run_command(self, cmd, input, output, error) | 523 | return self.script_runner.run_command(self, cmd, input, output, error) |
64 | 510 | 524 | ||
65 | 511 | 525 | ||
67 | 512 | def run_script(test_case, script_string): | 526 | def run_script(test_case, script_string, blank_output_matches_anything=False): |
68 | 513 | """Run the given script within a testcase""" | 527 | """Run the given script within a testcase""" |
70 | 514 | return ScriptRunner().run_script(test_case, script_string) | 528 | return ScriptRunner().run_script(test_case, script_string, |
71 | 529 | blank_output_matches_anything=blank_output_matches_anything) | ||
72 | 515 | 530 | ||
73 | 516 | 531 | ||
74 | === modified file 'bzrlib/tests/test_script.py' | |||
75 | --- bzrlib/tests/test_script.py 2010-10-27 23:27:41 +0000 | |||
76 | +++ bzrlib/tests/test_script.py 2010-11-05 03:06:51 +0000 | |||
77 | @@ -186,6 +186,14 @@ | |||
78 | 186 | $ echo foo | 186 | $ echo foo |
79 | 187 | """) | 187 | """) |
80 | 188 | 188 | ||
81 | 189 | def test_blank_output_matches_option(self): | ||
82 | 190 | """If you want blank output to be a wild card, you can pass | ||
83 | 191 | blank_output_matches_anything to run_script""" | ||
84 | 192 | self.run_script( | ||
85 | 193 | """ | ||
86 | 194 | $ echo foo | ||
87 | 195 | """, blank_output_matches_anything=True) | ||
88 | 196 | |||
89 | 189 | def test_ellipsis_everything(self): | 197 | def test_ellipsis_everything(self): |
90 | 190 | """A simple ellipsis matches everything.""" | 198 | """A simple ellipsis matches everything.""" |
91 | 191 | self.run_script(""" | 199 | self.run_script(""" |
92 | 192 | 200 | ||
93 | === modified file 'doc/developers/testing.txt' | |||
94 | --- doc/developers/testing.txt 2010-10-13 07:55:13 +0000 | |||
95 | +++ doc/developers/testing.txt 2010-11-05 03:06:51 +0000 | |||
96 | @@ -367,8 +367,9 @@ | |||
97 | 367 | The execution stops as soon as an expected output or an expected error is not | 367 | The execution stops as soon as an expected output or an expected error is not |
98 | 368 | matched. | 368 | matched. |
99 | 369 | 369 | ||
102 | 370 | When no output is specified, any ouput from the command is accepted | 370 | If output occurs and no output is expected, the execution stops and the |
103 | 371 | and execution continue. | 371 | test fails. If unexpected output occurs on the standard error, then |
104 | 372 | execution stops and the test fails. | ||
105 | 372 | 373 | ||
106 | 373 | If an error occurs and no expected error is specified, the execution stops. | 374 | If an error occurs and no expected error is specified, the execution stops. |
107 | 374 | 375 | ||
108 | @@ -447,11 +448,11 @@ | |||
109 | 447 | def test_unshelve_keep(self): | 448 | def test_unshelve_keep(self): |
110 | 448 | # some setup here | 449 | # some setup here |
111 | 449 | script.run_script(self, ''' | 450 | script.run_script(self, ''' |
114 | 450 | $ bzr add file | 451 | $ bzr add -q file |
115 | 451 | $ bzr shelve --all -m Foo | 452 | $ bzr shelve -q --all -m Foo |
116 | 452 | $ bzr shelve --list | 453 | $ bzr shelve --list |
117 | 453 | 1: Foo | 454 | 1: Foo |
119 | 454 | $ bzr unshelve --keep | 455 | $ bzr unshelve -q --keep |
120 | 455 | $ bzr shelve --list | 456 | $ bzr shelve --list |
121 | 456 | 1: Foo | 457 | 1: Foo |
122 | 457 | $ cat file | 458 | $ cat file |
123 | @@ -471,6 +472,19 @@ | |||
124 | 471 | yes | 472 | yes |
125 | 472 | """) | 473 | """) |
126 | 473 | 474 | ||
127 | 475 | To avoid having to specify "-q" for all commands whose output is | ||
128 | 476 | irrelevant, the run_script() method may be passed the keyword argument | ||
129 | 477 | ``blank_output_matches_anything=True``. For example:: | ||
130 | 478 | |||
131 | 479 | def test_ignoring_blank_output(self): | ||
132 | 480 | self.run_script(""" | ||
133 | 481 | $ bzr init | ||
134 | 482 | $ bzr ci -m 'first revision' --unchanged | ||
135 | 483 | $ bzr log --line | ||
136 | 484 | 1: ... | ||
137 | 485 | """, blank_output_matches_anything=True) | ||
138 | 486 | |||
139 | 487 | |||
140 | 474 | Import tariff tests | 488 | Import tariff tests |
141 | 475 | ------------------- | 489 | ------------------- |
142 | 476 | 490 | ||
143 | 477 | 491 | ||
144 | === modified file 'doc/en/release-notes/bzr-2.3.txt' | |||
145 | --- doc/en/release-notes/bzr-2.3.txt 2010-11-02 22:23:40 +0000 | |||
146 | +++ doc/en/release-notes/bzr-2.3.txt 2010-11-05 03:06:51 +0000 | |||
147 | @@ -109,6 +109,10 @@ | |||
148 | 109 | Instead, use '...' as a wildcard if you don't care about the output. | 109 | Instead, use '...' as a wildcard if you don't care about the output. |
149 | 110 | (Martin Pool, #637830) | 110 | (Martin Pool, #637830) |
150 | 111 | 111 | ||
151 | 112 | * Add a blank_output_matches_anything keyword argument with default False to | ||
152 | 113 | bzrlib.tests.script.ScriptRunner.run_script to specify if missing | ||
153 | 114 | output does not need to be matched. (Neil Martinsen-Burrell, #662509) | ||
154 | 115 | |||
155 | 112 | * ``bzr test-script script`` is a new command that runs a shell-like script | 116 | * ``bzr test-script script`` is a new command that runs a shell-like script |
156 | 113 | from an the ``script`` file. (Vincent Ladeuil) | 117 | from an the ``script`` file. (Vincent Ladeuil) |
157 | 114 | 118 |
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): runner. run_script( self, script, ignore_blanks)
39 + return self.script_
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): runner. run_script( self, script, ignore_blanks)
50 + return self.script_
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 blanks= True``. For example:: blank_output( self):
101 +``ignore_
102 +
103 + def test_ignoring_
Don't forget to rename here too.
I let poolie be the second reviewer here but my 'NeedsFixing' vote is really bb:tweak.