#6700 defect closed duplicate (duplicate)

twisted.names.server lacks documentation and test coverage

Reported by: rwall Owned by: rwall
Priority: normal Milestone:
Component: names Keywords:
Cc: Branch: branches/names-server-coverage-6700
(diff, github, buildbot, log)
Author: rwall Launchpad Bug:

Description

In #6645 I want to make a slight change to the behaviour of DNSServerFactory, but I found the existing tests and documentation very sparse.

Add tests and docstrings before making changes.

Change History (10)

comment:1 Changed 15 months ago by rwall

  • Author set to rwall
  • Branch set to branches/names-server-coverage-6700

(In [39729]) Branching to 'names-server-coverage-6700'

comment:2 Changed 15 months ago by rwall

  • Keywords review added

Ready for review in log:branches/names-server-coverage-6700

comment:3 follow-up: Changed 15 months ago by rwall

One thing I forgot to ask. Does the new test_server module need adding to the list of Python3 tests? test_names is not currently run on Python3 so I assumed not.

comment:4 in reply to: ↑ 3 Changed 12 months ago by glyph

Replying to rwall:

One thing I forgot to ask. Does the new test_server module need adding to the list of Python3 tests? test_names is not currently run on Python3 so I assumed not.

It would really save a lot of headaches if all new code were py3k-ready.

comment:5 Changed 11 months ago by exarkun

  • Owner set to exarkun
  • Status changed from new to assigned

It would really save a lot of headaches if all new code were py3k-ready.

True, although the cost may be headaches in making the new code py3k-ready. ;)

comment:6 follow-up: Changed 11 months ago by exarkun

  • Keywords review removed
  • Owner changed from exarkun to rwall
  • Status changed from assigned to new

Thanks.

  1. twisted/names/server.py
    1. DNSServerFactory.cache can also be None if no caches are specified.
    2. For DNSServerFactory.clients, when referencing an interface, I prefer to always be explicit about "implementer" or "provider". This is the vocabulary zope.interface uses. Typically the class is an implementer and instances of the class are providers (but things can be defined in other ways using other declaration apis from zope.interface). Examples of this ambiguity appear elsewhere in the diff as well.
    3. I don't think the @type for DNSServerFactory.protocol adds much to the @ivar. I even find it a bit confusing if interpreted literally: the callable should not actually return IProtocolFactory. It should return an IProtocolFactory provider. Also L{callable} is incorrect here. The callable builtin is just some random other API not related to the type of this instance attribute.
    4. If you care about Python 3 compat, then the signature of DNSServerFactory.gotResolverResponse is a problem (tuple unpacking in argument lists was removed from the language). Not new in this branch but the function is being changed here.
    5. In DNSServerFactory.gotResolverError, @param failure: A C{failure} ... - I'm not sure what useful information C{failure} is conveying. You might want to move the L{...} from the @type failure into the @param to replace C{failure}. Or you could be more abstract - eg, The reason for the failed resolution (as reported by C{self.resolver.query}). There are other instances of this elsewhere in the patch as well, eg C{deferred} in handleQuery.
  2. twisted/names/test/test_server.py
    1. Maybe NoresponseDNSServerFactory should be NoResponseDNSServerFactory.
    2. A pass statement following a docstring is superfluous - eg as in AllowQueryArguments and NoresponseDNSServerFactory.sendReply.
    3. The API documentation for NoresponseDNSServerFactory.allowQuery's args parameter seems somewhat silly. It *must* be positional arguments because it is *args. I don't think this is worth saying in a docstring. Relatedly, though, it seems like it might be better to define these methods with the correct signature anyway.
    4. All new "root" classes should be new-style (regardless of Python 3 considerations) unless they are helpers for testing behavior related to classic classes (iow always subclass object if you're not subclassing something else). It may also make sense for non-root classes to be forced to be new-style as well (mix object in if you're subclassing a classic class) but this is slightly more complicated so I'm not going to make a blanket statement about it.
    5. No "simple suites" - eg, no class DummyAuthority: pass.
    6. I think the coding standard says to use assertIs instead of assertIdentical now.
    7. test_cacheOverride doesn't actually demonstrate the first element is used as the cache (it could easily by the last element or the middle element or a random element and the test would still pass).
    8. The assertIsInstance in test_buildProtocolProtocolOverride seems redundant with the assertion made by test_buildProtocolDefaultProtocolType. Also I find that test methods with only one assertion create a higher quality test suite than test methods with multiple assertions.
  3. The wrapping applied throughout seems to be too narrow.

I think this are largely minor comments. I may have started to lose focus towards the end, though. It might have been nice to have separate tickets for the documentation and testing parts of this to avoid having any >800 LOC reviewing to do (particularly given the repetitive nature of large parts of this patch (necessitated by the unfortunately repetitive nature of the API being documented) - reviewing big blocks of identical or nearly identical documentation is probably one of the harder things to do).

I just initiated a build. Please check that for more issues that our tools catch. Please submit for review again after addressing any of those and the above points. Thanks again!

comment:7 in reply to: ↑ 6 Changed 11 months ago by rwall

  • Status changed from new to assigned

Replying to exarkun:

I think this are largely minor comments. I may have started to lose focus towards the end, though. It might have been nice to have separate tickets for the documentation and testing parts of this to avoid having any >800 LOC reviewing to do (particularly given the repetitive nature of large parts of this patch (necessitated by the unfortunately repetitive nature of the API being documented) - reviewing big blocks of identical or nearly identical documentation is probably one of the harder things to do).

I agree / sympathise.

Created #6886 for missing api documentation and #6887 for missing test coverage.

comment:8 Changed 11 months ago by rwall

(In [41100]) Merge names-server-documentation-6886

Author: rwall
Reviewer: exarkun
Fixes: #6886
Refs: #6700

Add API documentation to twisted.names.server

comment:9 Changed 11 months ago by rwall

(In [41109]) Merge names-server-coverage-6887-2

Author: rwall
Reviewer: exarkun
Fixes: #6887
Refs: #6700

Add test coverage for twisted.names.server

comment:10 Changed 11 months ago by rwall

  • Resolution set to duplicate
  • Status changed from assigned to closed

This ticket was broken into two separate branches which have now been merged. Referenced above.

Note: See TracTickets for help on using tickets.