Re: [PATCH FOR REVIEW] add tests for svn getopt processing

Greg,

Thanks for the input. Mods made and checked in.

Best,
Blair

Greg Stein wrote:
>
> On Fri, Apr 05, 2002 at 01:38:12PM -0800, Blair Zajac wrote:
> >...
> > +++ ./subversion/tests/clients/cmdline/getopt_tests.py Fri Apr 5 13:28:03 2002
> >...
> > +def load_expected_output(basename):
> > + "load the expected standard output and standard error"
> > +
> > + stdout_filename = os.path.join(getopt_output_dir, basename + '_stdout')
> > + stderr_filename = os.path.join(getopt_output_dir, basename + '_stderr')
> > +
> > + file = open(stdout_filename, 'r')
> > + exp_stdout = file.readlines()
> > + file.close
>
> You're only referring to the file.close method -- not actually invoking it.
> You need to put parens on there: file.close()
>
> But even simpler:
>
> exp_stdout = open(stdout_filename).readlines()
>
> That opens it ('r' is default), then reads all the lines in one shot. Since
> the file object loses all references once the statement ends, it goes away
> which closes the file automatically.
>
> [ note that file.close() wasn't even needed in your function since the files
> would be closed on exit from the function since the variables referring to
> the file went out of scope ]
>
> >...
> > +replace_lines_res = [ re.compile(r'version \d+\.\d+\.\d+ '), 'version X.Y.Z '
> > + ]
> >...
> > +def search_replace_line(line):
> > + "search and replace portions of lines that may change between builds"
> > +
> > + for i in range(0, len(replace_lines_res), 2):
> > + replace_re = replace_lines_res[i]
> > + replace_str = replace_lines_res[i+1]
> > + new_line = replace_re.sub(replace_str, line)
> > + if new_line != line:
> > + return new_line
>
> It would be a lot simpler if you used a tuple in that list:
>
> replace_lines_res = [ (re.compile(...), 'version ...'),
> ]
>
> Then you can do:
>
> for replace_re, replace_str in replace_lines_res:
> new_line = replace_re...
>
> Much clearer...
>
> >...
> > +def run_one_test(sbox, basename, *varargs):
> > + "run svn with args and compare against the specified output files"
> > +
> > + if sbox.build():
> > + return 1
> > +
> > + exp_stdout, exp_stderr = load_expected_output(basename)
> > +
> > + varargs = map(None, varargs)
>
> To change the varargs tuple to a list, use: list(varargs). That is much
> clearer what your real intent is.
>
> > + varargs.insert(0, 1)
> > + actual_stdout, actual_stderr = apply(svntest.main.run_svn, varargs)
>
> Usually, when I need to add an argument like the above, I'll do the
> following:
>
> apply(svntest.main.run_svn, (1,) + varargs)
>
> This is clearer about the intent: inserting an argument at the beginning of
> whatever was passed to you.
>
> > + # Delete and perform search and replaces on the lines from the
> > + # actual and expected output that may differ between build
> > + # environments.
> > + exp_stdout = map(search_replace_line, filter(delete_line, exp_stdout))
>
> Personally, I'd find this clearer if you used one function to post-process
> the lines. That function would look something like:
>
> def process_lines(lines):
> output = [ ]
> for line in lines:
> if line is to be deleted:
> continue
> line = do the replace thing
> output.append(line)
> return output
>
> Then you just do:
>
> exp_stdout = process_lines(exp_stdout)
>
> That makes the calls clearer, reduces the two functions to one, and
> simplifies the overall logic.
>
> 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

-- 
Blair Zajac <blair@orcaware.com>
Web and OS performance plots - http://www.orcaware.com/orca/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org