ExampleTestBase class for unit testing example code

[rwall]

This is part of the Twisted Better Examples plan:

• https://twistedmatrix.com/trac/wiki/Plan/BetterExamples
• #84

​The writing standard suggests that "example code should conform to the coding standard" which implies that examples should be unit tested just like the main twisted code.

One suggestion was to add something like Divmod ExampleTestBase, which imports the example scripts so that their components can be unit tested.

• ​https://bazaar.launchpad.net/~glyph/divmod.org/trunk/view/head:/Nevow/nevow/test/test_howtolistings.py

This has already been added to some of the twisted.names examples tests.

• source:trunk/twisted/names/test/test_examples.py

But it should be moved somewhere central eg

• source:trunk/twisted/test/proto_helpers.py or
• source:trunk/twisted/test/testutils.py

...where it can be used by tests for all Twisted examples.

In the same branch, add / update tests for twisted.names examples to demonstrate the new class and its central location.

[rwall]

(In [37471]) Branching to 'testable-examples-6362'

[rwall]

(In [37651]) Branching to 'testable-examples-6362-2'

[rwall]

(In [38175]) Branching to 'testable-examples-6362-3'

[rwall]

Ready for review in log:branches/testable-examples-6362-3

1. Moved ExampleTestBase from twisted.names.test.test_examples to twisted.test.testutils.
2. Split it into a base class for general testing of examples and an ExecutableExampleTest mixin for testing example scripts that are meant to be run from the command line and which adhere to a standard layout. (where "standard" means some sort of ideal which is being explored as part of #84 and applied first of all to the twisted.names examples).
3. Simplified and split up some of the existing tests.
4. Added a test for executing a standalone example script.
5. Modified some except and print statements to make the Python3 tests pass.
6. Removed what looked like an unnecessary StringIO import in twisted.python.reflect - to make Python3 tests pass.

Build Results:

• ​https://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/testable-examples-6362-3

gethostbyname.py unit tests:

In a previous version of this branch I had added further unit tests for the gethostbyname example, but I decided it might be easier for the reviewer if I split that work into another ticket. See #6449.

PYTHONPATH for trial subprocesses:

There is a slightly ugly manipulation of PYTHONPATH for the test_executable.

I create a PYTHONPATH out of the sys.path that has been set by trial.

The same trick is used in source:trunk/twisted/conch/test/test_cftp.py#L514

Exarkun and itamar suggested that there should be an API for doing this or that trial should automatically set the PYTHONPATH environment variable.

I'm not sure whether it's worth it since the solution is quite simple and only occurs twice. So I haven't raised a ticket for that.

Here's the conversation anyway...

14:13 < rwall> exarkun: Hi. I've written a test that attempts to execute examples scripts. The test runs on my laptop but fails on buildslaves.
14:13 < rwall> https://buildbot.twistedmatrix.com/builders/py-select-gc/builds/2074/steps/trial/logs/problems
14:13 < rwall> Have you got a moment to have a look?
14:14 < rwall> https://twistedmatrix.com/trac/changeset/38181/branches/testable-examples-6362-3
14:15 < rwall> I think its to do with the environment and PYTHONPATH.
14:16 < exarkun> Sure.
14:17 < exarkun> rwall: fwiw, you can see the environment the commands run it at the top of the stdio log
14:17 < rwall> Ah let me check that...
14:18 < exarkun> Basically, the command runs without PYTHONPATH set, relying on bin/trial to figure out where Twisted is and get it imported properly
14:18 < exarkun> Then trial changes its working directory to _trial_temp - which change is inherited by the child process run in that tes.
14:18 < exarkun> test.
14:18 < exarkun> and passing os.environ makes no differences, since there's nothing in it that affects Python imports
14:20 < rwall> ok. So I could look in the sys.path for twisted and set that as PYTHONPATH for the subprocess.
14:20 < exarkun> Various other Twisted-using subprocess tests in Twisted solve this problem in various gross, ad hoc ways
14:20 < exarkun> You might want to survey those solutions before deciding how to fix this one
14:20 < exarkun> Perhaps you'll even see a pattern and then you can introduce an API to help with this
14:20 < rwall> Ok, I'll have a look. Thanks.
14:21 < exarkun> a good start might be: grep getProcessOutput twisted/ -r --include 'test_*.py'
14:21 < rwall> yep
14:25 < itamar> I've been hit by this problem myself
14:26 < itamar> perhaps trial should also add the path to PYTHONPATH
14:26 < itamar> except then you're in gross shell quoting land
14:30 < rwall> itamar: ok. Do you remember how you solved it?
14:31 < exarkun> You have to learn how imports work eventually
14:31 < exarkun> trial can't fix this in general
14:32 < rwall> exarkun, itamar: Ok, I'll read the code and try and understand.
14:32 < exarkun> rwall: The existing solutions aren't very complex, I don't think you'll have trouble understanding them
14:33 < exarkun> They're also not /awesome/, but they work for their particular circumstances
14:33 < rwall> ok

[rwall]

Hmm. There seems to be a windows build failure:

• ​https://buildbot.twistedmatrix.com/builders/windows7-64-py2.7/builds/2449/steps/iocp/logs/problems

Maybe the PYTHONPATH environment variable is too long or something.

Looks like test_cftp skips executable tests on Windows.

Can I do the same?

[ERROR]
Traceback (most recent call last):
Failure: twisted.internet.utils._UnexpectedErrorOutput: got stderr: 'Traceback (most recent call last):\r\n  
File "C:\\Python27\\lib\\site.py", line 563, in <module>\r\n    
main()\r\n  
File "C:\\Python27\\lib\\site.py", line 539, in main\r\n    
known_paths = removeduppaths()\r\n  
File "C:\\Python27\\lib\\site.py", line 110, in removeduppaths\r\n    
dir, dircase = makepath(dir)\r\n  
File "C:\\Python27\\lib\\site.py", line 82, in makepath\r\n    
dir = os.path.abspath(dir)\r\n  
File "C:\\Python27\\lib\\ntpath.py", line 471, in abspath\r\n    
path = _getfullpathname(path)\r\n
TypeError: must be (buffer overflow), not str\r\n'

twisted.names.test.test_examples.DnsServiceTests.test_executableModule
twisted.names.test.test_examples.GetHostByNameTests.test_executableModule
twisted.names.test.test_examples.TestDnsTests.test_executableModule

[exarkun]

I'm not sure there's a good reason for Conch to skip those tests on Windows. It probably only gets away with it because big parts of Conch don't work on Windows so no one has really gotten too interested in making this better.

For any examples related to code that's supposed to work on Windows, it'd really be good to run their tests on Windows as well.

[rwall]

Replying to exarkun:

I'm not sure there's a good reason for Conch to skip those tests on Windows. It probably only gets away with it because big parts of Conch don't work on Windows so no one has really gotten too interested in making this better.

For any examples related to code that's supposed to work on Windows, it'd really be good to run their tests on Windows as well.

Ok. I think the problem was that I had used t.i.utils.getProcessOutput which terminates the subprocess immediately if it produces stderr.

On Windows, the example scripts print some Python import warnings to stderr.

The wacky error was probably due to this Python bug where a TypeError is raised instead of IOError.

• ​http://bugs.python.org/issue1776160#msg32652

I switched to t.i.utils.getProcessOutputAndValue, which collects both stdout and stderr and waits for the process to exit.

The tests now pass on windows7-64-py2.7 and winxp32-py2.7 (select):

• ​https://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/testable-examples-6362-3

[thijs]

Thanks rwall.

1. in twisted.python.usage:
   1. the lines you change in opt_help and opt_version are not tested. Should be straightforward enough to test those (with capturing the sys.stdout output and comparing it)
2. in twisted/test/testutils.py:
   1. can you place the os and sys imports *above* the BytesIO import
   2. i guess ExampleTestBaseMixin isn't public so it doesn't strictly need an @since marker, I'll leave it up to you
   3. sys.modules in tearDown needs some C{}
   4. ExecutableExampleTestMixin.test_definedOptions contains incorrect epytext
3. in twisted/python/reflect.py:
   1. the change in objgrep isn't tested

[rwall]

(In [38413]) Branching to 'testable-examples-6362-4'

[rwall]

Ready for another review in log:branches/testable-examples-6362-4

• Thanks Thijs. I merged forward and addressed most of your review comments
• Build Results - There are two twistedchecker errors which I think can be ignored and I don't understand why pyflakes builder shows all those "new" errors. I don't think they're related to my branch.
  • ​http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/testable-examples-6362-4

Replying to thijs:

1. in twisted/python/reflect.py:
   1. the change in objgrep isn't tested

That code branch is difficult to test. I'd need to find some object type which isn't currently handled by objgrep (which would be a bug) or I could modify the objgrep function - maybe I could add some special type (purely for testing purposes) which would never be recognised. But this seems outside the scope of my branch so I added a new ticket #6515.

[thijs]

Thanks rwall. I think it would be great if you could add some documentation (in the coding standard? or maybe a new document) on how to create a new example that uses this new base class. The coding standard also has some stuff about example files, maybe you can link/add it there? I guess this could be done in a new ticket as well but i bet we'll find something that way that needs some more work..

[glyph]

I'm really sorry to jump in at this late point in the ticket, but there's a design principle we've been trying to adhere to that bears mentioning:

You should keep test tools out of fixtures, for the same general reasons that you should try to avoid inheritance in all cases.

This is not a hard requirement, and I won't block this from landing, but I would really appreciate it if you could change this to be an isolated component rather than a thing you have to inherit from, mixin or no.

[rwall]

(In [38712]) Branching to 'testable-examples-6362-5'

[rwall]

Ready for another review in log:branches/testable-examples-6362-5

1. Test results
   • ​http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/testable-examples-6362-5
   • I think the new pyflakes and twistedchecker errors can be ignored.
2. I'm skipping the example script tests when the 'doc' directory is missing; which is the case for the debien easyinstall builders.

Replying to thijs:

Thanks rwall. I think it would be great if you could add some documentation (in the coding standard? or maybe a new document) on how to create a new example that uses this new base class. The coding standard also has some stuff about example files, maybe you can link/add it there? I guess this could be done in a new ticket as well but i bet we'll find something that way that needs some more work.

It's a good idea and I have suggested it as part of wiki:Plan/BetterExamples#Reviewandupdatewritingandcodingstandardsforexamplescripts but I don't think I'm ready to update the coding standard docs yet.

Before I do that, I'd like to get some feedback and agreement on whether these new tests do represent good standards.

For example, does everyone agree that *all* example scripts should have consistent --help output or that all example scripts should use usage.Options rather than quick and dirty sys.argv checks?

Once this branch is merged I'll work on #6449 which will be a prototype for more script specific unit test coverage.

Then I'll probably be in a position to update the example script coding standard documentation.

Replying to glyph:

I'm really sorry to jump in at this late point in the ticket, but there's a design principle we've been trying to adhere to that bears mentioning:

You should keep test tools out of fixtures, for the same general reasons that you should try to avoid inheritance in all cases.

This is not a hard requirement, and I won't block this from landing, but I would really appreciate it if you could change this to be an isolated component rather than a thing you have to inherit from, mixin or no.

I've refactored a little. This removes one level of inheritance and introduces the following new apis which will hopefully be useful elsewhere.

• added a separate class to handle external script module loading.

• added a convenience function for loading and returning a script module and registering a teardown function for the parent TestCase.

• factored out the code for returning a FilePath for a branch relative path.

I left these (untested) in twisted.test.testutils for now but I'm willing to move them to a more public location and add tests if anyone can suggest where and if anyone thinks they will be generally useful.

[glyph]

I started to review this but then got interrupted.

However, I did force a new build and it noticed some problems. rwall, if you could fix those it would make the next review go faster :)

