Re: svn commit: rev 5378 - trunk/subversion/tests/clients/cmdline

This looks fine in general. More comments below.

fitz@tigris.org wrote:

> if svntest.actions.run_and_verify_status(wc_dir, expected_status):
>- return 1
>+ raise svntest.Failure
>
> # Remove one property
> svntest.main.run_svn(None, 'propdel', 'yellow',
>@@ -89,8 +86,8 @@
> actual_disk_tree = svntest.tree.build_tree_from_wc(wc_dir, 1)
>
> # Compare actual vs. expected disk trees.
>- return svntest.tree.compare_trees(expected_disk.old_tree(), actual_disk_tree)
>-
>+ if svntest.tree.compare_trees(expected_disk.old_tree(), actual_disk_tree):
>+ raise svntest.Failure
>

These two functions (compare_trees and run_and_verify_status) are
probably the sort that could be converted to just throw an exception.
Also run_and_verify_commit, run_and_verify_update...

The trouble with that is, of course, that you'd have to check every call
in all the tests to make sure that we're not trying to do something
clever with the return code anywhere. (You'd not have to actually
convert all uses to stop returning error codes -- those checks would
just be skipped on error, like in the sbox.build() case.)

> # These should produce an error
> outlines,errlines = svntest.main.run_svn('Illegal target', 'propset',
> 'svn:executable', 'on',
> A_path)
> if not errlines:
>- return 1
>+ raise svntest.Failure
>

And this is the sort of thing I'd like to see abstracted in the long
run. You probably noticed that we have tons of code like this, where we
should instead just pass the expected stdout and stderr into a function
that would check them for us. Just a note, though -- no need to do it
now. Maybe it would be best to add another issue about this.

-- 
Brane Čibej   <brane_at_xbc.nu>   http://www.xbc.nu/brane/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org