Opened 5 years ago

Closed 4 years ago

#6120 defect closed fixed (fixed)

twisted.web.http.Request.__repr__ is untested

Reported by: Jean-Paul Calderone Owned by:
Priority: normal Milestone:
Component: web Keywords:
Cc: jknight, moijes12 Branch: branches/test-http-request-repr-6120
branch-diff, diff-cov, branch-cov, buildbot
Author: rwall

Description

It should be tested (found while porting http.py to Python 3).

Attachments (6)

3453.patch (902 bytes) - added by moijes12 5 years ago.
6120.patch (1.8 KB) - added by moijes12 5 years ago.
6120.2.patch (1.8 KB) - added by moijes12 5 years ago.
Please ignore the previous patch but the comments remain the same.
6120.3.patch (1.9 KB) - added by moijes12 5 years ago.
6120.4.patch (1.9 KB) - added by moijes12 5 years ago.
Pardon me for putting y patches. But I've tried to put the best one in.
6120.5.patch (1.9 KB) - added by moijes12 4 years ago.
Same as 6120.patch.4 but now in test_repr, a string literal is used for comparison.

Download all attachments as: .zip

Change History (31)

comment:1 Changed 5 years ago by DefaultCC Plugin

Cc: jknight added

Changed 5 years ago by moijes12

Attachment: 3453.patch added

comment:2 Changed 5 years ago by moijes12

Keywords: review added

comment:3 Changed 5 years ago by Richard Wall

Keywords: review removed
Owner: set to moijes12

Code Review

Thanks for the patch moijes12.

It looks mostly fine. Certainly better than some of the existing repr tests I found.