[glyph]

Oh; noticing the above comments, let me be clearer about what I mean about the build:

1. Don't ignore pyflakes and twistedchecker errors unless they're really horrible to fix. I don't want to have to think about whether new errors are "really important" or not; we should just make the buildbots be green unless working around the errors would really make us do something bad. If the tool is telling you something borderline that should really be OK, then please file a bug in the tool.
2. There are failing python 3.3 tests, which are more serious than the twistedchecker or pyflakes stuff.

Thanks!

[rwall]

(In [39464]) Branching to 'testable-examples-6362-6'

[rwall]

Replying to glyph:

Oh; noticing the above comments, let me be clearer about what I mean about the build:

Glyph,

Thanks for the review so far. Comments below...

1. Don't ignore pyflakes and twistedchecker errors unless they're really horrible to fix. I don't want to have to think about whether new errors are "really important" or not; we should just make the buildbots be green unless working around the errors would really make us do something bad. If the tool is telling you something borderline that should really be OK, then please file a bug in the tool.

​http://buildbot.twistedmatrix.com/builders/pyflakes/builds/806/steps/pyflakes/logs/new%20pyflakes%20errors

twisted/python/reflect.py:106: redefinition of function 'getter' from line 100

That's not a new warning, it just appears so because I touched that module in this branch to fix some broken Python3 imports.

