Opened 9 years ago

Closed 8 years ago

#6121 defect closed fixed (fixed)

twisted.web.http.Request.unregisterProducer has incomplete test coverage

Reported by: Jean-Paul Calderone Owned by: Jean-Paul Calderone
Priority: normal Milestone:
Component: web Keywords:
Cc: jknight Branch: branches/request-unregisterproducer-coverage-6121
branch-diff, diff-cov, branch-cov, buildbot
Author: rwall

Description

This method is not tested at all by test_http.py. Part of it is tested by something else in twisted.web.test though (not sure by what, though). It should be completed tested by tests in test_http.py. (found while porting http.py to Python 3)

Attachments (3)

6121.patch (2.3 KB) - added by moijes12 8 years ago.
6121.2.patch (2.1 KB) - added by moijes12 8 years ago.
6121.3.patch (2.3 KB) - added by moijes12 8 years ago.
Added 6121.misc file.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 9 years ago by DefaultCC Plugin

Cc: jknight added

Changed 8 years ago by moijes12

Attachment: 6121.patch added

comment:2 Changed 8 years ago by moijes12

Keywords: review added

The attached patch contains tests for 4 scenarios :

  1. For a queued request with streaming set to false
  2. For a queued request with streaming set to true
  3. For a non-queued request with streaming set to false
  4. For a non-queued request with streaming set to true

The non-queued request tests use StringTransport (see #6401).

The set of regression tests have been run and no unexpected failures or errors are reported.

comment:3 Changed 8 years ago by Richard Wall

Owner: set to Richard Wall
Status: newassigned

Reviewing...

comment:4 Changed 8 years ago by Richard Wall

Author: rwall
Branch: branches/request-unregisterproducer-coverage-6121

(In [38230]) Branching to 'request-unregisterproducer-coverage-6121'

comment:5 Changed 8 years ago by Richard Wall

(In [38231]) Apply 6121.patch from moijes12. Refs #6121

comment:6 Changed 8 years ago by Richard Wall

Keywords: review removed
Owner: Richard Wall deleted
Status: assignednew

Code Review

Thanks moijes12 for the new tests.

Notes:

Please answer or address the following comments:

  1. Patch didn't apply cleanly - it's probably ok - but please double check.
     patch -p0 < ~/Downloads/6121.patch
     patching file twisted/web/test/test_http.py
     Hunk #2 succeeded at 1699 with fuzz 1 (offset 46 lines).
    
  2. Patch contained trailing whitespace: https://buildbot.twistedmatrix.com/builders/twistedchecker/builds/619/steps/run-twistedchecker/logs/new%20twistedchecker%20errors
  3. Missing news file - I've added a blank .misc to the branch for you.
  4. DummyRequest is imported but unused: https://buildbot.twistedmatrix.com/builders/pyflakes/builds/330/steps/pyflakes/logs/new%20pyflakes%20errors
  5. Typo "requests'" should be "request's"
  6. There is a call to unregisterProducer in _cleanup which isn't tested. I'm not sure whether that should be dealt with in this branch. Perhaps another reviewer will comment on that.
  7. Maybe test that calling unregisterProducer twice (or without having first called registerProducer) does not raise an exception.
  8. I've struggled to trace through the http.Request code, but I notice you are assigning req.transport = StringTransport() in each of your *non-queued* tests, so StringTransport.(un)registerProducer should be called by Request.(un)registerProducer in those tests, but StringTransport.(un)registerProducer don't exist. So when I try and recreate this in a python shell I get "AttributeError: '_io.BytesIO' object has no attribute 'registerProducer'". Maybe I'm just confused.

Anyway, thanks again. Please address or answer the numbered points above and submit another patch (against the branch) for review. And someone more familiar with twisted.web should probably do the next review.

-RichardW.

Changed 8 years ago by moijes12

Attachment: 6121.2.patch added

comment:7 Changed 8 years ago by moijes12

Keywords: review added

Hi

I have tried to address points 2,3,4 and 5. For 1, I'm not sure what needs to be done and as for 6,7 and 8, I'll have a look and get back later. I have attached a patch that I built against the trunk.

comment:8 Changed 8 years ago by habnabit

The most recent attached patch does not add a news file.

Changed 8 years ago by moijes12

Attachment: 6121.3.patch added

Added 6121.misc file.

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

Keywords: review removed
Owner: set to Jean-Paul Calderone
  1. For the non-queued case, False should be passed in for the queued parameter to Request, not None.
  2. A sort of nice property for test methods to have is for them to only make one assertion. That way if they fail, you know everything that's wrong right away. Otherwise, the first assertion might fail - then you fix it; then the second assertion might still fail.
  3. It would probably be a good idea to make some assertions about the transport's producer in the queued case. Otherwise the conditional in unregisterProducer isn't really being tested (it is executed but no assertions depend on it being *correct*).

These are mostly minor points. I'll address these in the branch and then look at rwall's later points. Thanks!

comment:10 Changed 8 years ago by Jean-Paul Calderone

There is a call to unregisterProducer in _cleanup which isn't tested. I'm not sure whether that should be dealt with in this branch. Perhaps another reviewer will comment on that.

I think it's alright to test that later on. This ticket can focus on just the implementation of unregisterProducer.

Maybe test that calling unregisterProducer twice (or without having first called registerProducer) does not raise an exception.

Well.. maybe. It's sort of tempting. On the other hand, IConsumer doesn't specify what happens when you try to unregister a producer when one isn't registered. Some implementations silently accept it as a no-op, others don't (for example, StringTransport raises an exception :).

I feel like leaving this issue alone for now.

I've struggled to trace through the http.Request code, but I notice you are assigning req.transport = StringTransport() in each of your *non-queued* tests, so StringTransport.(un)registerProducer should be called by Request.(un)registerProducer in those tests, but StringTransport.(un)registerProducer don't exist. So when I try and recreate this in a python shell I get "AttributeError: '_io.BytesIO' object has no attribute 'registerProducer'". Maybe I'm just confused.

Hm? StringTransport.unregisterProducer does exist. Maybe you picked up StringTransport from the wrong place? (Notice it is from twisted.test.proto_helpers, not twisted.web.http)

comment:11 Changed 8 years ago by Jean-Paul Calderone

Resolution: fixed
Status: newclosed

(In [39881]) Merge request-unregisterproducer-coverage-6121

Author: moijes12, exarkun Reviewer: rwall, exarkun Fixes: #6121

Add some missing test coverage for twisted.web.http.Request.unregisterProducer.

Note: See TracTickets for help on using tickets.