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

On Tue, Mar 18, 2003 at 03:30:21PM -0600, fitz@tigris.org wrote:
>...
> +++ trunk/subversion/tests/clients/cmdline/prop_tests.py Tue Mar 18 15:29:54 2003
>...
> @@ -72,7 +69,7 @@
> expected_status.tweak('A/D/G', status=' M')
>
> if svntest.actions.run_and_verify_status(wc_dir, expected_status):
> - return 1
> + raise svntest.Failure

Woah! The idea is to *simplify* the test functions. This doesn't simplify
the stuff at all -- it just changes how the function has to handle an error.
Instead, run_and_verify_status() should raise the exception. The test
function would just do:

  svntest.actions.run_and_verify_status(wc_dir, expected_status)

Of course, to do this, you'll need to see if anybody analyzes the return
value from that function, or if they just use the "if test: return 1"
pattern everywhere.

But just switching "return 1" to "raise svntest.Failure" doesn't move us
forward. -1 on that design, sorry :-(

>...
> @@ -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

Same here. compare_trees() would just raise the exception.

Note that you would want to create subclasses of svntest.Failure for this
stuff to raise. For example:

  class DifferentTrees(svntest.Failure):
    pass
    ### or maybe some detailed info about how they are different, which
    ### can then be reported as part of the exception handling

>...
> @@ -121,13 +117,16 @@
> expected_status.tweak('A/mu', 'A/D/H', wc_rev=2, status=' ')
>
> # Commit the one file.
> - return svntest.actions.run_and_verify_commit (wc_dir,
> - expected_output,
> - expected_status,
> - None,
> - None, None,
> - None, None,
> - wc_dir)
> + if svntest.actions.run_and_verify_commit (wc_dir,

Same...

etc etc

>...
> @@ -323,17 +321,16 @@
> mo = re.match(pattern, node.name)
> if mo:
> extra_files.pop(extra_files.index(pattern)) # delete pattern from list
> - return 0
> + return
>
> print "Found unexpected disk object:", node.name
> - raise svntest.main.SVNTreeUnequal
> + raise svntest.Failure

Woah! Losing information. Create a subclass of svntest.Failure instead. Oh
hey... lookee there. SVNTreeUnequal *is* a subclass of svntest.Failure :-)

>...
> # 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

Okay. Now these are legitimate raises within the test functions. :-)

But you should still define a custom exception:

   class ExpectedErrorOutput(svntest.Failure):
     "docco..."
     pass

>...
> @@ -741,14 +730,12 @@
> out, err = svntest.main.run_svn(None, 'propset', '--revprop', '-r', '0',
> 'cash-sound', 'cha-ching!', sbox.wc_dir)
> if err:
> - return 1
> + raise svntest.Failure

I think that we want a run_svn() variant that raises exception if anything
is ever produced on stderr. Maybe run_svn_noerr().

Cheers,
-g

-- 
Greg Stein, http://www.lyra.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org