I guess the "new pyflakes errors" build recipe could be improved but I'm not sure if its even possible when the line numbers will have changed.

​http://buildbot.twistedmatrix.com/builders/twistedchecker/builds/1095/steps/run-twistedchecker/logs/new%20twistedchecker%20errors

************* Module twisted.test.test_usage
W9015:252,0: Too many blank lines, expected 2
************* Module twisted.test.testutils
W9204:296,4:ExecutableExampleTestMixin.test_executableModule: Missing epytext markup @return for return value

Again I don't think this is a new warning. It only appears so because I touched this module. I checked and there are only two blank lines before and three lines after which I think is correct. Maybe twistedchecker is confused because the two preceding functions aren't methods.

The second case is a known problem with Twisted Checker:

• ​https://bugs.launchpad.net/twistedchecker/+bug/1094942

Although in this case the test is part of a mixin rather than a TestCase subclass - so it would be difficult to identify it as a unittest.

How do you feel about mixin tests in view of wiki:Design/KeepTestToolsOutOfFixtures ?

<rant> I'm happy to contribute fixes to twistedchecker (I've already got a couple of branches awaiting review) but AFAIK no one except raphael has access to that project.

I'm happy to raise tickets and contribute fixes to the buildbot recipes but I'd have to go hunting around for the correct project page and I'm not convinced that anyone will look at the ticket.

