Code using twisted.names.client.Resolver is hard to unit test

[Jean-Paul Calderone]

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.

[Richard Wall]

This is the patch where I pass the reactor through

[Richard Wall]

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.

[Richard Wall]

(In [37479]) Branching to 'testable-resolver-3908'

[Richard Wall]

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.

[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.
• That probably wants to use MemoryReactor from test_rootresolver. That should be merged to the core MemoryReactor (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.

[Richard Wall]

Ready for another review in log:branches/testable-resolver-3908:

• ​Build Results

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 from test_rootresolver. That should be merged to the core MemoryReactor (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

[Stephen Thorne]

I approve this change, please merge.

[Richard Wall]

(In [37639]) Merge testable-resolver-3908: Pass the parameterized reactor to the DNSDatagramProtocol to allow easier testing.

Author: rwall Reviewer: tom.prince, jerub Fixes: #3908

Pass the parameterized reactor to the DNSDatagramProtocol to allow easier testing.