script tests fail with unspecified output

This was an intentional change, and the documentation should be updated to match.

The problem was that things were easily succeeding accidentally. We generally *do* care about what content is produced. If a given test does not care, I believe it can use:

>...

To say that it can match anything.

I'm really glad that you were finding script like tests useful, and
I'm sorry this inadvertently broke bzr-colo. Maybe we can get it
running in Babune to catch this earlier?

Anyhow, as John says, this was an intentional change, but we can also
learn from the feedback and there are a couple of options

1- Go through and add
...
in the tests, which is what I did for the builtin tests; then we can
mark this wontfix in bzr itself. That should still work against older
bzrs, if you're trying to support them from bzr-colo trunk.

2- Add an option on ScriptRunner that says "blank matches anything",
off by default. Then you can either explicitly turn it on in some
tests, or we could even add a fixture that turns it on across the
whole test class. This is a better choice if you really feel the
original behaviour is better rather than just needing an update. This
would be a bit like the options regexps have to match case-insensitive
or ignoring trailing whitespace...

If you want 2 I'll try to do it...

--
Martin

On 2010-10-18 21:51 , Martin Pool wrote:
> I'm really glad that you were finding script like tests useful, and
> I'm sorry this inadvertently broke bzr-colo. Maybe we can get it
> running in Babune to catch this earlier?
>
> Anyhow, as John says, this was an intentional change, but we can also
> learn from the feedback and there are a couple of options
>
> 1- Go through and add
> ...
> in the tests, which is what I did for the builtin tests; then we can
> mark this wontfix in bzr itself. That should still work against older
> bzrs, if you're trying to support them from bzr-colo trunk.
>
> 2- Add an option on ScriptRunner that says "blank matches anything",
> off by default. Then you can either explicitly turn it on in some
> tests, or we could even add a fixture that turns it on across the
> whole test class. This is a better choice if you really feel the
> original behaviour is better rather than just needing an update. This
> would be a bit like the options regexps have to match case-insensitive
> or ignoring trailing whitespace...

I think that I am okay with 1. Besides the large one-time load of work
for me, it is a better design (explicit better than implicit). 2 would
be reasonably explicit, but I think that updating my tests to match the
style that is used for the builtin tests is the most consistent way
forward. Now to come up with the search-and-replace that does it all in
one fell swoop...

Unfortunately, this seems like it is fairly ugly for lengthy script-based tests, making them look much, much less readable.

Consider the following original test for "bzr colo-branch --from-branch"

    def test_colo_branch_from(self):
        self.run_script("""
$ bzr colo-init
$ bzr ci -m 'commit in trunk' --unchanged
$ bzr colo-branch new
$ bzr ci -m 'commit in new branch' --unchanged
$ bzr colo-branch --from-branch=trunk new2
$ bzr log --line
1: ...
""")

This seems like a succinct description of what should work for the test, which is what Vincent intended when he created script-based tests from Maritza's idea. The new minimal working version is

    def test_colo_branch_from(self):
        self.run_script("""
$ bzr colo-init
...
$ bzr ci -m 'commit in trunk' --unchanged
2>...
$ bzr colo-branch new
2>...
$ bzr ci -m 'commit in new branch' --unchanged
2>...
$ bzr colo-branch --from-branch=trunk new2
2>...
$ bzr log --line
1: ...
""")

where it is even necessary to know which commands produce their output on stdout and which on stderr. This test is much less clear about what is supposed to be happening. I agree that we need a way to test if a command really does produce no output, but it seems like a bad trade-off to have to fill other tests that don't care about output with repeated explicit indications that they don't care about the output.

I thought this was wont-fix, but it looks like I would prefer a flag (False by default) for "ignore_blanks". I can certainly look at adding such a feature myself.

I've found liberal use of -q for preparatory steps the easiest option. That way if something goes wrong you still fail on output mismatch.

Really though, script tests are not a substitute for proper unit tests. If you're writing a bunch where you don't actually care about the console interaction, I'd suggest that's a sign you need more focused testing.

>>>>> Martin [gz] <email address hidden> writes:

    > I've found liberal use of -q for preparatory steps the easiest option.
    > That way if something goes wrong you still fail on output mismatch.

    > Really though, script tests are not a substitute for proper unit tests.
    > If you're writing a bunch where you don't actually care about the
    > console interaction, I'd suggest that's a sign you need more focused
    > testing.

Right, may be we should add that to the doc so I don't repeat it each
the meme surfaces again :)

Shell-like tests are *not* intended to replace unit tests, there main
target is people that don't know [yet] what unit test to write or just
want a way to easily reproduce a bug.

     Vincent