• ​https://bugs.launchpad.net/twisted-buildbot-configuration/+bug/1152535

I've made the point before, but I personally think it would be much simpler if all the Twisted issues and code lived in one place - twistedmatrix.com/trac

• ​https://bugs.launchpad.net/twisted-website/+bug/1140776/comments/1

</rant>

I also need to draft a new ticket message as promised in that ticket.

2. There are failing python 3.3 tests, which are more serious than the twistedchecker or pyflakes stuff.

I merged forward to pick up the chained deferred fixes which were failing before.

[glyph]

Replying to rwall:

Replying to glyph:

Oh; noticing the above comments, let me be clearer about what I mean about the build:

Glyph,

Thanks for the review so far. Comments below...

1. Don't ignore pyflakes and twistedchecker errors (...)

That's not a new warning, (...) Again I don't think this is a new warning. (...) The second case is a known problem with Twisted Checker: (...)

OK, I apologize for wording my exhortation so strongly. Clearly you knew that these errors were spurious, and why they were spurious. Sorry I prompted you to take up so much time writing about them.

In the future, in the case of similarly-spurious errors, all I'd really like is a brief explanation ("I've looked at the twistedchecker warnings and they're not new, just an artifact of 'diff' changing the line numbers") and a comment on any relevant affected bugs, so we can see how often bugs in twistedchecker/pyflakes are causing stress during development, and which bugs are the hot spots. I've already added a link back here to ​https://bugs.launchpad.net/twistedchecker/+bug/1094942 so you can see what I mean.

How do you feel about mixin tests in view of wiki:Design/KeepTestToolsOutOfFixtures ?

Haven't had enough time to look deeply :-(. Pretty much out of time with dealing with the other stuff on this comment...

<rant> I'm happy to contribute fixes to twistedchecker (I've already got a couple of branches awaiting review) but AFAIK no one except raphael has access to that project.

That is definitely a problem.

​https://bugs.launchpad.net/twistedchecker/+bug/1208702

