Opened 10 years ago
Last modified 9 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)
Change History (11)
comment:1 Changed 10 years ago by
Cc: | jknight added |
---|
Changed 9 years ago by
Attachment: | 6118.patch added |
---|
comment:2 Changed 9 years ago by
Keywords: | review added |
---|
comment:3 Changed 9 years ago by
Author: | → tomprince |
---|---|
Branch: | → branches/Request.noLongerQueued-test-coverage-6118 |
(In [36871]) Branching to Request.noLongerQueued-test-coverage-6118.
comment:4 Changed 9 years ago by
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:
- That data is written to the channel transport if-and-only-if there is existing data).
- If there is a producer, than it is properly registered with the transport (which may trigger writes)
- If the request if finished, then cleanup is done.
Each of those probably deserves at least a couple of test cases.
comment:5 Changed 9 years ago by
Keywords: | review added |
---|---|
Owner: | moijes12 deleted |
comment:6 Changed 9 years ago by
Branch: | branches/Request.noLongerQueued-test-coverage-6118 → branches/noLongerQueued-tests-6118 |
---|
(In [37072]) Branching to defer-passthru-helper-6118.
comment:7 Changed 9 years ago by
Keywords: | review removed |
---|---|
Owner: | set to moijes12 |
Thanks. There are a couple of more things that it would be worthwhile to test.
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.- 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.
- 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). - There should be a test that ensure that the request is no longer queued, afterwards.
test_noLongerQueuedCleanupIfFinished
could be improved to use.notifyFinish()
and 'TestCase.successResultOfto verify that
_cleanupgets called.
notifyFinish` is something that is definitely going to happen, whereas the other could change.- (minor)
.finished
should be set toTrue
rather than1
. (I suspect this code predates the introduction ofTrue
)
Please resubmit for review, after addressing the above points.
comment:8 Changed 9 years ago by
Author: | tomprince → moijes12 |
---|---|
Branch: | branches/noLongerQueued-tests-6118 → branches/Request.noLongerQueued-test-coverage-6118 |
(fixing branch info)
comment:9 Changed 9 years ago by
- 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.
- 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.
Written a test for a scenario where it is called for an object that is not in queued results in a RuntimeError being raised.