On Wed, 2008-08-27 at 04:03 +0200, Neels Hofmeyr wrote:
> Let me illustrate further...
>
> If you go to trunk's subversion/tests/cmdline/update_tests.py file and edit:
>
> Index: update_tests.py
> ===================================================================
> --- update_tests.py (revision 32743)
> +++ update_tests.py (working copy)
> @@ -152,7 +152,7 @@
> # look! binary contents, and a binary property!
> expected_disk = svntest.main.greek_state.copy()
> expected_disk.add({
> - 'A/theta' : Item(theta_contents_local,
> + 'A/theta' : Item(
> props={'svn:mime-type' : 'application/octet-stream'}),
> })
>
> , i.e. "forget to insert" binary content for the expected disk node. This is
> in update_tests.py 1.
>
> If you then run update_tests.py 1, you get:
>
>
> [[[
> $ ./update_tests.py 1
> =============================================================
> Expected 'theta' and actual 'theta' in disk tree are different!
> =============================================================
> EXPECTED NODE TO BE:
> =============================================================
> * Node name: theta
> Path: __SVN_ROOT_NODE/A/theta
> UNEXPECTED EXCEPTION:
> Traceback (most recent call last):
> File "/arch/elego/svn/trunk/subversion/tests/cmdline/svntest/main.py",
> line 1116, in run
> rc = apply(self.pred.run, (), kw)
> File "/arch/elego/svn/trunk/subversion/tests/cmdline/svntest/testcase.py",
> line 121, in run
> return self.func(sandbox)
> File "./update_tests.py", line 183, in update_binary_file
> None, None, 1)
> File "/arch/elego/svn/trunk/subversion/tests/cmdline/svntest/actions.py",
> line 740, in run_and_verify_update
> check_props)
> File "/arch/elego/svn/trunk/subversion/tests/cmdline/svntest/actions.py",
> line 644, in verify_update
> singleton_handler_b, b_baton)
> File "/arch/elego/svn/trunk/subversion/tests/cmdline/svntest/tree.py",
> line 582, in compare_trees
> singleton_handler_b, b_baton)
> File "/arch/elego/svn/trunk/subversion/tests/cmdline/svntest/tree.py",
> line 582, in compare_trees
> singleton_handler_b, b_baton)
> File "/arch/elego/svn/trunk/subversion/tests/cmdline/svntest/tree.py",
> line 556, in compare_trees
> display_nodes(a, b)
> File "/arch/elego/svn/trunk/subversion/tests/cmdline/svntest/tree.py",
> line 538, in display_nodes
> b.pprint()
> File "/arch/elego/svn/trunk/subversion/tests/cmdline/svntest/tree.py",
> line 196, in pprint
> print >> stream, " Contents: %d bytes (binary)" % len(self.contents)
> TypeError: object of type 'NoneType' has no len()
> FAIL: update_tests.py 1: update a locally-modified binary file
> ]]]
>
>
> Note that the exception *interrupts* the normal error output. Usually, it
> would print some more info on the expected node, followed by the actual
> node. This exception though complains about a NoneType having no len().
>
> This is an exception that happens while trying to describe another exception
> that has already happened and needs attention. It's useless information
> obstructing the useful information from the test developer.
>
>
> After my proposed patch is applied, the output of update_tests.py 1 is:
>
> [[[
> $ ./update_tests.py 1
> =============================================================
> Expected 'theta' and actual 'theta' in disk tree are different!
> =============================================================
> EXPECTED NODE TO BE:
> =============================================================
> * Node name: theta
> Path: __SVN_ROOT_NODE/A/theta
> Contents: 0 bytes (binary)
> Properties: {'svn:mime-type': 'application/octet-stream'}
> Attributes: {}
> Children: None (node is probably a file)
> =============================================================
> ACTUAL NODE FOUND:
> =============================================================
> * Node name: theta
> Path: __SVN_ROOT_NODE/A/theta
> Contents: 1396 bytes (binary)
> Properties: {'svn:mime-type': 'application/octet-stream'}
> Attributes: {}
> Children: None (node is probably a file)
> Unequal at node theta
> Unequal at node A
> ACTUAL DISK TREE:
> svntest.wc.State('', {
> 'iota' : Item(contents="This is the file 'iota'.\n"),
> 'A' : Item(),
> 'A/theta.r3' : Item(contents="[ <comment: This actually prints a
> lot of binary garbage because the test does not set the mime type correctly.
> It's got nothing to do with this patch though.> ]\nrevision 3 text"),
> 'A/theta' : Item(content is binary data,
> props={'svn:mime-type':'application/octet-stream'}),
> 'A/theta.r2' : Item(contents="[ <comment: more binary, s.a.> ]\n"),
> 'A/mu' : Item(contents="This is the file 'mu'.\n"),
> 'A/C' : Item(),
> 'A/B' : Item(),
> 'A/B/lambda' : Item(contents="This is the file 'lambda'.\n"),
> 'A/B/F' : Item(),
> 'A/B/E' : Item(),
> 'A/B/E/beta' : Item(contents="This is the file 'beta'.\n"),
> 'A/B/E/alpha' : Item(contents="This is the file 'alpha'.\n"),
> 'A/D' : Item(),
> 'A/D/gamma' : Item(contents="This is the file 'gamma'.\n"),
> 'A/D/G' : Item(),
> 'A/D/G/tau' : Item(contents="This is the file 'tau'.\n"),
> 'A/D/G/pi' : Item(contents="This is the file 'pi'.\n"),
> 'A/D/G/rho' : Item(contents="This is the file 'rho'.\n"),
> 'A/D/H' : Item(),
> 'A/D/H/chi' : Item(contents="This is the file 'chi'.\n"),
> 'A/D/H/psi' : Item(contents="This is the file 'psi'.\n"),
> 'A/D/H/omega' : Item(contents="This is the file 'omega'.\n"),
> })
> EXCEPTION: SVNTreeUnequal
> Traceback (most recent call last):
> File "/arch/elego/svn/trunk/subversion/tests/cmdline/svntest/main.py",
> line 1116, in run
> rc = apply(self.pred.run, (), kw)
> File "/arch/elego/svn/trunk/subversion/tests/cmdline/svntest/testcase.py",
> line 121, in run
> return self.func(sandbox)
> File "./update_tests.py", line 183, in update_binary_file
> None, None, 1)
> File "/arch/elego/svn/trunk/subversion/tests/cmdline/svntest/actions.py",
> line 740, in run_and_verify_update
> check_props)
> File "/arch/elego/svn/trunk/subversion/tests/cmdline/svntest/actions.py",
> line 644, in verify_update
> singleton_handler_b, b_baton)
> File "/arch/elego/svn/trunk/subversion/tests/cmdline/svntest/tree.py",
> line 583, in compare_trees
> singleton_handler_b, b_baton)
> File "/arch/elego/svn/trunk/subversion/tests/cmdline/svntest/tree.py",
> line 583, in compare_trees
> singleton_handler_b, b_baton)
> File "/arch/elego/svn/trunk/subversion/tests/cmdline/svntest/tree.py",
> line 558, in compare_trees
> raise SVNTreeUnequal
> SVNTreeUnequal
> FAIL: update_tests.py 1: update a locally-modified binary file
> ]]]
>
>
> Now the exception is, sensibly, SVNTreeUnequal, and node information is
> printed out to the end. It is visible that the actual node has 1396 bytes of
> binary data, while the expected node has no data (0 bytes).
>
> Do you see my point now? :)
No, not really. I see what is heppening, I just don't think it is an
improvement. Sorry.
- Julian
> Julian Foad wrote:
> > On Thu, 2008-08-21 at 23:13 +0200, Neels Janosch Hofmeyr wrote:
> >> Julian Foad wrote:
> >>> On Thu, 2008-08-21 at 03:53 +0200, Neels Janosch Hofmeyr wrote:
> >>>> [[[
> >>>> * subversion/tests/cmdline/svntest/tree.py (pprint): Fix failure to get
> >>>> len(contents) for error output, for the case that contents is `None'.
> >>>> ]]]
> >>> Neels, this patch fixes the case where the MIME type is set to something
> >>> (but not something beginning with "text/") and content is None. If the
> >>> MIME type is set, I don't think it makes sense for the content to be
> >>> None. If some caller is setting content to None, maybe the caller is
> >>> broken.
> >>>
> >>> There might be some complication, like if the object being represented
> >>> is a deleted file, and therefore its content is None, and for some
> >>> reason the properties have not been deleted. (There is currently a bug
> >>> in Subversion whereby the working properties can still be accessed by
> >>> some of svn's property commands when an object is scheduled for
> >>> deletion. Maybe that sort of thing is happening here.)
> >>>
> >>> Does that make sense? Did you find a case where this happens?
> >> Well, I had the simple case where I tried to write a test with binary data,
> >> and I set the MIME type property, but forgot to add binary `content'.
> >> `content' was None, the node mismatched, it tried to len() it, exception.
> >>
> >> With the fix, instead of aborting mid-sentence and printing a test-unrelated
> >> exception, it prints something like "contents: 0 bytes binary data" and
> >> continues with the rest of the normal error output.
> >>
> >> It's a corner case where one wrote a dumb test, but I'd say it should be
> >> reported in a useful manner.
> >
> > To me it seems like adding explicit checks for this would be against the
> > spirit of Python (in which errors like this are caught by exceptions),
> > so I don't want to make this change.
> >
> > However, I wouldn't stop somebody else from doing it if they feel it
> > really is worth it.
> >
> > Thanks.
> > - Julian
> >
> >
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-08-28 13:40:05 CEST