Merge lp:~mbp/bzr/scripts into lp:bzr

[Martin Pool]

empty output in shell-like tests is no longer a wildcard

This changes the scriptrunner behaviour so that

"""
$ echo foo
"""

will fail, because it asserts echo doesn't produce output.

I think this is less confusing. If you want to allow for a command that might produce output but you don't care, you can say '...'. It's also more consistent with regular doctest.

This is going to put on a slight extra burden that you need to think about commands that may produce output, and if we change a command that is initially silent to produce output, we may need to update a bunch of tests. However I think this is much safer than not letting people check things actually are silent.

test_resolve is a bit messy, but I think it's actually better after the changes: more clear about which bits it actually is or isn't testing, and it points out the usefulness of -q when you don't want to see the output for a particular command.

[Vincent Ladeuil (vila)]

I agree, having the test writer think about -q and '...' sounds like a good plan.
The only drawback I could think of is that it will require a bit more expertise
than a mere copy/paste. No big loss here, I'm still waiting for bug reporters to
do that anyway :-D

127 - test_case.assertEqualDiff(expected, actual)
128 + test_case.assertEquals(expected, actual)

Why ??? EqualDiff provides valuable info, please don't remove that !

review: Needs Fixing

[Martin Pool (mbp)]

On 15 September 2010 03:29, Vincent Ladeuil <email address hidden> wrote:
> Review: Needs Fixing
> I agree, having the test writer think about -q and '...' sounds like a good plan.
> The only drawback I could think of is that it will require a bit more expertise
> than a mere copy/paste. No big loss here, I'm still waiting for bug reporters to
> do that anyway :-D
>
> 127     - test_case.assertEqualDiff(expected, actual)
> 128     + test_case.assertEquals(expected, actual)
>
> Why ??? EqualDiff provides valuable info, please don't remove that !

The diff is nicer to read, but it's less precise when there are
differences of whitespace etc, to the extent that I couldn't work out
what was going wrong. Perhaps I'll write a separate method that tries
to get the best of both, in a followon patch. For now I'll just
revert that.

--
Martin

[Martin Pool (mbp)]

sent to pqm by email

[Martin Pool (mbp)]

sent to pqm by email

[Andrew Bennetts (spiv)]

sent to pqm by email

[John A Meinel (jameinel)]

sent to pqm by email

[John A Meinel (jameinel)]

Manually updated to resolve NEWS and submitted to pqm via pqm-submit.

[John A Meinel (jameinel)]

The tests are failing, not just a NEWS conflict, so I'm bouncing this back to Martin.

[Martin Pool (mbp)]

The script failures are fixed by <https://code.edge.launchpad.net/~mbp/bzr/656694-verbosity/+merge/37933>. The problem was that if you set -q or -v in a script command, it was inherited by the whole process. Thanks spiv, vila!

[John A Meinel (jameinel)]

seems fine to me

review: Approve

[Martin Pool (mbp)]

sent to pqm by email

=== modified file 'NEWS'
--- NEWS	2010-10-08 01:47:43 +0000
+++ NEWS	2010-10-08 06:36:46 +0000
@@ -504,6 +504,10 @@
 Testing
 *******
 