I'm happy to raise tickets and contribute fixes to the buildbot recipes but I'd have to go hunting around for the correct project page and I'm not convinced that anyone will look at the ticket.

• ​https://bugs.launchpad.net/twisted-buildbot-configuration/+bug/1152535

I think that the correct project page for this is ​https://github.com/twisted-infra/twisted-buildbot-configuration

You can blame Tom for the somewhat stealthy move of 80% or so of infrastructure stuff (notably, not twistedchecker) to github, but don't be too mad, he made huge improvements to pretty much all the project infrastructure in the meanwhile :-).

I've made the point before, but I personally think it would be much simpler if all the Twisted issues and code lived in one place - twistedmatrix.com/trac

This is a conversation that would probably be better had on the mailing list, lest we derail this ticket further. At the very least, we ought to have a wiki page that is prominently linked to that explains where various things are tracked.

• ​https://bugs.launchpad.net/twisted-website/+bug/1140776/comments/1

</rant>

I also need to draft a new ticket message as promised in that ticket.

2. There are failing python 3.3 tests, which are more serious than the twistedchecker or pyflakes stuff.

I merged forward to pick up the chained deferred fixes which were failing before.

Cool, thank you.

[exarkun]

Thanks for your continued work on this effort. I will be pretty happy to live in the world where documentation can be unit tested.

1. twisted/test/testutils.py
   1. This isn't a place for public APIs. Maybe that's good because maybe this isn't a public API yet. However, we do need to decide. If it's not public, then it probably isn't going to get a feature news fragment. If it's just an internal tool for Twisted development, then that's great for us but it probably doesn't get announced the same way, say, DNSSEC support gets announced. :) If it is public, then it needs unit tests, documentation, and a better name - probably something in twisted.trial (which is our library for testing tools - all of the things lying around in test directories are... well, they might turn into tools someday). Private APIs also don't get @since markers.
   2. It probably shouldn't be public. I'm not sure these tests make a lot of sense outside of the Twisted Names examples they're already being applied to. It's 'possible' - but I don't think it's all that likely that they'll apply to any code outside of Twisted.
   3. After some discussion with glyph, I think this should have more pieces like ScriptLoader and fewer pieces like ExecutableExampleTestMixin. The former is a cool tool for composition and offers a lot of flexibility in how it is used. The latter is really just more subclassing - better mix it in with TestCase (oh and put it *first* in the base class list or you lose one of its features!).

One specific API idea the discussion seemed to converge on was something like this:

from twisted.trial.exampletester import Example
from twisted.trial.unittest import TestCase

class FooTests(TestCase):
    foo = Example(b"doc/core/examples/foo.py")

    def test_bar(self):
        self.assertEqual(3, self.foo.loadScript().bar)

    def test_baz(self):
        d = self.foo.run([self.foo.path.path, b"baz"])
        d.addCallback(self.assertEqual, b"quux")
        return d

There may be more useful APIs for this Example thing to expose - but getting the script as a module object and running the script and getting its results (maybe more completely than just a string like b"quux") seem to cover most of the use cases demonstrated in this branch.

I think it's probably also worth thinking about two more things:

1. more specific tests that cover actual functionality in the twisted.names examples (sure, they parse options, but do they do anything related to dns correctly?)
2. how can common tests be shared between different examples with a less complex api (where "complex" means "subclass this mixin and TestCase and set this attribute")? Maybe something along the lines of DnsServiceUsageTests = exampleUsageTests("doc/names/examples/dns-service.py")?

Thanks again for your work on this! Let me know if I can clarify any of these points further.

[rwall]

A test module that I wrote for some twisted.names documentation in #6864

[rwall]

Replying to exarkun:

I think it's probably also worth thinking about two more things:

1. more specific tests that cover actual functionality in the twisted.names examples (sure, they parse options, but do they do anything related to dns correctly?)

Thanks for the review and sorry for the delay replying to your points.

I did TDD of the new examples in #6864 but the tests require some PYTHONPATH mangling until this branch lands. So I've attached the test module here for the time being.

• attachment:test_overrideserver.py​

[adiroiban]

rwall are you still working in this?

I would like to have this merged so that I can write a test for #7715 bug.

Thanks!