Opened 8 years ago

Closed 4 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)

testable-examples-6362-1.patch (2.2 KB) - added by Richard Wall 4 years ago.
This is the patch where I pass the reactor through

Download all attachments as: .zip

Change History (9)

comment:1 Changed 6 years ago by <automation>

Owner: Jean-Paul Calderone deleted

Changed 4 years ago by Richard Wall

This is the patch where I pass the reactor through

comment:2 Changed 4 years ago by Richard Wall

Owner: set to Richard Wall
Status: newassigned

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 4 years ago by Richard Wall

Author: rwall
Branch: branches/testable-resolver-3908

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

comment:4 Changed 4 years ago by Richard Wall

Keywords: review added
Owner: Richard Wall deleted
Status: assignednew

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 Changed 4 years ago by Tom Prince

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

comment:6 in reply to:  5 Changed 4 years ago by Richard Wall

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

comment:7 Changed 4 years ago by Stephen Thorne

Keywords: review removed
Owner: set to Richard Wall

I approve this change, please merge.

comment:8 Changed 4 years ago by Richard Wall

Resolution: fixed
Status: newclosed

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

Note: See TracTickets for help on using tickets.