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

[twisted-trac]

	@exarkun reported
Trac ID	trac#6118
Type	defect
Created	2012-10-16 13:08:03Z
Branch	https://github.com/twisted/twisted/tree/Request.noLongerQueued-test-coverage-6118

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

Attachments:

• 6118.patch (673 bytes) - added by moijes12 on 2013-01-20 11:51:38Z - 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 (2745 bytes) - added by moijes12 on 2013-01-23 21:21:44Z - Made some additions.

Searchable metadata

``` trac-id__6118 6118 type__defect defect reporter__exarkun exarkun priority__normal normal milestone__ branch__branches_Request_noLongerQueued_test_coverage_6118 branches/Request.noLongerQueued-test-coverage-6118 branch_author__moijes12 moijes12 status__new new resolution__None None component__web web keywords__ time__1350392883000000 1350392883000000 changetime__1374175473000000 1374175473000000 version__None None owner__moijes12 moijes12 cc__jknight ```

[twisted-trac]

	@tomprince set owner 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.

[twisted-trac]

	@moijes12 removed owner

[twisted-trac]

	@tomprince set owner 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.successResultOfto verify that_cleanupgets 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.

[twisted-trac]

	@tomprince commented

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

[twisted-trac]

	@tomprince commented

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

[twisted-trac]

	@tomprince commented

(fixing branch info)

[twisted-trac]

	@moijes12 commented

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.

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

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

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