Opened 8 years ago

Closed 8 years ago

#1929 defect closed fixed (fixed)

twisted.web2.test.test_xmlrpc should not use twisted.web.xmlrpc.Proxy

Reported by: dreid Owned by:
Priority: high Milestone: Web2-Gold-Master
Component: web2 Keywords:
Cc: oubiwann, dreid, exarkun Branch:
Author: Launchpad Bug:

Description (last modified by oubiwann)

These tests are essentially invalid and should be rewritten:

  1. They use the network and don't disconnect properly (#1906)
  2. Both the client and the server use xmlrpclib so if it's broken they can still pass

Attachments (2)

1929.xmlrpc.patch (2.5 KB) - added by oubiwann 8 years ago.
1929.test_xmlrpc.patch (9.9 KB) - added by oubiwann 8 years ago.

Download all attachments as: .zip

Change History (23)

comment:1 Changed 8 years ago by oubiwann

  • Cc oubiwann added

comment:2 Changed 8 years ago by oubiwann

  • Milestone set to Web2-Gold-Master

comment:3 Changed 8 years ago by oubiwann

  • Cc dreid added
  • Status changed from new to assigned

More explanation from dreid on IRC tonight:

[9:12pm] dreid: your tests are still all over the network :(
[9:13pm] oubiwann: yeah, 1) I'm not even looking at netw vs. nonnetw tests
         yet (just focusing on proper disconnects)
[9:13pm] oubiwann: and 2) I'm doing that work in a separate branch
[9:13pm] dreid: well if you don't hit the network you don't have to worry 
         about disconnects.
[9:14pm] oubiwann: I'm not convinced that not hitting the network is a good 
         thing...
[9:14pm] oubiwann: but I'm willing to be
[9:23pm] dreid: :\
[9:24pm] dreid: for one thing, all the tests for the client include talking 
         to the server, and all your tests for the server inclue talking to 
         the client.
[9:25pm] dreid: you're not really proving that either one is correct.
[9:25pm] oubiwann: ah, I see what you were saying in the tracker item now...

comment:4 Changed 8 years ago by oubiwann

  • Priority changed from normal to high
  • Type changed from enhancement to defect

comment:5 Changed 8 years ago by oubiwann

I'm about half-way through rewriting the tests. The parts that are rewritten do not use the network, though they are still using xmlrpclib to do the heavy lifting of generating payload XML.

dreid and I discussed this for a while on IRC last night (the fact that xmlrpclib is being trusted to work). One suggestion I offered is that, in the event that it is important to prove the trustworthiness of xmlrpclib, we write an additional unittest at the top of t.w2.xmlrpc specifically for testing xmlrpclib's XML-generation and parsing capabilities. With that done, we could then feel free to use xmlrpclib in the rest of the tests instead of building all the XML by hand.

comment:6 Changed 8 years ago by oubiwann

Converted the first introspection unit test (the last test class that needs converting); 2 more introspection tests to go (and then docstrings). Hopefully will finish this tonight.

comment:7 Changed 8 years ago by oubiwann

  • Keywords review added
  • Owner changed from oubiwann to dreid
  • Status changed from assigned to new

