Opened 5 years ago

Last modified 4 years ago

#6118 defect new

twisted.web.http.Request.noLongerQueued has no test coverage

Reported by: Jean-Paul Calderone Owned by: moijes12
Priority: normal Milestone:
Component: web Keywords:
Cc: jknight Branch: branches/Request.noLongerQueued-test-coverage-6118
branch-diff, diff-cov, branch-cov, buildbot
Author: moijes12

Description

It should have complete test coverage (found while porting http.py to Python 3).

Attachments (2)

6118.patch (673 bytes) - added by moijes12 5 years ago.
Written a test for a scenario where it is called for an object that is not in queued results in a RuntimeError being raised.
6118.2.patch (2.7 KB) - added by moijes12 5 years ago.
Made some additions.

Download all attachments as: .zip

Change History (11)

comment:1 Changed 5 years ago by DefaultCC Plugin

Cc: jknight added

Changed 5 years ago by moijes12

Attachment: 6118.patch added

Written a test for a scenario where it is called for an object that is not in queued results in a RuntimeError being raised.

comment:2 Changed 5 years ago by moijes12

Keywords: review added

comment:3 Changed 5 years ago by Tom Prince

Author: tomprince
Branch: branches/Request.noLongerQueued-test-coverage-6118

(In [36871]) Branching to Request.noLongerQueued-test-coverage-6118.

comment:4 Changed 5 years ago by Tom Prince

Keywords: review removed
Owner: set to moijes12

Thanks for working on this. The test you added looks good, but could be improved by checking the message of the exception (there are a bunch of examples throughout the existing tests).

This test covers one case, but there are a bunch more need. Off hand:

  1. That data is written to the channel transport if-and-only-if there is existing data).
  2. If there is a producer, than it is properly registered with the transport (which may trigger writes)
  3. If the request if finished, then cleanup is done.

Each of those probably deserves at least a couple of test cases.

Changed 5 years ago by moijes12

Attachment: 6118.2.patch added

Made some additions.

comment:5 Changed 5 years ago by moijes12

Keywords: review added
Owner: moijes12 deleted

comment:6 Changed 5 years ago by Tom Prince

Branch: branches/Request.noLongerQueued-test-coverage-6118branches/noLongerQueued-tests-6118

(In [37072]) Branching to defer-passthru-helper-6118.

comment:7 Changed 5 years ago by Tom Prince

Keywords: review removed
Owner: set to moijes12

Thanks. There are a couple of more things that it would be worthwhile to test.

  1. noLongerQueued switches the transport of the request from an in-memory transport to the one provided by the channel. There should be a test that verifies this.
    1. Relatedly, the tests that assert that a producer is hooked up to a transport could check that they are hooked up to the channel transport, rather than whatever transport request ends up pointing to.
  2. I'm not sure what test_noLongerQueuedWriteIfData is trying to test, but I think it wants to test that the channel transport has the data that was originally written to the temporary request transport. (DummyChannel has a test transport which can be used to check this).
  3. There should be a test that ensure that the request is no longer queued, afterwards.
  4. test_noLongerQueuedCleanupIfFinished could be improved to use .notifyFinish() and 'TestCase.successResultOf to verify that _cleanup gets called. notifyFinish` is something that is definitely going to happen, whereas the other could change.
  5. (minor) .finished should be set to True rather than 1. (I suspect this code predates the introduction of True)

Please resubmit for review, after addressing the above points.

comment:8 Changed 5 years ago by Tom Prince

Author: tomprincemoijes12
Branch: branches/noLongerQueued-tests-6118branches/Request.noLongerQueued-test-coverage-6118

(fixing branch info)

comment:9 Changed 4 years ago by moijes12

  1. test_noLongerQueuedCleanupIfFinished could be improved to use .notifyFinish() and 'TestCase.successResultOf to verify that _cleanup gets called. notifyFinish` is something that is definitely going to happen, whereas the other could change.

[moijes12] est_noLongerQueuedCleanupIfFinished verifyies that _cleanup gets called by checking that self.content is deleted.

  1. I'm not sure what test_noLongerQueuedWriteIfData is trying to test, but I think it wants to test that the channel transport has the data that was originally written to the temporary request transport. (DummyChannel has a test transport which can be used to check this).

[moijes12] The function documentation of test_noLongerQueuedWriteIfData clearly mentions its purpose. Also, I cannot find the test transport method in twisted.web.test.requesthelper.DummyChannel.

Note: See TracTickets for help on using tickets.