Opened 3 years ago

Closed 3 years ago

#6700 defect closed duplicate (duplicate)

twisted.names.server lacks documentation and test coverage

Reported by: Richard Wall Owned by: Richard Wall
Priority: normal Milestone:
Component: names Keywords:
Cc: Branch: branches/names-server-coverage-6700
branch-diff, diff-cov, branch-cov, buildbot
Author: rwall

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

Author: rwall
Branch: branches/names-server-coverage-6700

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

comment:2 Changed 3 years ago by Richard Wall

Keywords: review added

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

comment:3 Changed 3 years ago by Richard Wall

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 3 years 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 3 years ago by Jean-Paul Calderone

Owner: set to Jean-Paul Calderone
Status: newassigned

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 Changed 3 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: changed from Jean-Paul Calderone to Richard Wall
Status: assignednew

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

Status: newassigned

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

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

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

Resolution: duplicate
Status: assignedclosed

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

Note: See TracTickets for help on using tickets.