On Tue, Mar 4, 2008 at 7:29 PM, jeremy hinds <jeremy.hinds_at_gmail.com> wrote:
> Thanks for offering to test these changes on Windows. I've modified
> the rest of the test scripts to accept the returned exit-code and made
> a few other changes and fixes. Platforms without the Popen3 class
> have an exit code of "None" passed around, so Windows compatibility
> should be handled by doing no exit-code validation when it is None.
Hi Jeremy,
The tests work fine on Windows. The patch looks fine* except for some
minor issues listed below.
*I can't confirm that it works on platforms with Popen3(), it
certainly looks as if it should! Hopefully some other committer can
check that.
> If I generalized too much in the commit message for the individual
> test scripts, let me know and I'll provide more detail about what was
> changed.
A few minor formatting notes. As described here,
http://subversion.tigris.org/hacking.html#other-conventions, we try to
keep lines to 79 columns or less. There are several places in your
patch where you exceed this. No big deal, but please fix. Also, some
of the lines following your edits are no longer indented properly. If
all the arguments to a method/function don't fit on one 80 character
line, then we put the extra args on the following line, indented to
line up with the arguments on the first line.
Here is an example of what I mean in update_tests.py:
@@ -1822,7 +1823,7 @@
url2 = sbox.repo_url + '/A2/mu'
# Remember the original text of the file
- text_r1, err = svntest.actions.run_and_verify_svn(None, None, [],
+ exit_code, text_r1, err = svntest.actions.run_and_verify_svn(None, None, [],
'cat', '-r1', url)
Do this instead:
+ exit_code, text_r1, err = svntest.actions.run_and_verify_svn(None, None, [],
- 'cat', '-r1', url)
+ 'cat', '-r1',
+ url)
I know there are already some examples where we don't do this, but I'd
prefer not to add to these cases.
2nd nitpick: If the opening paren of the function only is almost at 80
characters and would necessitate ugly splitting of individual args,
you can put the first arg on the next line, for example (also from
update_tests.py) instead of this:
@@ -2062,7 +2063,7 @@
I_url = sbox.repo_url + "/A/C/I"
os.remove(I_path)
os.mkdir(I_path)
- so, se = svntest.actions.run_and_verify_svn("Unexpected error during co",
+ exit_code, so, se = svntest.actions.run_and_verify_svn("Unexpected
error during co",
['Checked out revision 2.\n'],
[], "co", I_url, I_path)
This is fine:
+ exit_code, so, se = svntest.actions.run_and_verify_svn(
+ "Unexpected error during co",
+ ['Checked out revision 2.\n'],
+ [], "co", I_url, I_path)
Lastly, while I know we occasionally use the line continuation '\' in
our Python scripts and we have no specific prohibition against it, a
quick survey of committers on #svn-dev showed a decided preference for
not using them. So changes like the following...
Index: subversion/tests/cmdline/basic_tests.py
===================================================================
--- subversion/tests/cmdline/basic_tests.py (revision 29693)
+++ subversion/tests/cmdline/basic_tests.py (working copy)
@@ -189,17 +189,20 @@
# Unversioned paths, those that are not immediate children of a versioned
# path, are skipped and do not raise an error
xx_path = os.path.join(wc_dir, 'xx', 'xx')
- out, err = svntest.actions.run_and_verify_svn("update xx/xx",
+ exit_code, out, err = \
+ svntest.actions.run_and_verify_svn("update xx/xx",
["Skipped
'"+xx_path+"'\n"], [],
'update', xx_path)
...would be better as:
+ exit_code, out, err = svntest.actions.run_and_verify_svn(
+ "update xx/xx", ["Skipped '"+xx_path+"'\n"], [], 'update', xx_path)
> [[[
> Allow testing of application exit codes.
>
> This makes all of the lower-level process-running functions in
> cmdline/svntest/main.py and cmdline/svntest/actions.py return
> the exit code, all of the "run_and_verify_*()" functions guess the
> expected exit code based on whether or not output on stderr is
> expected, and new "run_and_verify_*2()" functions allow the
>
> expected exit code to be provided explicitly.
>
> On platforms without the Popen3 Python class (e.g. Windows), exit
> codes are returned as None, and therefore disregarded during
> validation.
>
>
>
> * subversion/tests/cmdline/svntest/main.py
> (run_command, run_svn, run_svnadmin, run_svnlook, run_svnsync,
> run_svnversion): Include exit_code in the returned tuple.
Mention run_command_stdin() too.
> (create_repos): Accept exit_code returned from run_command.
>
> (run_one): Clarify a comment concerning exit codes
>
>
> * subversion/tests/cmdline/svntest/actions.py
> (run_and_verify_svnlook2, run_and_verify_svnadmin2,
> run_and_verify_svnversion2, run_and_verify_svn2,
> run_and_verify_svn_match_any2): New, execute the indicated binary
> and check actual outputs and exit code against the expected value
> parameters.
>
> (run_and_verify_svnlook, run_and_verify_svnadmin,
> run_and_verify_svnversion, run_and_verify_svn,
> run_and_verify_svn_match_any): Guess whether the expected exit should
> be 0 or 1 based on whether output is expected on stderr. Then invoke
> the coresponding run_and_verify_*2 function with that value. Return
> exit_code, stdout_lines, stderr_lines.
>
> (setup_pristine_repository, run_and_verify_load, run_and_verify_dump,
> run_and_verify_checkout, run_and_verify_update, run_and_verify_commit,
> run_and_verify_status, run_and_verify_unquiet_status,
> run_and_verify_diff_summarize, run_and_verify_diff_summarize_xml,
> run_and_verify_log_xml, run_and_verify_merge2,
> run_and_verify_mergeinfo, run_and_verify_switch, run_and_validate_lock,
> check_prop, inject_conflict_into_wc, run_and_verify_export):
> Accept exit_code value returned from main.run_svn, main.run_command,
> main.run_svnadmin, main.run_command_stdin, actions.run_and_verify_svn
>
> * subversion/tests/cmdline/svntest/tree.py
> (get_props): Accept exit code returned by main.run_svn.
>
>
> * subversion/tests/cmdline/svntest/verify.py
> (SVNUnexpectedExitCode): New exception raised when the exit code was not
> what was expected.
>
> (verify_exit_code): New, compares expected and actual exit code and raises
> an exception if they are different. Comparison is not performed if expected
> exit code is None.
>
> * subversion/tests/cmdline/cat_tests.py,
> subversion/tests/cmdline/lock_tests.py,
> subversion/tests/cmdline/stat_tests.py:
> Accept exit code returned from run_*(), replacing those calls with the
> run_*2() counterparts for cases where stderr output is produced while
> exiting 0.
In the the three preceeding tests you frequently declare
'expected_exit = 0', then pass it to
svntest.actions.run_and_verify_svn2(). Why not just pass 0 to
svntest.actions.run_and_verify_svn2()?
> * subversion/tests/cmdline/authz_tests.py,
> subversion/tests/cmdline/autoprop_tests.py,
> subversion/tests/cmdline/basic_tests.py,
> subversion/tests/cmdline/blame_tests.py,
> subversion/tests/cmdline/changelist_tests.py,
> subversion/tests/cmdline/checkout_tests.py,
> subversion/tests/cmdline/commit_tests.py,
> subversion/tests/cmdline/copy_tests.py,
> subversion/tests/cmdline/depth_tests.py,
> subversion/tests/cmdline/diff_tests.py,
> subversion/tests/cmdline/getopt_tests.py,
> subversion/tests/cmdline/import_tests.py,
> subversion/tests/cmdline/log_tests.py,
> subversion/tests/cmdline/merge_tests.py,
> subversion/tests/cmdline/prop_tests.py,
Please break out prop_tests.py separately since you are accepting the
exit code returned from the run_*() function calls *and* verifying it.
> subversion/tests/cmdline/revert_tests.py,
> subversion/tests/cmdline/schedule_tests.py,
> subversion/tests/cmdline/special_tests.py,
> subversion/tests/cmdline/svnadmin_tests.py,
> subversion/tests/cmdline/svndumpfilter_tests.py,
> subversion/tests/cmdline/svnlook_tests.py,
> subversion/tests/cmdline/svnsync_tests.py,
> subversion/tests/cmdline/svnversion_tests.py,
> subversion/tests/cmdline/switch_tests.py,
> subversion/tests/cmdline/update_tests.py:
> Accept exit code returned from the run_*() function calls.
>
> ]]]
> Index: subversion/tests/cmdline/svntest/actions.py
> ===================================================================
> --- subversion/tests/cmdline/svntest/actions.py (revision 29693)
> +++ subversion/tests/cmdline/svntest/actions.py (working copy)
> @@ -54,9 +54,10 @@
> # import the greek tree, using l:foo/p:bar
> ### todo: svn should not be prompting for auth info when using
> ### repositories with no auth/auth requirements
> - output, errput = main.run_svn(None, 'import',
> - '-m', 'Log message for revision 1.',
> - main.greek_dump_dir,
> main.pristine_url)
> + exit_code, output, errput = main.run_svn(None, 'import',
> + '-m', 'Log message for
> revision 1.',
> + main.greek_dump_dir,
> + main.pristine_url)
>
> # check for any errors from the import
> if len(errput):
> @@ -119,34 +120,78 @@
>
> def run_and_verify_svnlook(message, expected_stdout,
> expected_stderr, *varargs):
> - "Run svnlook command and check its output"
> - out, err = main.run_svnlook(*varargs)
> + """Run svnlook command and check its output and exit code. The
> + expected exit code is assumed to be 0 if no output is expected on
> + stderr, and 1 otherwise."""
While you mention in verify.py:verify_exit_code() that the exit code
is not checked on platforms not supporting Popen3(), I think it is
worth mentioning the same in each of the run_and_verify* functions
here. You only have to explain it in detail once (probably in
run_and_verify_svn2), then put a note in the other functions comments
like "Exit code is not checked on platforms without Popen3 - see note
in run_and_verify_svn2". I just want it obvious what the limitations
are and not assume folks are going to drill down to
verify_exit_code().
> + expected_exit = 0
> + if expected_stderr is not None and expected_stderr != []:
> + expected_exit = 1
> + return run_and_verify_svnlook2(message, expected_stdout,
> expected_stderr,
> + expected_exit, *varargs)
> +
> +def run_and_verify_svnlook2(message, expected_stdout, expected_stderr,
> + expected_exit, *varargs):
> + "Run svnlook command and check its output and exit code."
> + exit_code, out, err = main.run_svnlook(*varargs)
> verify.verify_outputs("Unexpected output", out, err,
> expected_stdout, expected_stderr)
> - return out, err
> + verify.verify_exit_code(message, exit_code, expected_exit)
> + return exit_code, out, err
>
> -
> def run_and_verify_svnadmin(message, expected_stdout,
> expected_stderr, *varargs):
> + """Run svnadmin command and check its output and exit code. The
> + expected exit code is assumed to be 0 if no output is expected on
> + stderr, and 1 otherwise."""
> + expected_exit = 0
> + if expected_stderr is not None and expected_stderr != []:
> + expected_exit = 1
> + return run_and_verify_svnadmin2(message, expected_stdout,
> expected_stderr,
> + expected_exit, *varargs)
> +
> +def run_and_verify_svnadmin2(message, expected_stdout, expected_stderr,
>
> + expected_exit, *varargs):
> "Run svnadmin command and check its output"
> - out, err = main.run_svnadmin(*varargs)
> + exit_code, out, err = main.run_svnadmin(*varargs)
> verify.verify_outputs("Unexpected output", out, err,
> expected_stdout, expected_stderr)
> - return out, err
> + verify.verify_exit_code(message, exit_code, expected_exit)
> + return exit_code, out, err
>
>
> def run_and_verify_svnversion(message, wc_dir, repo_url,
> expected_stdout, expected_stderr):
> + expected_exit = 0
> + if expected_stderr is not None and expected_stderr != []:
> + expected_exit = 1
> + return run_and_verify_svnversion2(message, wc_dir, repo_url,
> + expected_stdout, expected_stderr,
> + expected_exit)
> +
> +def run_and_verify_svnversion2(message, wc_dir, repo_url,
> + expected_stdout, expected_stderr,
> + expected_exit):
> "Run svnversion command and check its output"
> - out, err = main.run_svnversion(wc_dir, repo_url)
> + exit_code, out, err = main.run_svnversion(wc_dir, repo_url)
> verify.verify_outputs("Unexpected output", out, err,
> expected_stdout, expected_stderr)
> - return out, err
> + verify.verify_exit_code(message, exit_code, expected_exit)
> + return exit_code, out, err
>
> +def run_and_verify_svn(message, expected_stdout, expected_stderr,
> *varargs):
> + """like run_and_verify_svn2, but the expected exit code is assumed to
> + be 0 if no output is expected on stderr, and 1 otherwise."""
>
> -def run_and_verify_svn(message, expected_stdout, expected_stderr,
> *varargs):
> - """Invokes main.run_svn() with *VARARGS, return stdout and stderr as
> - lists of lines. For both EXPECTED_STDOUT and EXPECTED_STDERR,
> + expected_exit = 0
> + if expected_stderr is not None and expected_stderr != []:
> + expected_exit = 1
> + return run_and_verify_svn2(message, expected_stdout, expected_stderr,
> + expected_exit, *varargs)
> +
> +def run_and_verify_svn2(message, expected_stdout, expected_stderr,
> + expected_exit, *varargs):
> + """Invokes main.run_svn() with *VARARGS, returns exit code as int,
> stdout
> + and stderr as lists of lines. For both EXPECTED_STDOUT and
> EXPECTED_STDERR,
> create an appropriate instance of verify.ExpectedOutput (if
> necessary):
>
> - If it is an array of strings, create a vanilla ExpectedOutput.
> @@ -163,6 +208,8 @@
> If EXPECTED_STDOUT is None, do not check stdout.
> EXPECTED_STDERR may not be None.
>
> + If output checks pass, the expected and actual codes are compared.
> +
> If a comparison fails, a Failure will be raised."""
>
> if expected_stderr is None:
> @@ -172,15 +219,28 @@
> if expected_stderr is not None and expected_stderr != []:
> want_err = True
>
> - out, err = main.run_svn(want_err, *varargs)
> + exit_code, out, err = main.run_svn(want_err, *varargs)
> verify.verify_outputs(message, out, err, expected_stdout,
> expected_stderr)
> - return out, err
> + verify.verify_exit_code(message, exit_code, expected_exit)
> + return exit_code, out, err
>
> def run_and_verify_svn_match_any(message, expected_stdout,
> expected_stderr,
> *varargs):
> """Like run_and_verify_svn, except that only one stdout line must
> match
> EXPECTED_STDOUT."""
> + expected_exit = 0
> + if expected_stderr is not None and expected_stderr != []:
> + expected_exit = 1
> + return run_and_verify_svn_match_any2(message, expected_stdout,
> + expected_stderr, expected_exit,
> + *varargs)
> +
>
> +def run_and_verify_svn_match_any2(message, expected_stdout,
> expected_stderr,
> + expected_exit, *varargs):
> + """Like run_and_verify_svn, except that only one stdout line must
^^^
run_and_verify_svn2 no?
> match
> + EXPECTED_STDOUT."""
> +
> if expected_stderr is None:
> raise verify.SVNIncorrectDatatype("expected_stderr must not be
> None")
>
Paul
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-03-05 20:53:52 CET