You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by kf...@collab.net on 2005/05/31 21:52:10 UTC
Review of patch in issue #2301 (test for -rDATE with diff).
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
Re: Review of patch in issue #2301 (test for -rDATE with diff).
Posted by kf...@collab.net.
Vivek <vi...@collab.net> writes:
> On Wed, 2005-06-01 at 03:22, kfogel@collab.net wrote:
> > I don't have the original post handy, but here is a review of the
> > patch attached to issue #2301.
>
> This was posted sometime back. :-)
Yes. The wheels of code review grind slow, but they grind exceeding fine.
> > 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.)
>
> Thanks for the detailed review Karl. I did this patch as a test case for
> the GNU date/time bug. Is going for a V2 worth it?
> And Thanks for committing patch of issue #2300.
If it's to test a bug for regression, then I think a V2 is worth it.
Make sure the test describes the bug it's testing for, though.
Let's not put it in diff_tests.py, since it's really got nothing to do
with testing diff. Instead, I think it would be best to make a new
date_tests.py. Even though it's only one test (for now), that would
keep things organized and clear.
Best,
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Review of patch in issue #2301 (test for -rDATE with diff).
Posted by Vivek <vi...@collab.net>.
On Wed, 2005-06-01 at 03:22, kfogel@collab.net wrote:
> I don't have the original post handy, but here is a review of the
> patch attached to issue #2301.
This was posted sometime back. :-)
>
> 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.)
Thanks for the detailed review Karl. I did this patch as a test case for
the GNU date/time bug. Is going for a V2 worth it?
And Thanks for committing patch of issue #2300.
Vivek.
>
> -Karl
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org