+* Blank output section in scriptrunner tests no longer match any output.
+  Instead, use '...' as a wildcard if you don't care about the output.
+  (Martin Pool, #637830)
+
 * ``build_tree_contents`` can create symlinks.
   (Martin Pool, John Arbash Meinel)
 

=== modified file 'bzrlib/commands.py'
--- bzrlib/commands.py	2010-08-13 07:56:06 +0000
+++ bzrlib/commands.py	2010-10-08 06:36:46 +0000
@@ -687,7 +687,12 @@
 
         self._setup_outf()
 
-        return self.run(**all_cmd_args)
+        try:
+            return self.run(**all_cmd_args)
+        finally:
+            # reset it, so that other commands run in the same process won't
+            # inherit state
+            trace.set_verbosity_level(0)
 
     def _setup_run(self):
         """Wrap the defined run method on self with a cleanup.

=== modified file 'bzrlib/tests/__init__.py'
--- bzrlib/tests/__init__.py	2010-10-08 01:43:13 +0000
+++ bzrlib/tests/__init__.py	2010-10-08 06:36:46 +0000
@@ -858,6 +858,10 @@
         self._track_transports()
         self._track_locks()
         self._clear_debug_flags()
+        # Isolate global verbosity level, to make sure it's reproducible
+        # between tests.  We should get rid of this altogether: bug 656694. --
+        # mbp 20101008
+        self.overrideAttr(bzrlib.trace, '_verbosity_level', 0)
 
     def debug(self):
         # debug a frame up.

=== modified file 'bzrlib/tests/blackbox/test_bound_branches.py'
--- bzrlib/tests/blackbox/test_bound_branches.py	2010-05-02 20:10:25 +0000
+++ bzrlib/tests/blackbox/test_bound_branches.py	2010-10-08 06:36:46 +0000
@@ -443,7 +443,9 @@
     def test_bind_when_bound(self):
         self.run_script("""
 $ bzr init trunk
+...
 $ bzr init copy
+...
 $ cd copy
 $ bzr bind ../trunk
 $ bzr bind
@@ -453,6 +455,7 @@
     def test_bind_before_bound(self):
         self.run_script("""
 $ bzr init trunk
+...
 $ cd trunk
 $ bzr bind
 2>bzr: ERROR: No location supplied and no previous location known

=== modified file 'bzrlib/tests/blackbox/test_dpush.py'
--- bzrlib/tests/blackbox/test_dpush.py	2010-06-23 08:19:28 +0000
+++ bzrlib/tests/blackbox/test_dpush.py	2010-10-08 06:36:46 +0000
@@ -104,6 +104,7 @@
 
         script.run_script(self, '''
             $ bzr dpush -d dc d
+            2>Pushed up to revision 2.
             $ bzr revno dc
             2
             $ bzr status dc
@@ -121,6 +122,7 @@
         self.build_tree_contents([("dc/foofile", "blaaaal")])
         script.run_script(self, '''
             $ bzr dpush -d dc d --no-strict
+            2>Pushed up to revision 2.
             ''')
         self.assertFileEqual("blaaaal", "dc/foofile")
         # if the dummy vcs wasn't that dummy we could uncomment the line below

=== modified file 'bzrlib/tests/blackbox/test_shelve.py'
--- bzrlib/tests/blackbox/test_shelve.py	2010-09-29 12:58:17 +0000
+++ bzrlib/tests/blackbox/test_shelve.py	2010-10-08 06:36:46 +0000
@@ -77,10 +77,18 @@
         sr = ScriptRunner()
         sr.run_script(self, '''
 $ bzr add file
+adding file
 $ bzr shelve --all -m Foo
+2>Selected changes:
+2>-D  file
+2>Changes shelved with id "1".
 $ bzr shelve --list
   1: Foo
 $ bzr unshelve --keep
+2>Using changes with id "1".
+2>Message: Foo
+2>+N  file
+2>All changes applied successfully.
 $ bzr shelve --list
   1: Foo
 $ cat file

=== modified file 'bzrlib/tests/script.py'
--- bzrlib/tests/script.py	2010-09-13 08:14:44 +0000
+++ bzrlib/tests/script.py	2010-10-08 06:36:46 +0000
@@ -221,20 +221,30 @@
         retcode, actual_output, actual_error = method(test_case,
                                                       str_input, args)
 
-        self._check_output(output, actual_output, test_case)
-        self._check_output(error, actual_error, test_case)
+        try:
+            self._check_output(output, actual_output, test_case)
+        except AssertionError, e:
+            raise AssertionError(str(e) + " in stdout of command %s" % cmd)
+        try:
+            self._check_output(error, actual_error, test_case)
+        except AssertionError, e:
+            raise AssertionError(str(e) +
+                " in stderr of running command %s" % cmd)
         if retcode and not error and actual_error:
             test_case.fail('In \n\t%s\nUnexpected error: %s'
                            % (' '.join(cmd), actual_error))
         return retcode, actual_output, actual_error
 
     def _check_output(self, expected, actual, test_case):
-        if expected is None:
-            # Specifying None means: any output is accepted
-            return
-        if actual is None:
-            test_case.fail('We expected output: %r, but found None'
-                           % (expected,))
+        if not actual:
+            if expected is None:
+                return
+            elif expected == '...\n':
+                return
+            else:
+                test_case.fail('expected output: %r, but found nothing'
+                            % (expected,))
+        expected = expected or ''
         matching = self.output_checker.check_output(
             expected, actual, self.check_options)
         if not matching:

=== modified file 'bzrlib/tests/test_conflicts.py'
--- bzrlib/tests/test_conflicts.py	2010-04-06 10:12:42 +0000
+++ bzrlib/tests/test_conflicts.py	2010-10-08 06:36:46 +0000
@@ -625,20 +625,18 @@
     # tests MissingParent resolution :-/
     preamble = """
 $ bzr init trunk
+...
 $ cd trunk
 $ mkdir dir
-$ bzr add dir
-$ bzr commit -m 'Create trunk'
-
+$ bzr add -q dir
+$ bzr commit -m 'Create trunk' -q
 $ echo 'trunk content' >dir/file
-$ bzr add dir/file
-$ bzr commit -m 'Add dir/file in trunk'
-
-$ bzr branch . -r 1 ../branch
+$ bzr add -q dir/file
+$ bzr commit -q -m 'Add dir/file in trunk'
+$ bzr branch -q . -r 1 ../branch
 $ cd ../branch
-$ bzr rm dir
-$ bzr commit -m 'Remove dir in branch'
-
+$ bzr rm dir -q
+$ bzr commit -q -m 'Remove dir in branch'
 $ bzr merge ../trunk
 2>+N  dir/
 2>+N  dir/file
@@ -649,15 +647,15 @@
 
     def test_take_this(self):
         self.run_script("""
-$ bzr rm dir  --force
+$ bzr rm -q dir  --force
 $ bzr resolve dir
-$ bzr commit --strict -m 'No more conflicts nor unknown files'
+$ bzr commit -q --strict -m 'No more conflicts nor unknown files'
 """)
 
     def test_take_other(self):
         self.run_script("""
 $ bzr resolve dir
-$ bzr commit --strict -m 'No more conflicts nor unknown files'
+$ bzr commit -q --strict -m 'No more conflicts nor unknown files'
 """)
 
 
@@ -665,22 +663,20 @@
 
     preamble = """
 $ bzr init trunk
+...
 $ cd trunk
 $ mkdir dir
 $ echo 'trunk content' >dir/file
-$ bzr add
-$ bzr commit -m 'Create trunk'
-
+$ bzr add -q
+$ bzr commit -m 'Create trunk' -q
 $ echo 'trunk content' >dir/file2
-$ bzr add dir/file2
-$ bzr commit -m 'Add dir/file2 in branch'
-
-$ bzr branch . -r 1 ../branch
+$ bzr add -q dir/file2
+$ bzr commit -q -m 'Add dir/file2 in branch'
+$ bzr branch -q . -r 1 ../branch
 $ cd ../branch
-$ bzr rm dir/file --force
-$ bzr rm dir
-$ bzr commit -m 'Remove dir/file'
-
+$ bzr rm -q dir/file --force
+$ bzr rm -q dir
+$ bzr commit -q -m 'Remove dir/file'
 $ bzr merge ../trunk
 2>+N  dir/
 2>+N  dir/file2
@@ -692,34 +688,36 @@
     def test_keep_them_all(self):
         self.run_script("""
 $ bzr resolve dir
-$ bzr commit --strict -m 'No more conflicts nor unknown files'
+$ bzr commit -q --strict -m 'No more conflicts nor unknown files'
 """)
 
     def test_adopt_child(self):
         self.run_script("""
-$ bzr mv dir/file2 file2
-$ bzr rm dir --force
+$ bzr mv -q dir/file2 file2
+$ bzr rm -q dir --force
 $ bzr resolve dir
-$ bzr commit --strict -m 'No more conflicts nor unknown files'
+$ bzr commit -q --strict -m 'No more conflicts nor unknown files'
 """)
 
     def test_kill_them_all(self):
         self.run_script("""
-$ bzr rm dir --force
+$ bzr rm -q dir --force
 $ bzr resolve dir
-$ bzr commit --strict -m 'No more conflicts nor unknown files'
+$ bzr commit -q --strict -m 'No more conflicts nor unknown files'
 """)
 
     def test_resolve_taking_this(self):
         self.run_script("""
 $ bzr resolve --take-this dir
-$ bzr commit --strict -m 'No more conflicts nor unknown files'
+2>...
+$ bzr commit -q --strict -m 'No more conflicts nor unknown files'
 """)
 
     def test_resolve_taking_other(self):
         self.run_script("""
 $ bzr resolve --take-other dir
-$ bzr commit --strict -m 'No more conflicts nor unknown files'
+2>...
+$ bzr commit -q --strict -m 'No more conflicts nor unknown files'
 """)
 
 
@@ -727,22 +725,20 @@
 
     preamble = """
 $ bzr init trunk
+...
 $ cd trunk
 $ mkdir dir
 $ echo 'trunk content' >dir/file
-$ bzr add
-$ bzr commit -m 'Create trunk'
-
-$ bzr rm dir/file --force
-$ bzr rm dir --force
-$ bzr commit -m 'Remove dir/file'
-
-$ bzr branch . -r 1 ../branch
+$ bzr add -q
+$ bzr commit -m 'Create trunk' -q
+$ bzr rm -q dir/file --force
+$ bzr rm -q dir --force
+$ bzr commit -q -m 'Remove dir/file'
+$ bzr branch -q . -r 1 ../branch
 $ cd ../branch
 $ echo 'branch content' >dir/file2
-$ bzr add dir/file2
-$ bzr commit -m 'Add dir/file2 in branch'
-
+$ bzr add -q dir/file2
+$ bzr commit -q -m 'Add dir/file2 in branch'
 $ bzr merge ../trunk
 2>-D  dir/file
 2>Conflict: can't delete dir because it is not empty.  Not deleting.
@@ -753,34 +749,36 @@
     def test_keep_them_all(self):
         self.run_script("""
 $ bzr resolve dir
-$ bzr commit --strict -m 'No more conflicts nor unknown files'
+$ bzr commit -q --strict -m 'No more conflicts nor unknown files'
 """)
 
     def test_adopt_child(self):
         self.run_script("""
-$ bzr mv dir/file2 file2
-$ bzr rm dir --force
+$ bzr mv -q dir/file2 file2
+$ bzr rm -q dir --force
 $ bzr resolve dir
-$ bzr commit --strict -m 'No more conflicts nor unknown files'
+$ bzr commit -q --strict -m 'No more conflicts nor unknown files'
 """)
 
     def test_kill_them_all(self):
         self.run_script("""
-$ bzr rm dir --force
+$ bzr rm -q dir --force
 $ bzr resolve dir
-$ bzr commit --strict -m 'No more conflicts nor unknown files'
+$ bzr commit -q --strict -m 'No more conflicts nor unknown files'
 """)
 
     def test_resolve_taking_this(self):
         self.run_script("""
 $ bzr resolve --take-this dir
-$ bzr commit --strict -m 'No more conflicts nor unknown files'
+$ bzr commit -q --strict -m 'No more conflicts nor unknown files'
 """)
 
     def test_resolve_taking_other(self):
         self.run_script("""
 $ bzr resolve --take-other dir
-$ bzr commit --strict -m 'No more conflicts nor unknown files'
+2>deleted dir/file2
+2>deleted dir
+$ bzr commit -q --strict -m 'No more conflicts nor unknown files'
 """)
 
 
@@ -882,19 +880,19 @@
 
     preamble = """
 $ bzr init trunk
+...
 $ cd trunk
 $ bzr mkdir foo
-$ bzr commit -m 'Create trunk'
+...
+$ bzr commit -m 'Create trunk' -q
 $ echo "Boing" >foo/bar
-$ bzr add foo/bar
-$ bzr commit -m 'Add foo/bar'
-
-$ bzr branch . -r 1 ../branch
+$ bzr add -q foo/bar
+$ bzr commit -q -m 'Add foo/bar'
+$ bzr branch -q . -r 1 ../branch
 $ cd ../branch
 $ rm -r foo
 $ echo "Boo!" >foo
-$ bzr commit -m 'foo is now a file'
-
+$ bzr commit -q -m 'foo is now a file'
 $ bzr merge ../trunk
 2>+N  foo.new/bar
 2>RK  foo => foo.new/
@@ -906,32 +904,34 @@
 
     def test_take_this(self):
         self.run_script("""
-$ bzr rm foo.new --force
+$ bzr rm -q foo.new --force
 # FIXME: Isn't it weird that foo is now unkown even if foo.new has been put
 # aside ? -- vila 090916
-$ bzr add foo
+$ bzr add -q foo
 $ bzr resolve foo.new
-$ bzr commit --strict -m 'No more conflicts nor unknown files'
+$ bzr commit -q --strict -m 'No more conflicts nor unknown files'
 """)
 
     def test_take_other(self):
         self.run_script("""
-$ bzr rm foo --force
-$ bzr mv foo.new foo
+$ bzr rm -q foo --force
+$ bzr mv -q foo.new foo
 $ bzr resolve foo
-$ bzr commit --strict -m 'No more conflicts nor unknown files'
+$ bzr commit -q --strict -m 'No more conflicts nor unknown files'
 """)
 
     def test_resolve_taking_this(self):
         self.run_script("""
 $ bzr resolve --take-this foo.new
-$ bzr commit --strict -m 'No more conflicts nor unknown files'
+2>...
+$ bzr commit -q --strict -m 'No more conflicts nor unknown files'
 """)
 
     def test_resolve_taking_other(self):
         self.run_script("""
 $ bzr resolve --take-other foo.new
-$ bzr commit --strict -m 'No more conflicts nor unknown files'
+2>...
+$ bzr commit -q --strict -m 'No more conflicts nor unknown files'
 """)
 
 
@@ -943,19 +943,19 @@
         # conflict.
         self.run_script("""
 $ bzr init trunk
+...
 $ cd trunk
 $ bzr mkdir foo
-$ bzr commit -m 'Create trunk'
+...
+$ bzr commit -m 'Create trunk' -q
 $ rm -r foo
 $ echo "Boo!" >foo
-$ bzr commit -m 'foo is now a file'
-
-$ bzr branch . -r 1 ../branch
+$ bzr commit -m 'foo is now a file' -q
+$ bzr branch -q . -r 1 ../branch -q
 $ cd ../branch
 $ echo "Boing" >foo/bar
-$ bzr add foo/bar
-$ bzr commit -m 'Add foo/bar'
-
+$ bzr add -q foo/bar -q
+$ bzr commit -m 'Add foo/bar' -q
 $ bzr merge ../trunk
 2>bzr: ERROR: Tree transform is malformed [('unversioned executability', 'new-1')]
 """)

=== modified file 'bzrlib/tests/test_delta.py'
--- bzrlib/tests/test_delta.py	2010-04-26 16:59:56 +0000
+++ bzrlib/tests/test_delta.py	2010-10-08 06:36:46 +0000
@@ -65,8 +65,7 @@
         reporter.report(file_id, (old_path, path), versioned_change, renamed,
             modified, exe_change, kind)
         if expected_lines is not None:
-            for i in range(len(expected_lines)):
-                self.assertEqualDiff(expected_lines[i], result[i])
+            self.assertEqualDiff('\n'.join(expected_lines), '\n'.join(result))
         else:
             self.assertEqual([], result)
 

=== modified file 'bzrlib/tests/test_script.py'
--- bzrlib/tests/test_script.py	2010-09-13 09:00:03 +0000
+++ bzrlib/tests/test_script.py	2010-10-08 06:36:46 +0000
@@ -19,6 +19,7 @@
     commands,
     osutils,
     tests,
+    trace,
     ui,
     )
 from bzrlib.tests import script
@@ -162,6 +163,31 @@
     def test_unknown_command(self):
         self.assertRaises(SyntaxError, self.run_script, 'foo')
 
+    def test_blank_output_mismatches_output(self):
+        """If you give output, the output must actually be blank.
+        
+        See <https://bugs.launchpad.net/bzr/+bug/637830>: previously blank
+        output was a wildcard.  Now you must say ... if you want that.
+        """
+        self.assertRaises(AssertionError,
+            self.run_script,
+            """
+            $ echo foo
+            """)
+
+    def test_ellipsis_everything(self):
+        """A simple ellipsis matches everything."""
+        self.run_script("""
+        $ echo foo
+        ...
+        """)
+
+    def test_ellipsis_matches_empty(self):
+        self.run_script("""
+        $ cd .
+        ...
+        """)
+
     def test_stops_on_unexpected_output(self):
         story = """
 $ mkdir dir
@@ -170,7 +196,6 @@
 """
         self.assertRaises(AssertionError, self.run_script, story)
 
-
     def test_stops_on_unexpected_error(self):
         story = """
 $ cat
@@ -190,10 +215,13 @@
         # The status matters, not the output
         story = """
 $ bzr init
+...
 $ cat >file
 <Hello
 $ bzr add file
+...
 $ bzr commit -m 'adding file'
+2>...
 """
         self.run_script(story)
 
@@ -245,6 +273,15 @@
 cat dog "chicken" 'dragon'
 """)
 
+    def test_verbosity_isolated(self):
+        """Global verbosity is isolated from commands run in scripts.
+        """
+        # see also 656694; we should get rid of global verbosity
+        self.run_script("""
+        $ bzr init --quiet a
+        """)
+        self.assertEquals(trace.is_quiet(), False)
+
 
 class TestCat(script.TestCaseWithTransportAndScript):
 
@@ -356,7 +393,10 @@
 class TestBzr(script.TestCaseWithTransportAndScript):
 
     def test_bzr_smoke(self):
-        self.run_script('$ bzr init branch')
+        self.run_script("""
+            $ bzr init branch
+            Created a standalone tree (format: ...)
+            """)
         self.failUnlessExists('branch')
 
 

=== modified file 'doc/developers/testing.txt'
--- doc/developers/testing.txt	2010-09-23 09:54:51 +0000
+++ doc/developers/testing.txt	2010-10-08 06:36:46 +0000
@@ -388,6 +388,12 @@
 If you want the command to succeed for any output, just use::
 
   $ bzr add file
+  ...
+  2>...
+
+or use the ``--quiet`` option::
+
+  $ bzr add -q file
 
 The following will stop with an error::