But there are a few things you need to do before this can be merged...

  1. Add a News file (see wiki:ReviewProcess#Newsfiles)
  2. Remove all trailing whitespace from the patch (see http://twistedmatrix.com/documents/current/core/development/policy/coding-standard.html#auto5)
  3. Remove "Test that " from the beginning of the test docstring...its superfluous (see https://plus.google.com/115348217455779620753/posts/YA3ThKWhSAj)
  4. I suppose that now this is tested, it becomes an official part of the http.Request API...so I think you should add a short epytext docstring to http.Request.repr documenting the expected repr format.

-RichardW.

Changed 5 years ago by moijes12

Attachment: 6120.patch added

comment:4 Changed 5 years ago by moijes12

Keywords: review added
Owner: moijes12 deleted

But there are a few things you need to do before this can be merged...

Changed 5 years ago by moijes12

Attachment: 6120.2.patch added

Please ignore the previous patch but the comments remain the same.

comment:5 Changed 5 years ago by Thijs Triemstra

Why not assert with the same repr string formatting used in the actual implementation: '<%s %s %s>'% (client.method, client.uri, client.clientproto)

Changed 5 years ago by moijes12

Attachment: 6120.3.patch added

Changed 5 years ago by moijes12

Attachment: 6120.4.patch added

Pardon me for putting y patches. But I've tried to put the best one in.

comment:6 Changed 5 years ago by Tom Prince

Keywords: review removed
Owner: set to moijes12

Thanks for your persistence in working on this:

  1. While repr wants to be tested, and is part of the interface (it has to be, since every python object implements it), we don't want people depending on its contents. (It is mostly there to add debugging and/or logging). Thus, we shouldn't document the format in __repr__s docstring (doing so in the test docstring is good).
  2. The test should use a string literal, to compare the repr against, since we have enough information to do that (c.f. point 3 here).

Please resubmit after addressing the above points.

comment:7 Changed 4 years ago by moijes12

Hi

  1. Richard ( rwall ), in his comment, has asked for the format to be documented but tom.prince says it shouldn't. I am confused. What should I do?

comment:8 Changed 4 years ago by moijes12

Cc: moijes12 added

comment:9 Changed 4 years ago by Jean-Paul Calderone

While repr wants to be tested, and is part of the interface (it has to be, since every python object implements it), we don't want people depending on its contents. (It is mostly there to add debugging and/or logging). Thus, we shouldn't document the format in reprs docstring (doing so in the test docstring is good).

I disagree. And not documenting something is a strategy with a long history of total failure as a strategy for getting people not to depend on it, anyway.

Please do say something in the docstring of __repr__ about what its contents represent. You don't have to be explicit about every single tiny detail, but mentioning what information about the Request is going to presented would be nice.

Changed 4 years ago by moijes12

Attachment: 6120.5.patch added

Same as 6120.patch.4 but now in test_repr, a string literal is used for comparison.

comment:10 Changed 4 years ago by moijes12

Keywords: review added
Owner: moijes12 deleted

comment:11 Changed 4 years ago by Richard Wall

Owner: set to Richard Wall
Status: newassigned

reviewing...

comment:12 Changed 4 years ago by Richard Wall

Author: rwall
Branch: branches/test-http-request-repr-6120

(In [37802]) Branching to 'test-http-request-repr-6120'

comment:13 Changed 4 years ago by Richard Wall

(In [37803]) apply 6120.5.patch from moijes12. refs #6120

comment:14 Changed 4 years ago by Richard Wall

Owner: Richard Wall deleted
Status: assignednew

Code Review:

Thanks again moijes12 and sorry for prolonging such a simple ticket.

The latest patch looked fine to me, but when I applied it to a branch and pointed buildbot at it I noticed a Python3 test failure.

At the moment I'm not sure what the correct remedy is so I'll ask for some feedback from exarkun who knows more about the Python3 porting effort.

  1. Build results
    1. There's a Python3 build failure
      https://buildbot.twistedmatrix.com/builders/python-3.3-tests/builds/649/steps/shell/logs/stdio
      
      FAIL: test_repr (twisted.web.test.test_http.RequestTests)
      test_repr
      ----------------------------------------------------------------------
      Traceback (most recent call last):
        File "/var/lib/buildbot/buildarea/python-3_3-tests/Twisted/twisted/internet/defer.py", line 137, in maybeDeferred
          result = f(*args, **kw)
        File "/var/lib/buildbot/buildarea/python-3_3-tests/Twisted/twisted/internet/_utilspy3.py", line 41, in runWithWarningsSuppressed
          reraise(exc_info[1], exc_info[2])
        File "/var/lib/buildbot/buildarea/python-3_3-tests/Twisted/twisted/python/compat.py", line 295, in reraise
          raise exception.with_traceback(traceback)
        File "/var/lib/buildbot/buildarea/python-3_3-tests/Twisted/twisted/internet/_utilspy3.py", line 37, in runWithWarningsSuppressed
          result = f(*a, **kw)
        File "/var/lib/buildbot/buildarea/python-3_3-tests/Twisted/twisted/web/test/test_http.py", line 1665, in test_repr
          self.assertEqual(repr(request), '<GET /foo/bar HTTP/1.0>')
        File "/var/lib/buildbot/buildarea/python-3_3-tests/Twisted/twisted/trial/_synctest.py", line 356, in assertEqual
          % (msg, pformat(first), pformat(second)))
      twisted.trial.unittest.FailTest: not equal:
      a = "<b'GET' b'/foo/bar' b'HTTP/1.0'>"
      b = '<GET /foo/bar HTTP/1.0>'
      
      

There is a discussion about whether repr should return unicode or bytes here: http://stackoverflow.com/questions/3627793/best-output-type-and-encoding-practices-for-repr-functions

  1. radix (on #twisted-dev) also suggested that we raise another ticket to make the Request.repr more informative.
    19:24 < radix> rwall: hmm
    19:25 < radix> rwall: can you please include the class name and instance ID in that repr as well
    19:25 < radix> I really hate it when classes __repr__ don't include the basic info
    19:26 < radix> if you want me to put a comment on the ticket I can do that
    19:26 < radix> oh, you're reviewing, not authoring.
    19:26 < rwall> radix: Yes. That sounds like a good idea. Is it ok to change the repr without warning?
    19:27 < radix> I see. and it's not even changing the contents
    19:27 < rwall> Yep, just adding a missing test.
    19:27 < radix> rwall: yes, as tomprince said in an earlier comment on the ticket, we don't want users relying on the content of the __repr__
    19:27 < radix> but I see the content isn't even being changed in this branch, so it's probably not the appropriate branch to change it in.
    19:29 < rwall> radix: Ok, but I'll raise another ticket. What's the ideal repr? What should be the standard?
    19:30 < rwall> radix: and without wanting to dwell too long on such a small ticket...do you think __repr__ should have full docstring?
    19:32 < radix> huh, standards, that's an idea. we don't have any
    19:32 < radix> rwall: I don't think __repr__'s docstring needs to document the format absolutely. but I don't really think it matters either way.
    19:33 < rwall> ok
    19:34 < radix> Personally, off the cuff, I would suggest something like <ClassName at 0xffffffff ...> or something, where "..." could be "GET /" or                maybe even "method=GET path=/" etc
    
    

Here are some related discussions on Stackoverflow

I'll leave the ticket in review for now.

-RichardW.

comment:15 Changed 4 years ago by Julian Berman

Agreed on adding the class name.

As for the repr on Py3, I'd suggest just doing the string formatting in the test as well, i.e. `self.assertEqual(repr(request), '<%s %s %s>' % (method, uri, proto))

comment:16 Changed 4 years ago by Richard Wall

Owner: set to Richard Wall
Status: newassigned

Some further discussion on #twisted-dev. There was reluctant agreement to allow use of string template in the test.

20:34 < rwall> radix, exarkun: That new web.Request.repr test fails on Python3...a unicode vs bytes issue. If you have any suggestions can you 
               comment on #6120.
20:53 < tos9> rwall: probably just do the string formatting in the test too IMHO (I'll put that on the ticket)
20:59 < rwall> tos9: Thanks I'll try that...once I've installed Python3
21:00 < tos9> heh.
21:01 -!- zooko [~user@97-118-88-207.hlrn.qwest.net] has quit [Ping timeout: 245 seconds]
21:07 -!- trenton42 [~trenton@173-13-18-66-Michigan.hfc.comcastbusiness.net] has quit [Ping timeout: 252 seconds]
21:26 -!- z3p [~Adium@4.53.51.66] has quit [Ping timeout: 252 seconds]
21:35 < rwall> tos9: Yeah that seems like the right thing to do...and that's how the test was originally implemented...but in an earlier review he'd 
               been asked to use a string literal in the test. :)
21:38 < tos9> rwall: Ah, I see :). Well, yeah think it's reasonable to go back to string formatted.
21:40 -!- hynek [~hynek@pdpc/supporter/active/hynek] has joined #twisted-dev
21:41 < tomprince> tos9 rwall: That actually looks like the test is catching a bug in python3 support.
21:41 < tomprince> Because we don't want b'' in the repr.
21:41 < tomprince> Which is exactly why having an explicit string is better in the tests.
21:44 < rwall> tomprince: let me think about that for a moment....
21:45 < glyph> tomprince: "explicit string" sounds like a contradiction :)
21:46 < glyph> tomprince: do you mean like, nativeString?
21:49 < rwall> isn't it useful to see the value type in the repr...in fact shouldn't we be using %r so that its obvious if you've assigned a unicode 
               URI
21:50 < rwall> Python3 >>> '<Request method=%r>' % (u'GET', )
21:50 < rwall> "<Request method='GET'>"
21:50 < rwall> Python2 >>> '<Request method=%r>' % (u'GET', )

21:51 < exarkun> literal string
21:52 < exarkun> rwall: The way you find out of if you totally broke your program by passing unicode where bytes is required /should not/ be by 
                 looking at the repr of the object and scanning for u in the right place
21:53 < exarkun> I think actually "<Request method=GET>" would be better than either of those strings
21:54 < rwall> exarkun: Yeah I know, but it's still useful to see the type isn't it?
21:54 < tomprince> Why?
21:56 < exarkun> rwall: I don't know.
21:56 < exarkun> Perhaps the fact that the type is /wrong/ is what leads me to think it probably doesn't need to be reflected in the repr string
21:56 < exarkun> (GET is neither bytes nor text, it is a value in the "HTTP request method" enumeration type)
21:58 < rwall> Ideally it should be one of t.p.constant?
21:58 < exarkun> yes
22:00 < rwall> If it was, then the Request.__repr__ string format would be better as %r...I think.
22:01 < rwall> tomprince, exarkun: anyway, does this reveal a bug in Python3 support. I can't see it.
22:01  * exarkun shrugs
22:02 < exarkun> an extra b in the repr isn't the end of the world, certainly

22:02 < glyph> rwall: does a test fail? :)

