Opened 11 years ago

Closed 10 years ago

#1929 defect closed fixed (fixed)

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

Reported by: David Reid Owned by:
Priority: high Milestone: Web2-Gold-Master
Component: web2 Keywords:
Cc: oubiwann, David Reid, Jean-Paul Calderone Branch:
Author:

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 10 years ago.
1929.test_xmlrpc.patch (9.9 KB) - added by oubiwann 10 years ago.

Download all attachments as: .zip

Change History (23)

comment:1 Changed 10 years ago by oubiwann

Cc: oubiwann added

comment:2 Changed 10 years ago by oubiwann

Milestone: Web2-Gold-Master

comment:3 Changed 10 years ago by oubiwann

Cc: David Reid added
Status: newassigned

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 10 years ago by oubiwann

Priority: normalhigh
Type: enhancementdefect

comment:5 Changed 10 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 10 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 10 years ago by oubiwann

Keywords: review added
Owner: changed from oubiwann to David Reid
Status: assignednew

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

comment:8 Changed 10 years ago by Jean-Paul Calderone

Cc: Jean-Paul Calderone added
Keywords: review removed
Owner: changed from David Reid 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 10 years ago by oubiwann

Keywords: review added
Owner: changed from oubiwann to Jean-Paul Calderone

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

Owner: changed from Jean-Paul Calderone to David Reid

comment:11 Changed 10 years ago by David Reid

Keywords: review removed
Owner: changed from David Reid 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 10 years ago by oubiwann

Status: newassigned

comment:13 in reply to:  11 Changed 10 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 10 years ago by oubiwann

Attachment: 1929.xmlrpc.patch added

comment:14 Changed 10 years ago by oubiwann

Keywords: review added
Owner: changed from oubiwann to David Reid
Status: assignednew

comment:15 Changed 10 years ago by David Reid

Keywords: review removed
Owner: changed from David Reid 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 10 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 10 years ago by oubiwann

Description: modified (diff)
Owner: changed from oubiwann to David Reid

comment:18 Changed 10 years ago by oubiwann

Keywords: review added

Changed 10 years ago by oubiwann

Attachment: 1929.test_xmlrpc.patch added

comment:19 Changed 10 years ago by David Reid

Keywords: review removed
Status: newassigned

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 10 years ago by David Reid

Resolution: fixed
Status: assignedclosed

(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 6 years ago by <automation>

Owner: David Reid deleted
Note: See TracTickets for help on using tickets.