David, patches have been uploaded (I don't have branch commit access). They are ready for your comments/suggestions/etc.

comment:8 Changed 8 years ago by exarkun

  • Cc exarkun added
  • Keywords review removed
  • Owner changed from dreid to oubiwann

Some simple initial comments:

  • test_xmlrpc.py now imports t.web2.channel.http and t.web2.client.http. Some other facts pointed out by pyflakes:
    twisted/web2/test/test_xmlrpc.py:12: 'server' imported but unused
    twisted/web2/test/test_xmlrpc.py:14: redefinition of unused 'http' from line 13
    twisted/web2/test/test_xmlrpc.py:14: 'http' imported but unused
    twisted/web2/test/test_xmlrpc.py:17: 'unittest' imported but unused
    twisted/web2/test/test_xmlrpc.py:18: 'reactor' imported but unused
    twisted/web2/test/test_xmlrpc.py:21: 'http_headers' imported but unused
    twisted/web2/test/test_xmlrpc.py:22: 'ClientTests' imported but unused
    twisted/web2/test/test_xmlrpc.py:22: 'TestServer' imported but unused
    
    These should all be cleaned up.
  • There are some relative imports which should be changed to absolute imports. Relative imports aren't allowed in Twisted.
  • \-style line continuations are also fairly undesirable. These should use triple-quoted strings or unbalanced parenthesis instead.
  • There's a debug print in a nested function which isn't even used
  • Test methods named like test_MethodHelp should be test_methodHelp instead.
  • wait_timneout violates the naming convention and also seems to be completely ignored. What's the point of this?

comment:9 Changed 8 years ago by oubiwann

  • Keywords review added
  • Owner changed from oubiwann to exarkun

Replying to exarkun:

Some simple initial comments:

  • Some other facts pointed out by pyflakes:

[snip]

Pyflakes now reports no badness for test/test_xmlrpc.py

  • There are some relative imports which should be changed to absolute imports. Relative imports aren't allowed in Twisted.

Done.

  • \-style line continuations are also fairly undesirable. These should use triple-quoted strings or unbalanced parenthesis instead.

Fixed (used parens).

  • There's a debug print in a nested function which isn't even used

Both removed.

  • Test methods named like test_MethodHelp should be test_methodHelp instead.

Changed.

  • wait_timneout violates the naming convention and also seems to be completely ignored. What's the point of this?

Dunno. I copied this from twisted.web2.test.test_server.BaseCase. Removed in test_xmlrpc.py.

Additionally, I ran Pyflakes against xmlrpc.py, found some unused imports and removed those as well.

I've uploaded the newest changes.

comment:10 Changed 8 years ago by exarkun

  • Owner changed from exarkun to dreid

comment:11 follow-up: Changed 8 years ago by dreid

  • Keywords review removed
  • Owner changed from dreid to oubiwann
  • test_RPCFailure uses log.flushErrors which is deprecated. Sould use self.flushLoggedErrors instead.
  • Extraneous whitespace in the list comprehensions in xmlrpc_listMethods
  • Since TestServer and ClientTests really aren't used, there is no reason for their import to be there. You can just remove the line completely.

comment:12 Changed 8 years ago by oubiwann

  • Status changed from new to assigned

comment:13 in reply to: ↑ 11 Changed 8 years ago by oubiwann

Replying to dreid:

  • test_RPCFailure uses log.flushErrors which is deprecated. Sould use self.flushLoggedErrors instead.
  • Extraneous whitespace in the list comprehensions in xmlrpc_listMethods
  • Since TestServer and ClientTests really aren't used, there is no reason for their import to be there. You can just remove the line completely.
  • I'm assuming you meant self.flushErrors()... I could only find one example of that in all of the Twisted source code, and that was in the tests for t.w2.test.test_wsgi. I used that as an example.
  • Whitespace in list comprehensions fixed.
  • Commented import removed.

I will upload the new patches and set to review again.

Changed 8 years ago by oubiwann

comment:14 Changed 8 years ago by oubiwann

  • Keywords review added
  • Owner changed from oubiwann to dreid
  • Status changed from assigned to new

comment:15 Changed 8 years ago by dreid

  • Keywords review removed
  • Owner changed from dreid to oubiwann

Sorry with self.flushLoggedErrors I was refering to twisted.trial.unittest.TestCase.flushLoggedErrors. You should probably update you're working copy if you're not seeing the deprecation warning that explains this.

comment:16 Changed 8 years ago by oubiwann

Ah, got it -- thanks! Yeah, branch 1929 is enough out of date that the tests won't run with the newest trunk (fails on from twisted.application.reactors import Reactor). I created my own local "branch" from the latest trunk, and now I see the deprecation message.

I fixed this to use TestCase.flushLoggedErrors and will shortly update this ticket with the latest patch.

comment:17 Changed 8 years ago by oubiwann

  • Description modified (diff)
  • Owner changed from oubiwann to dreid

comment:18 Changed 8 years ago by oubiwann

  • Keywords review added

Changed 8 years ago by oubiwann

comment:19 Changed 8 years ago by dreid

  • Keywords review removed
  • Status changed from new to assigned

branched to web2-xmlrpc-tests-1929-2

builds on:

  • debian-py2.4-select
  • debian64-py2.4-select
  • osx-py2.4-select
  • osx-py2.4-select-gc
  • win32-py2.4-select
  • win32-py2.5-select

unrelated failures on:

  • win32-py2.4-er
  • debian-py2.5-select
  • suse-py2.5-select

comment:20 Changed 8 years ago by dreid

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

(In [18820]) Merge web2-xmlrpc-tests-1929-2

Author: oubiwann
Reviewer: dreid,exarkun
Fixes #1929

The Web2 XML-RPC tests no longer require twisted.web.xmlrpc.Proxy to run. In fact they do not
hit the network at all and so do not cause any Unclean Reactor warnings.

comment:21 Changed 4 years ago by <automation>

  • Owner dreid deleted
Note: See TracTickets for help on using tickets.