Code review comment for lp:~vila/bzr/shell-like-tests-borked

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Vincent Ladeuil wrote:
> Vincent Ladeuil has proposed merging lp:~vila/bzr/shell-like-tests into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
>
> This implement a new TestCase that provides the ability to run shell-like scenarios like:
>
> class TestConflict(script.TestCaseWithScript):
>
> def test_content_conflict(self):
> story = """
> bzr init trunk
> cd trunk
> echo "trunk content" >file
> bzr add file
> bzr commit -m 'Create trunk'
> bzr branch . ../branch
> cd ../branch
> bzr rm file
> bzr commit -m 'Delete file'
> cd ../trunk
> echo "more content" >>file
> bzr commit -m 'Modify file'
> cd ../branch
> bzr merge ../trunk
> """
> self.run_script(story)
> wt, relpath = workingtree.WorkingTree.open_containing('branch')
> self.assertEqual(1, len(wt.conflicts()))
>
> Commands implemented: bzr (ha ha), cat, echo, cd, mkdir.
>
> cd and mkdir are jailed.
> Redirections are handled for echo and cat (<, >, >>).
>
> More can be done but I think that implementation is already usable and I don't want to
> add too much features without getting some feedback first.

 review: needs_information

1) My first concern is that the above doesn't provide a way to do any
assertions. It just provides a way to run a bunch of commands.

If you look at how doctest does it, it does:

>>> command_to_run()
output

We could do something similar with:
$ command_to_run()
output

Though we then don't have a way to declare a different between stdout
and stderr. Nor do we have a way to say that this command should succeed
cleanly, or this command should fail in a particular way.

Because of that, it means that the shell scripting really only works as
a way to *set up* a test case. Which is arguably where we should be
spending the least amount of time, not the bulk of the time as we work
in 'command' level rather than internal object level.

Maybe the overhead isn't as big as it once was, because we don't spawn
anymore. But given Robert's recent threads about wanting to *speed up*
selftest, this doesn't seem to be going in the right direction.

2)

+class TestCaseWithScript(tests.TestCaseWithTransport):
...
+ def do_bzr(self, input, args):
...
+ def do_cat(self, input, args):
...

It feels to me like things such as "do_bzr" should be part of a helper
class, not part of the TestCase itself.

So something more like:

class ScriptClass(object):

  def __init__(self, actions_str):
    # parse and build up a list of actions

  def run(self):
    ...

  def do_bzr(...)
  def do_cat(...)
  def do_echo(...)

class TestCaseWithScript(tests.TestCaseWithTransport):

  _script_class = ScriptClass

  def run_script(self, actions):
    script = self._script_class(actions)
    script.run() # Should we pass the TestCase (self) in?

By making _script_class a member variable, test cases that want to
override behavior can do so by subclassing ScriptClass rather than
directly overriding self.do_foo()

3) I think the functionality is well tested. My biggest concerns are how
is this actually going to benefit us.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkqdTioACgkQJdeBCYSNAAOtggCfTZrOoCUzavBOTCynkIr23KEA
UCUAnRcC0OSxvhMfOXfZD5M8kjHRkLzi
=Lzrr
-----END PGP SIGNATURE-----