Re: [PATCH] tests: fix len(contents) for error output, if contents is None

From: Neels Hofmeyr <neels_at_elego.de>
Date: Wed, 27 Aug 2008 04:03:43 +0200

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:

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

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

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

application/pgp-signature attachment: OpenPGP digital signature

Received on 2008-08-27 04:04:33 CEST

This message: [ Message body ]
Next message: Jeremy Whitlock: "[RFC] Issue 2269 - svn st|up Do Not Warn When Ran Against a Nonexistent Path"
Previous message: Paul Burba: "Re: svn commit r32706/32707/32730: Fix yet another issue #3067 bug"
In reply to: Julian Foad: "Re: [PATCH] tests: fix len(contents) for error output, if contents is None"
Next in thread: Julian Foad: "Re: [PATCH] tests: fix len(contents) for error output, if contents is None"
Reply: Julian Foad: "Re: [PATCH] tests: fix len(contents) for error output, if contents is None"

Contemporary messages sorted: [ by date ] [ by thread ] [ by subject ] [ by author ] [ by messages with attachments ]