Opened 13 years ago
Closed 9 years ago
#3908 enhancement closed fixed (fixed)
Code using twisted.names.client.Resolver is hard to unit test
Reported by: | Jean-Paul Calderone | Owned by: | Richard Wall |
---|---|---|---|
Priority: | low | Milestone: | |
Component: | names | Keywords: | |
Cc: | Branch: |
branches/testable-resolver-3908
branch-diff, diff-cov, branch-cov, buildbot |
|
Author: | rwall |
Description
Resolver
uses IReactorUDP
and IReactorTime
, but also uses the global reactor and does not allow this to be overridden (using public APIs). This means any code using Resolver
will probably do actual UDP traffic and use the actual system clock. This makes it hard to test.
Parameterizing the reactor would all alternate UDP/time implementations to be supplied. This would be nice. Alternatively, Twisted Names could provide a verified fake of the Resolver
interface, and tests could use that instead of the actual Resolver
. This fake should have easily controlled behavior, including error cases.
Attachments (1)
Change History (9)
comment:1 Changed 11 years ago by
Owner: | Jean-Paul Calderone deleted |
---|
Changed 9 years ago by
Attachment: | testable-examples-6362-1.patch added |
---|
comment:2 Changed 9 years ago by
Owner: | set to Richard Wall |
---|---|
Status: | new → assigned |
I'm trying to test the source:docs/names/examples/testdns.py in #6362
I can pass a fake reactor to client.Resolver but it isn't passed to dns.DNSDatagramProtocol, so I get unclean reactor errors in the tests like exarkun predicted.
I also found an existing MemoryReactor in source:twisted/names/test/test_rootresolve.py which does implement IReactorUDP. So maybe that can be merged with the proto_helpers.MemoryReactor as part of this ticket.
See attachement:testable-examples-6362-1.patch for an example of what I am trying to do.
comment:3 Changed 9 years ago by
Author: | → rwall |
---|---|
Branch: | → branches/testable-resolver-3908 |
(In [37479]) Branching to 'testable-resolver-3908'
comment:4 Changed 9 years ago by
Keywords: | review added |
---|---|
Owner: | Richard Wall deleted |
Status: | assigned → new |
Ready for review in log:branches/testable-resolver-3908
- Build results
- Added a test that client.Resolver uses its parameterized reactor for delayed calls during resolv.conf checking and in udp query cancellation.
- Pass the parameterized reactor to the DNSDatagramProtocol to prevent it loading the global reactor.
The ticket implies that client.Resolver doesn't accept a reactor parameter. It does, but it does not pass that reactor to the DNSDatagramProtocol used in the queries.
Therefore the global reactor is used by DNSDatagramProtocol to schedule a query cancellation function.
This triggers dirty reactor warnings in trial when you attempt to test client.Resolver (or in my case, the examples that use it #6362.)
I've added some assertions to the new test method, but they seem quite tightly coupled to the implementation of client.Resolver. I wondered if it was sufficient to simply write a test which should not trigger dirty reactor warnings.
I haven't attempted to create a verified fake of IResolver as suggested in the ticket description. I don't need it for #6362 and it seems like a big enough task to split into a separate ticket. I'll create a ticket for that if the reviewer agrees.
I also didn't attempt to merge t.n.test.test_rootresolve.MemoryReactor into t.test.proto_helpers.MemoryReactor. Would that be a good thing to do? If so, I'll create another ticket for that too.
Thanks.
-RichardW.
comment:5 follow-up: 6 Changed 9 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Richard Wall |
- I think it would be better to test
_connectedProtocol
directly, rather some public thing that eventually calls it. Then you should be able to write a better assertion. - That probably wants to use
MemoryReactor
fromtest_rootresolver
. That should be merged to the coreMemoryReactor
(or better, composed with, using exarkun's idea of composable reactor bits). But the latter would be a separate ticket. - In any case, we separately want a verified fake resolver. But that should be done in a separate ticket.
comment:6 Changed 9 years ago by
Keywords: | review added |
---|---|
Owner: | Richard Wall deleted |
Ready for another review in log:branches/testable-resolver-3908:
Replying to tom.prince:
- I think it would be better to test
_connectedProtocol
directly, rather some public thing that eventually calls it. Then you should be able to write a better assertion.
Done. Much simpler.
- That probably wants to use
MemoryReactor
fromtest_rootresolver
. That should be merged to the coreMemoryReactor
(or better, composed with, using exarkun's idea of composable reactor bits). But the latter would be a separate ticket.
It was already using that version of MemoryReactor.
- In any case, we separately want a verified fake resolver. But that should be done in a separate ticket.
Raised a new ticket for that: #6379
comment:7 Changed 9 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Richard Wall |
I approve this change, please merge.
comment:8 Changed 9 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
This is the patch where I pass the reactor through