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

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