Re: Lots of test suite code

On Tue, May 08, 2001 at 05:46:19PM -0500, Ben Collins-Sussman wrote:
> 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.

Soon. I promise. :-)

>
>
> And here's a bunch o' comments. :)
>

Thanks.

>
> > + 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. ;)

Yeah, Greg's solution is the correct one here. I just didn't know join
was varargs, and got lazy.

> > +def check_svn_features():
>
> Very nice. Useful if we want to combine ra_local and ra_dav tests in
> the same python file someday.

Well, I think that will be neccessary. We will definitely end up with
more tests than we want to maintain in just two files. At over 100
lines of code per test (348/3 for local_tests [1]) it could get big
quick.

[1] Yeah, I know that's a lousy stat.

>
> > +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?

It was getting used in bug_tests.py, which I sent a while back. I
think you have some routines in local_tests which do the same thing.
These should really be abstracted out, so the DAV stuff can use them
as well.

I'll fix the return value stuff.

>
> > +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.
>

Makes sense. I'd still like to have a function to do this, suggestions?

> > +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. (?)
>

Well, I wrote this since there was a comment that said we needed it.
:-)

guarantee_greek_repos is better, but it didn't exist when I wrote
that. :-)

>
> > + print "%d Passes" % passed
> > + print "%d Failures" % failed
>
> Hmm, this looks interesting. First step on the path to XFAIL?
>

Well, I wrote it at the same time, but it's useful anyway, without any
additions.

>
> > 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?
>

The motivation for this change is to allow the kind of stuff I wrote
in basic_tests.py (at least, I think I used it). The idea is to
compare trees generated w/o contents (like from entries files) to
trees generated w/o props (like from working copies). Or other such
comparisons.

You are right, however, that this implementation is broken, since it
will confuse a directory with a file from a contents-less tree. I'll
try to think of a better way to do this. Maybe we can add some sort
of a.dir attribute to fix this. However, any node w/ props == None is
generated from a method that didn't have access to props. Otherwise,
the degenerate case is { }.

Does that make sense?

>
> > + 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!
>

Robustness good. Actually, the idea was to simplify the callers, to
make test writing as easy as possible.

>
> > +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.
>

Good idea.

>
> > 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.

Will do.

Hopefully more code soon.
           
sam th --- sam_at_uchicago.edu --- http://www.abisource.com/~sam/
OpenPGP Key: CABD33FC --- http://samth.dyndns.org/key
DeCSS: http://samth.dynds.org/decss