Sam TH <sam@uchicago.edu> writes:
> Here's the revised diff, having merged in all of Ben's new code. It
> includes:
>
> - Lots of helper functions
> - build_tree_from_entries()
> - a way to compare different kinds of trees
> - various formatting changes
>
> But no XFAIL stuff. More on that tommorow.
And here's a bunch o' comments. :)
> Index: svn_entry.py
> ===================================================================
Glad to see you're working on this file. This is really your domain
right now; I'm currently only interested in examining subcommand
output and disk contents. But it looks like your tree "diff" system
is going to be looking at entries files, which will be a good thing.
> + if os.path.exists(os.path.join(path, "SVN/entries")):
> + entry_path = os.path.join(path, "SVN/entries")
Just a nit here: if you have a slash in the string, it kind of
defeats the point of os.path.join. ;)
> Index: svn_test_main.py
> ===================================================================
> +def create_file(path, text):
Cool.
> +def create_dir(path):
Swell.
> def write_tree(path, lists):
Nice rewrite te use create_file() and create_dir(). Definitely more
readable.
> +def check_svn_features():
Very nice. Useful if we want to combine ra_local and ra_dav tests in
the same python file someday.
> +def checkout_wc(repos_path, module, wc_name):
OK... but this doesn't do any kind of verification, nor does it return
any value. I don't see any callers of this routine. Who's going to
use it?
> +def import_dir(repos_path, dir_path, name):
Again, no verification or return values. And it forces us into using
only the 3-arg version of 'svn import', rather than the 2-arg version.
I'm using the 2 arg verison in local_tests.guarantee_greek_repos(),
because we want 'A' and 'iota' to both exist in the repository root.
That's the way our test repositories have always been, and how we want
them to continue to be.
> +def create_greek_repos(repos_path, name="greek-tree"):
> + """Create a repository at REPOS_PATH, and import the greek tree into
> + it. The Greek Tree will be named NAME in the repository."""
> +
> + tmp_path = "tmp_greek_tree"
> +
> + create_repos(repos_path)
> + write_tree_list(tmp_path, greek_tree)
> + import_dir(repos_path, tmp_path, name)
> + remove_dir(tmp_path)
Simple and sweet... but why did you write this? Again, there's no
verification of any kind going on, and again, and no callers. Maybe
you have big future plans for this routine? Or you could just call
guarantee_greek_repos(), which already does a better job of what this
routine does. (?)
> + print "%d Passes" % passed
> + print "%d Failures" % failed
Hmm, this looks interesting. First step on the path to XFAIL?
> Index: svn_tree.py
> ===================================================================
> - if a.contents != b.contents:
> + if (a.contents != b.contents) and not((a.contents is None) or
> + (b.contents is None)):
> return 1
> - if a.props != b.props: ## is it legal to compare hashes like this?!?
> + if (a.props != b.props) and not((a.props is None) or
> + (b.props is None)):
Whooaaaaaa there. I actually object to this. Just because some
treenode's contents or props is None doesn't mean we can ignore the
comparison! For example, when I build an "expected" tree, all the dir
paths always have their contents set to None. What if the wc tree had
a file (with content) of the same name as one of the expected dirs?
We definitely want to flag a node-type difference.
What's your motivation for this change?
> + ignore_re = re.compile('^Commit.+')
> +
> for line in lines:
> + if not((ignore_re.match(line)) is None):
> + continue
I'm not sure this is necessary... since my callers always deliberately
look for a final "Commit succeeded" and pop it off the list before
calling this routine. But I guess it doesn't hurt to make things more
robust!
> +if __name__ == '__main__':
> + dump_tree(build_tree_from_wc('wc-t1'))
Let's just kill these lines. If we want to test dump_tree(), we can
do it interactively. I never meant this line to live.
> Index: xml_tests.py
> ===================================================================
Wow, you did quite a lot of work here... definitely worth saving! I
appreciate you keeping the xml tests "up to date" with the framework.
My advice, for right now, is to officially stop thinking about XML
tests. It's much, much higher priority to write local_tests.py and
dav_tests.py. We'll come back to XML testing a little later. For
now, put them on hold.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Oct 21 14:36:30 2006