Review of patch in issue #2301 (test for -rDATE with diff).

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

Review of patch in issue #2301 (test for -rDATE with diff).

kfogel
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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: Review of patch in issue #2301 (test for -rDATE with diff).

Vivek Chenecharry
On Wed, 2005-06-01 at 03:22, [hidden email] 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: [hidden email]
> For additional commands, e-mail: [hidden email]


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: Review of patch in issue #2301 (test for -rDATE with diff).

kfogel
Vivek <[hidden email]> writes:
> On Wed, 2005-06-01 at 03:22, [hidden email] 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: [hidden email]
For additional commands, e-mail: [hidden email]