22:02 < exarkun> I think it's better if it's not there, but I don't _really_ care

22:05 < rwall> glyph: It does with the current test implementation, but its simple to fix.

comment:17 Changed 4 years ago by Richard Wall

(In [37804]) revert to string template to make tests pass in python3. refs #6120

comment:18 Changed 4 years ago by Richard Wall

(In [37805]) fix some epydoc links. refs #6120

comment:19 Changed 4 years ago by Richard Wall

  • I changed the new test to use a string template again (as originally suggested by thijs and then again by julian.
  • Changed a couple of C{Request} to L{Request} / L{http.Request} so that the API docs have correct links.

I'll merge it if the Build Results look ok.

comment:20 Changed 4 years ago by Richard Wall

I raised #6391 for improving the http.Request.repr format.

comment:21 Changed 4 years ago by Richard Wall

Owner: Richard Wall deleted
Status: assignednew

comment:22 Changed 4 years ago by Richard Wall

(In [37841]) ensure all repr attributes are unicode strings for consistent output in python 2 and python3. refs #6120

comment:23 Changed 4 years ago by Richard Wall

Ready for another review in log:branches/test-http-request-repr-6120

After further discussion on #twisted-dev

  • Use t.p.compat.nativeString to ensure that the values are prefixed with b in python3.
  • Which allowed me to change the test back to use a literal string
  • Added another test to demonstrate the default Request attribute values some of which are byte strings and some of which are native strings.
16:32 < tomprince> rwall: Changing tests to make them pass makes me sad.
16:33 < tomprince> If the test is *wrong* that's one thing, but I dont think that is the case here.
16:33 < rwall> I thought there was some agreement yesterday that a string template would be ok.
16:34 < rwall> tomprince: can you suggest an alternative.
16:34 < rwall> separate python2 and python3 tests?
16:34 < tomprince> Fix the code to make the test pass?
16:35 < tomprince> Its a minor point, but I think the test *is* correct.
16:35 < rwall> alright let me take another look.
16:36  * rwall searches for other repr tests
16:38 < tomprince> point 3 at https://twistedmatrix.com/trac/ticket/5544#comment:18 talks about it.
16:45 < rwall> yep, you mentioned it yesterday. But the difference is byte strings....
16:46 < rwall> take https://twistedmatrix.com/trac/browser/trunk/twisted/web/test/test_http_headers.py?annotate=blame&rev=37416#L199 for another
               recent example of using string templates in repr tests.
16:46 < tomprince> So, fix the implementation.

17:18 < kenaan_> rwall r37841 branches/test-http-request-repr-6120/twisted/web M(http.py test_http.py): ensure all repr attributes are unicode
                 strings for consistent output in python 2 and python3. refs #6120 ...
17:19 < rwall> tomprince: ^^ How about that? It looks over complicated to me.
17:19 < itamar> rwall: you don't want it to be unicode
17:19 < itamar> you want it to be a native string
17:19 < itamar> i.e. str, i.e. bytes on python 2, unicode on python 3
17:20 < itamar> luckily there's a handy function for that, twisted.python.compat.nativeString
17:20 < exarkun> also, separately, you don't ever want bytes % unicode
17:21 < tomprince> The second assertion is reasonable, but should be a seperate test.
17:21 < rwall> This is just for consistent output in __repr__ and the http.Request defaults are not all byte strings...
17:21  * rwall looks at twisted.python.compat.nativeString
17:21 < itamar> __repr__ is supposed to return a str in python 2, i.e. bytes
17:22 < exarkun> if you return unicode, Python will implicitly encode it for you (and fail if it contains non-defaultencoding encodeable characters)
17:22 < exarkun> ideally we wouldn't ever trigger implicit encoding or decoding

17:27 < kenaan_> rwall r37842 M(branches/test-http-request-repr-6120/twisted/web/http.py): use nativeString instead ...
17:30 < kenaan_> rwall r37843 M(branches/test-http-request-repr-6120/twisted/web/test/test_http.py): a separate test for the Request default values
                 ...
17:31 < rwall> tomprince, exarkun, itamar: ok thanks. I've switched to nativeString and separated the tests. I'll put that ticket back up for review.
17:32 < tomprince> You could get away with doing the conversion inline, rather than looping and doing getattr, if you wanted. (If that was part of
                   the over-complication you were commenting about.
17:33 < rwall> yeah I guess that would be clearer now.

-RichardW.

comment:24 Changed 4 years ago by Jean-Paul Calderone

Keywords: review removed

Thanks. The discussion here has been incorporated into the tests added as part of #6391. That ticket expands the contents of the repr string and adds the necessary tests, so I'm just going to apply those changes rather than these. Thanks again for working to figure out what to do here.

comment:25 Changed 4 years ago by Jean-Paul Calderone

Resolution: fixed
Status: newclosed

(In [37977]) Merge http-request-repr-6391

Author: introom, exarkun Reviewer: exarkun, tom.prince Fixes: #6391, #6120

Expand the output of twisted.web.http.Request.__repr__ to include a bit more information. Also add unit tests for it.

Note: See TracTickets for help on using tickets.