I don't have the original post handy, but here is a review of the
patch attached to issue #2301.
[[[
Add revision to take time agrument in this testing module.
* /subversion/tests/clients/cmdline/diff_tests.py
Imported time and datetime modules.
(diff_with_time_option): A new function to test
revision with date and time arguments.
]]]
Don't use a leading slash on "/subversion", that would make it an
absolute path instead of relative to trunk.
> --- diff_tests.py (revision 14491)
> +++ diff_tests.py (working copy)
Please use the relative path from trunk when generating diffs:
"subversion/tests/clients/cmdline/diff_tests.py".
> # General modules
> -import string, sys, re, os.path
> +import string, sys, re, os.path, datetime,time
Space before "time".
> # Our testing module
> import svntest
> @@ -1771,7 +1771,56 @@
> raise svntest.Failure
>
>
> +# Added diff with time option
> +# Tried to include almost all differing
> +# time options in the book
> +def diff_with_time_option(sbox):
> + "diff with time option"
> + sbox.build()
> + wc_dir =sbox.wc_dir
This comment is almost as though you don't believe you're really
writing committable code :-). Every function in the file was "added"
at some point or another, so you don't need to say it.
The next two lines of the comment belong in the test's code...
> + current_dir = os.getcwd()
> + os.chdir(sbox.wc_dir)
...right here, above the combinations:
> + arg1 = time.strftime('%Y-%m-%d')
> + arg2 = time.strftime('%H:%M')
> + arg3 = time.strftime('%Y-%m-%d %H:%M')
> + arg4 = datetime.datetime.today().isoformat()
> + arg5 = datetime.datetime.now()
> + arg6 = time.strftime("%H:%M:%S")
> + arg7 = time.strftime('%Y%m%dT%H%M')
> + arg8 = time.strftime('%Y%m%dT%H%M%z')
> + arg9 = time.strftime('%Y-%m-%dT%H:%M%z')
> +
> + #Needed a space in between,could'nt anything better
> + s = []
> + s = time.strftime('%Y-%m-%d %H:%M%z')
> + s = s[:16] + ' ' + s[16:]
Grammar, and space after "#".
> + try:
> + svntest.actions.run_and_verify_svn(None,None,[],
> + 'diff', '-r','{'+str(arg1)+'}')
> + svntest.actions.run_and_verify_svn(None,None,[],
> + 'diff', '-r','{'+str(arg2)+'}')
> + svntest.actions.run_and_verify_svn(None,None,[],
> + 'diff', '-r','{'+str(arg3)+'}')
> + svntest.actions.run_and_verify_svn(None,None,[],
> + 'diff', '-r','{'+str(arg4)+'}')
> + svntest.actions.run_and_verify_svn(None,None,[],
> + 'diff', '-r','{'+str(arg5)+'}')
> + svntest.actions.run_and_verify_svn(None,None,[],
> + 'diff', '-r','{'+str(arg6)+'}')
> + svntest.actions.run_and_verify_svn(None,None,[],
> + 'diff', '-r','{'+str(arg7)+'}')
> + svntest.actions.run_and_verify_svn(None,None,[],
> + 'diff', '-r','{'+str(arg8)+'}')
> + svntest.actions.run_and_verify_svn(None,None,[],
> + 'diff', '-r','{'+str(arg9)+'}')
> + svntest.actions.run_and_verify_svn(None,None,[],
> + 'diff', '-r','{'+str(s)+'}')
> + finally:
> + os.chdir(current_dir)
Thse tests don't actually check that the diffs are correct, they just
test that passing a single date to -r (not even a date range!) does
not immediately error. I don't think that's sufficiently useful for
us to be adding a whole new test in this file.
> ########################################################################
> #Run the tests
>
> @@ -1803,7 +1852,8 @@
> diff_within_renamed_dir,
> diff_prop_on_named_dir,
> diff_keywords,
> - diff_force
> + diff_force,
> + diff_with_time_option
> ]
>
> if __name__ == '__main__':
Please leave a comma after the last element, so the next diff can
affect only one line instead of two. (It should have been this way
before, I don't know why it was not.)
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Jun 1 00:31:23 2005