A new version of the patch. Replies below.
[[[
Make the cmdline tests print out the complete actual tree and meta-
data, as python script, in case of a node mismatch in output, disk,
status or skip tree.
* subversion/tests/cmdline/svntest/tree.py
(print_script): New function in SVNTreeNode class, prints out the node
metadata as python script line.
(dump_tree_script): New function, prints a whole tree as py script,
using SVNTreeNode.print_script().
* subversion/tests/cmdline/svntest/actions.py
(run_and_verify_checkout, run_and_verify_export, verify_update,
run_and_verify_merge2, run_and_verify_commit, run_and_verify_status,
run_and_verify_unquiet_status): Print the output / disk / status / skip
tree as py script, if tree comparison throws an exception, using
dump_tree_script() from tree.py.
]]]
Julian Foad wrote:
>> I can choose to copy-and-paste the error message's printout of the
>> actually found tree into my python test script to pass the current
>> situation.
>
> ... here. That's brilliant. I have always found the test scripts' output
> a bit awkward to read, but especially I have always found it difficult
> to write the code to set up the "expected" status tree structure.
thanks :)
...
>> ACTUAL OUTPUT TREE:
>> svntest.wc.State('svn-test-work/working_copies/update_tests-9', {
>> 'A' : Item(),
>> 'A/mu' : Item(verb='Restored'),
>> 'A/D' : Item(),
>> 'A/D/G' : Item(),
>> 'A/D/G/rho' : Item(verb='Restored'),
>> 'A/D/H' : Item(status='A '),
>> 'A/D/H/chi' : Item(status='A '),
>> 'A/D/H/omega' : Item(status='A '),
>> 'A/D/H/psi' : Item(status='A '),
>> 'A/B' : Item(),
>> 'A/B/E' : Item(status='A '),
>> 'A/B/E/alpha' : Item(status='A '),
>> 'A/B/E/beta' : Item(status='A '),
>> })
>
> We can imagine further enhancements such as markers indicating which of
> these lines don't match, but this is already useful.
That would be nice.
>
>
>> [[[
>> Make the cmdline tests print out the complete actual tree and meta-
>> data, as python script, in case of a node mismatch in output, disk,
>> status or skip tree.
>>
>> * subversion/tests/cmdline/svntest/tree.py
>> (print_script): New function in SVNTreeNode class, prints out the node
>> metadata as python script line.
>> (dump_tree_script): New function, prints a whole tree as py script,
>> using print_script().
>>
>> * subversion/tests/cmdline/svntest/actions.py
>> (verify_update): Print the output or disk tree if tree comparison
>> throws an exception, using dump_tree_script() from tree.py.
>> (run_and_verify_merge2): Print the skip tree if tree comparison
>> throws an exception, using dump_tree_script() from tree.py.
>> (run_and_verify_status): Print the status tree if tree comparison
>> throws an exception, using dump_tree_script() from tree.py.
>
> It's good to avoid repetition by combining those lines like this:
>
> * subversion/tests/cmdline/svntest/actions.py
> (verify_update, run_and_verify_merge2, run_and_verify_status):
> Print the output or disk tree if tree comparison throws an
> exception, using dump_tree_script() from tree.py.
>
>
Ah, that's cool!
>
>> + def print_script(self, stream = sys.stdout, strip = 0):
>> + "Python-script-print the meta data for this node."
>
> Please make the doc-string say what each of the arguments means. (Well,
> 'self' is probably an exception. The 'stream' argument is pretty obvious
> but only because of its default value which might be changed in future,
> so it should be mentioned.)
Hm, I think I just copied the pprint() function from just above and adjusted
as needed. I never paid attention to these things... I figured it's
committed, so I can just copy it. Tidying up...
>
> Suggestion: an introductory comment for this next block of 5 lines of
> code, something like:
>
> # Strip the root node name and the requested amount from 'path'.
done.
>> + mime_type = self.props.get("svn:mime-type")
>> + if not mime_type or mime_type.startswith("text/"):
>> + if self.children is None and not self.contents is None:
>
> I don't think the "self.children is None" check helps here, and it might
> hide a bug by not showing anything if a node has both children AND
> contents. How about just "if self.contents:"?
Right, in trying to simplify, I messed up the condition from pprint().
>
>> + line += "contents=\"\"\"%s\"\"\"" % self.contents
>> + comma = True
>
> There should probable be an "else: line += 'content is binary data'".
>
>> +
>> + for name in self.atts:
>> + if comma:
>> + line += ", "
>> + line += "%s='%s'" % (name, self.atts[name])
>> + comma = True
>> +
>> + # Then, there are the props. I never needed to validate them in
>> + # any of my tests. I havent checked how they would be scripted.
>> Maybe
>> + # like self.atts above?
>> + #
>> + # for name in self.props:
>> + # if comma:
>> + # line += ", "
>> + # line += "%s='%s'" % (name, self.props[name])
>> + # comma = True
>
> Yes, I expect so, but I haven't investigated.
It's actually a dictionary parameter "props={ 'foo' : 'bar', ... }".
>
>> + line += "),"
>> + print >> stream, line
>
>
>> +def dump_tree_script(n, strip=0):
>> + "Print out a python script representation of the tree's structure."
>
> The doc-string should say what the argument "n" is and what the argument
> "strip" is.
Hm, "n" isn't documented in dump_tree, either.
Fixed mine.
>
>> + if n.children is None:
>> + tmp_children = []
>> + else:
>> + tmp_children = n.children
>
> I think this construct is normally written in Python as:
> tmp_children = n.children or []
>
> because the "or" operator says, "if the first operand is false then use
> the next operand".
I like that.
>
>> + if n.name != root_node_name:
>> + if strip < len(n.path):
>> + n.print_script(strip = strip)
>
> I'm not sure about this block. It's not printing anything about the root
> node? And it's not printing anything about nodes whose path is less than
> or equal to the "strip" argument"?
>
>> + for i in range(len(tmp_children)):
>> + c = tmp_children[i]
>> + if i == len(tmp_children
>> + )-1:
>> + dump_tree_script(c, strip=strip)
>> + else:
>> + dump_tree_script(c, strip=strip)
>
> I guess you were experimenting with omitting the trailing comma on the
> last item, or something.
>
> I think the function body all reduces to:
>
> [[[
> # print this node
> if n.name != root_node_name:
> if strip < len(n.path):
> n.print_script(strip = strip)
>
> # print this node's children
> for child in n.children or []:
> dump_tree_script(child, strip=strip)
> ]]]
You're right, that was awkward. It's a result of blindly copying the
dump_tree() function and search-replace like editing.
I have once over seriously looked at the code structure. My!
It's also matching strings now instead of relying on string lengths. That
fixed some potential problems and brought some clarity.
>
>
> I will gladly commit your patch if you can tidy up or explain these
> points.
>
> Regards,
> - Julian
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
> For additional commands, e-mail: dev-help_at_subversion.tigris.org
>
--
Neels Hofmeyr -- elego Software Solutions GmbH
Gustav-Meyer-Allee 25 / Gebäude 12, 13355 Berlin, Germany
phone: +49 30 23458696 mobile: +49 177 2345869 fax: +49 30 23458695
http://www.elegosoft.com | Geschäftsführer: Olaf Wagner | Sitz: Berlin
Handelsreg: Amtsgericht Charlottenburg HRB 77719 | USt-IdNr: DE163214194
Received on 2008-08-21 03:50:19 CEST