Merge lp:~florent.x/pyflakes/1097061-unittest2 into lp:~divmod-dev/pyflakes/trunk

[Florent]

Enhancement for continuous integration and coverage of Python version 2.5 (and 3.x in the near future)

[Jean-Paul Calderone (exarkun)]

I'd prefer not to see Pyflakes gain a setuptools dependency (even an optional one). setuptools causes far more problems than it solves. Regardless, as a part of this branch, the addition seems out of place - Pyflakes' setup.py did not declare a dependency on Twisted previously (in other words, it's an unrelated "enhancement").

[Florent (florent.x)]

To be fair, the testsuite was dependent on Twisted previously, even if it was not declared in setup.py.

Here, the optional import gives a benefit for commands like:

 $ python setup.py test

This allows to run the tests simply, and it takes care of installing the 'test_requires' dependency without other action (without any assumption on the test framework: nose, py.test, twisted.trial, unittest, etc...).
This particular use case is not supported by the distutils library.

In addition, the optional dependency is not on setuptools exclusively. It works perfectly with distribute as well.
I just tried without distribute/setuptools, and I see that there's UserWarnings in the console.
I could change setup.py in order to add 'test_requires'/'test_suite' keywords only if the feature is available.

For the record, there are some users which use the advanced features of distribute/setuptools and already tried to have it supported: https://github.com/kevinw/pyflakes/pull/19

[Florent (florent.x)]

> Here, the optional import gives a benefit for commands like:
>
> $ python setup.py test
>

Another verb which is widely used but not supported by distutils:

  $ python setup.py develop

[Jean-Paul Calderone (exarkun)]

> To be fair, the testsuite was dependent on Twisted previously, even if it was not declared in setup.py.

Yes. That was part of my point. It depended on Twisted and did not declare it in setup.py. You can switch to unittest2 and still not declare it it setup.py. The change to introduce a setuptools dependency can be considered *completely* separately from the change to use unittest2.

[Florent (florent.x)]

Sorry, but I fail to understand the reasoning to reject the setuptools optional dependency.
It makes testing easier and more standard for the developer.
Since the 'tests_require' contains information about the tests requirements, anyone can run the tests without digging in the source code to know which libraries are required.
And when the distribute/setuptools package is not installed, it imports from distutils and it behaves the same as before.

If the request is to split the merge proposal into 2 patches ... why not.
For me, they are complementary changes to improve testing, so it makes sense to propose them together.

[Jean-Paul Calderone (exarkun)]

> It makes testing easier and more standard for the developer.

It doesn't do either of these.

> Since the 'tests_require' contains information about the tests requirements, anyone can run the tests without digging in the source code to know which libraries are required.

This can be addressed just as easily with an INSTALL or README file.

> And when the distribute/setuptools package is not installed, it imports from distutils and it behaves the same as before.

So there are now twice as many possible install outcomes, which makes debugging installation problems (at least) twice as hard.

> If the request is to split the merge proposal into 2 patches ... why not.

Yes, that's what I was requesting (so that I can only reject the setuptools change while not rejecting the unittest2 change).

> For me, they are complementary changes to improve testing, so it makes sense to propose them together.

Many, many, many, many changes are complementary. That doesn't mean they should be made simultaneously. Changesets should focus on one change, not many complementary changes. Basically, the smallest useful change is what you should try to make. That way issues with *unrelated* changes don't hold up useful improvements, reviews are easier, the history is easier to navigate, tickets can have simple descriptions, and problems that require changes to be reverted don't also revert working functionality.

[Florent (florent.x)]

The branch was replaced according to your request.

[Glyph Lefkowitz (glyph)]

On Jan 8, 2013, at 8:20 AM, Jean-Paul Calderone <email address hidden> wrote:

>> If the request is to split the merge proposal into 2 patches ... why not.
>
> Yes, that's what I was requesting (so that I can only reject the setuptools change while not rejecting the unittest2 change).
>
>> For me, they are complementary changes to improve testing, so it makes sense to propose them together.
>
> Many, many, many, many changes are complementary. That doesn't mean they should be made simultaneously. Changesets should focus on one change, not many complementary changes. Basically, the smallest useful change is what you should try to make. That way issues with *unrelated* changes don't hold up useful improvements, reviews are easier, the history is easier to navigate, tickets can have simple descriptions, and problems that require changes to be reverted don't also revert working functionality.

My feelings about setuptools are somewhat more ambivalent, but this point is more important. The fact that this discussion about setuptools is happening is now holding up an unrelated change. If the changes were split up, the less controversial parts (which are by far the larger part of these changes) could be discussed on their own merits and landed much more quickly.

[Florent (florent.x)]

This branch contains only the replacement of Twisted with unittest2 and is ready for review.

[Florent (florent.x)]

LGTM ;)

review: Approve (self-review)

=== modified file '.travis.yml'
--- .travis.yml	2013-01-07 17:24:54 +0000
+++ .travis.yml	2013-01-08 22:58:19 +0000
@@ -1,13 +1,14 @@
 language: python
 python:
+  - 2.5
   - 2.6
   - 2.7
   - pypy
 install:
   - python setup.py install
-  - pip install Twisted
+  - pip install unittest2
 script:
-  - trial pyflakes/test
+  - python -m unittest2 discover
 matrix:
   allow_failures:
     - python: pypy

=== modified file 'pyflakes/test/harness.py'
--- pyflakes/test/harness.py	2010-04-13 14:53:04 +0000
+++ pyflakes/test/harness.py	2013-01-08 22:58:19 +0000
@@ -2,12 +2,12 @@
 import textwrap
 import _ast
 
-from twisted.trial import unittest
+import unittest2
 
 from pyflakes import checker
 
 
-class Test(unittest.TestCase):
+class Test(unittest2.TestCase):
 
     def flakes(self, input, *expectedOutputs, **kw):
         ast = compile(textwrap.dedent(input), "<test>", "exec",
@@ -17,7 +17,7 @@
         expectedOutputs = list(expectedOutputs)
         outputs.sort()
         expectedOutputs.sort()
-        self.assert_(outputs == expectedOutputs, '''\
+        self.assertEqual(outputs, expectedOutputs, '''\
 for input:
 %s
 expected outputs:

=== modified file 'pyflakes/test/test_imports.py'
--- pyflakes/test/test_imports.py	2011-10-31 15:19:35 +0000
+++ pyflakes/test/test_imports.py	2013-01-08 22:58:19 +0000
@@ -1,5 +1,6 @@
 
 from sys import version_info
+from unittest2 import skip, skipIf
 
 from pyflakes import messages as m
 from pyflakes.test import harness
@@ -446,6 +447,7 @@
         self.flakes('import fu; [fu, bar] = fu')
         self.flakes('import fu; fu += fu')
 
+    @skip("todo")
     def test_tryingMultipleImports(self):
         self.flakes('''
         try:
@@ -453,7 +455,6 @@
         except ImportError:
             import bar as fu
         ''')
-    test_tryingMultipleImports.todo = ''
 
     def test_nonGlobalDoesNotRedefine(self):
         self.flakes('''
@@ -479,6 +480,7 @@
         fu
         ''', m.RedefinedWhileUnused)
 
+    @skip("todo")
     def test_importingForImportError(self):
         self.flakes('''
         try:
@@ -486,8 +488,8 @@
         except ImportError:
             pass
         ''')
-    test_importingForImportError.todo = ''
 
+    @skip("todo: requires evaluating attribute access")
     def test_importedInClass(self):
         '''Imports in class scope can be used through self'''
         self.flakes('''
@@ -496,7 +498,6 @@
             def __init__(self):
                 self.i
         ''')
-    test_importedInClass.todo = 'requires evaluating attribute access'
 
     def test_futureImport(self):
         '''__future__ is special'''
@@ -639,10 +640,8 @@
     """
     Tests for checking of syntax which is valid in PYthon 2.6 and newer.
     """
-    if version_info < (2, 6):
-        skip = "Python 2.6 required for class decorator tests."
-
-
+
+    @skipIf(version_info < (2, 6), "Python >= 2.6 only")
     def test_usedAsClassDecorator(self):
         """
         Using an imported name as a class decorator results in no warnings,

=== modified file 'pyflakes/test/test_other.py'
--- pyflakes/test/test_other.py	2011-11-17 16:21:58 +0000
+++ pyflakes/test/test_other.py	2013-01-08 22:58:19 +0000
@@ -6,6 +6,7 @@
 """
 
 from sys import version_info
+from unittest2 import skip, skipIf
 
 from pyflakes import messages as m
 from pyflakes.test import harness
@@ -16,6 +17,7 @@
     def test_duplicateArgs(self):
         self.flakes('def fu(bar, bar): pass', m.DuplicateArgument)
 
+    @skip("todo: this requires finding all assignments in the function body first")
     def test_localReferencedBeforeAssignment(self):
         self.flakes('''
         a = 1
@@ -23,7 +25,6 @@
             a; a=1
         f()
         ''', m.UndefinedName)
-    test_localReferencedBeforeAssignment.todo = 'this requires finding all assignments in the function body first'
 
     def test_redefinedFunction(self):
         """
@@ -148,6 +149,7 @@
         ''', m.RedefinedWhileUnused)
 
 
+    @skip("todo: Too hard to make this warn but other cases stay silent")
     def test_doubleAssignment(self):
         """
         If a variable is re-assigned to without being used, no warning is
@@ -158,8 +160,6 @@
         x = 10
         x = 20
         ''', m.RedefinedWhileUnused)
-    test_doubleAssignment.todo = (
-        "Too hard to make this warn but other cases stay silent")
 
 
     def test_doubleAssignmentConditionally(self):
@@ -406,14 +406,6 @@
         ''')
 
 
-
-class Python25Test(harness.Test):
-    """
-    Tests for checking of syntax only available in Python 2.5 and newer.
-    """
-    if version_info < (2, 5):
-        skip = "Python 2.5 required for if-else and with tests"
-
     def test_ifexp(self):
         """
         Test C{foo if bar else baz} statements.
@@ -627,15 +619,7 @@
             pass
         ''', m.UndefinedName)
 
-
-
-class Python27Test(harness.Test):
-    """
-    Tests for checking of syntax only available in Python 2.7 and newer.
-    """
-    if version_info < (2, 7):
-        skip = "Python 2.7 required for dict/set comprehension tests"
-
+    @skipIf(version_info < (2, 7), "Python >= 2.7 only")
     def test_dictComprehension(self):
         """
         Dict comprehensions are properly handled.
@@ -644,6 +628,7 @@
         a = {1: x for x in range(10)}
         ''')
 
+    @skipIf(version_info < (2, 7), "Python >= 2.7 only")
     def test_setComprehensionAndLiteral(self):
         """
         Set comprehensions are properly handled.

=== modified file 'pyflakes/test/test_script.py'
--- pyflakes/test/test_script.py	2012-10-23 11:52:32 +0000
+++ pyflakes/test/test_script.py	2013-01-08 22:58:19 +0000
@@ -4,15 +4,12 @@
 
 import os
 import sys
+import shutil
+import subprocess
+import tempfile
 from StringIO import StringIO
 
-from twisted.internet import protocol
-from twisted.internet.utils import (
-    _callProtocolWithDeferred,
-    getProcessOutputAndValue,
-    )
-from twisted.python.filepath import FilePath
-from twisted.trial.unittest import TestCase
+from unittest2 import TestCase
 
 from pyflakes.messages import UnusedImport
 from pyflakes.reporter import Reporter
@@ -20,7 +17,7 @@
     checkPath,
     checkRecursive,
     iterSourceCode,
-    )
+)
 
 
 def withStderrTo(stderr, f, *args, **kwargs):
@@ -34,7 +31,6 @@
         sys.stderr = outer
 
 
-
 class LoggingReporter(object):
     """
     Implementation of Reporter that just appends any errors to a list.
@@ -67,13 +63,25 @@
     Tests for L{iterSourceCode}.
     """
 
+    def setUp(self):
+        self.tempdir = tempfile.mkdtemp()
+
+    def tearDown(self):
+        shutil.rmtree(self.tempdir)
+
+    def makeEmptyFile(self, *parts):
+        assert parts
+        fpath = os.path.join(self.tempdir, *parts)
+        fd = open(fpath, 'a')
+        fd.close()
+        return fpath
+
+
     def test_emptyDirectory(self):
         """
         There are no Python files in an empty directory.
         """
-        tempdir = FilePath(self.mktemp())
-        tempdir.createDirectory()
-        self.assertEqual(list(iterSourceCode([tempdir.path])), [])
+        self.assertEqual(list(iterSourceCode([self.tempdir])), [])
 
 
     def test_singleFile(self):
@@ -81,22 +89,16 @@
         If the directory contains one Python file, C{iterSourceCode} will find
         it.
         """
-        tempdir = FilePath(self.mktemp())
-        tempdir.createDirectory()
-        tempdir.child('foo.py').touch()
-        self.assertEqual(
-            list(iterSourceCode([tempdir.path])),
-            [tempdir.child('foo.py').path])
+        childpath = self.makeEmptyFile('foo.py')
+        self.assertEqual(list(iterSourceCode([self.tempdir])), [childpath])
 
 
     def test_onlyPythonSource(self):
         """
         Files that are not Python source files are not included.
         """
-        tempdir = FilePath(self.mktemp())
-        tempdir.createDirectory()
-        tempdir.child('foo.pyc').touch()
-        self.assertEqual(list(iterSourceCode([tempdir.path])), [])
+        self.makeEmptyFile('foo.pyc')
+        self.assertEqual(list(iterSourceCode([self.tempdir])), [])
 
 
     def test_recurses(self):
@@ -104,18 +106,14 @@
         If the Python files are hidden deep down in child directories, we will
         find them.
         """
-        tempdir = FilePath(self.mktemp())
-        tempdir.createDirectory()
-        tempdir.child('foo').createDirectory()
-        tempdir.child('foo').child('a.py').touch()
-        tempdir.child('bar').createDirectory()
-        tempdir.child('bar').child('b.py').touch()
-        tempdir.child('c.py').touch()
+        os.mkdir(os.path.join(self.tempdir, 'foo'))
+        apath = self.makeEmptyFile('foo', 'a.py')
+        os.mkdir(os.path.join(self.tempdir, 'bar'))
+        bpath = self.makeEmptyFile('bar', 'b.py')
+        cpath = self.makeEmptyFile('c.py')
         self.assertEqual(
-            sorted(iterSourceCode([tempdir.path])),
-            sorted([tempdir.child('foo').child('a.py').path,
-                    tempdir.child('bar').child('b.py').path,
-                    tempdir.child('c.py').path]))
+            sorted(iterSourceCode([self.tempdir])),
+            sorted([apath, bpath, cpath]))
 
 
     def test_multipleDirectories(self):
@@ -123,18 +121,15 @@
         L{iterSourceCode} can be given multiple directories.  It will recurse
         into each of them.
         """
-        tempdir = FilePath(self.mktemp())
-        tempdir.createDirectory()
-        foo = tempdir.child('foo')
-        foo.createDirectory()
-        foo.child('a.py').touch()
-        bar = tempdir.child('bar')
-        bar.createDirectory()
-        bar.child('b.py').touch()
+        foopath = os.path.join(self.tempdir, 'foo')
+        barpath = os.path.join(self.tempdir, 'bar')
+        os.mkdir(foopath)
+        apath = self.makeEmptyFile('foo', 'a.py')
+        os.mkdir(barpath)
+        bpath = self.makeEmptyFile('bar', 'b.py')
         self.assertEqual(
-            sorted(iterSourceCode([foo.path, bar.path])),
-            sorted([foo.child('a.py').path,
-                    bar.child('b.py').path]))
+            sorted(iterSourceCode([foopath, barpath])),
+            sorted([apath, bpath]))
 
 
     def test_explicitFiles(self):
@@ -142,10 +137,9 @@
         If one of the paths given to L{iterSourceCode} is not a directory but
         a file, it will include that in its output.
         """
-        tempfile = FilePath(self.mktemp())
-        tempfile.touch()
-        self.assertEqual(list(iterSourceCode([tempfile.path])),
-                         [tempfile.path])
+        epath = self.makeEmptyFile('e.py')
+        self.assertEqual(list(iterSourceCode([epath])),
+                         [epath])
 
 
 
@@ -239,9 +233,11 @@
         """
         Make a temporary file containing C{content} and return a path to it.
         """
-        path = FilePath(self.mktemp())
-        path.setContent(content)
-        return path.path
+        _, fpath = tempfile.mkstemp()
+        fd = open(fpath, 'wb')
+        fd.write(content)
+        fd.close()
+        return fpath
 
 
     def assertHasErrors(self, path, errorList):
@@ -314,8 +310,12 @@
         # isn't, something this test was unprepared for has happened.
         def evaluate(source):
             exec source
-        exc = self.assertRaises(SyntaxError, evaluate, source)
-        self.assertTrue(exc.text.count('\n') > 1)
+        try:
+            evaluate(source)
+        except SyntaxError, e:
+            self.assertTrue(e.text.count('\n') > 1)
+        else:
+            self.fail()
 
         sourcePath = self.makeTempFile(source)
         self.assertHasErrors(
@@ -384,14 +384,13 @@
         If the source file is not readable, this is reported on standard
         error.
         """
-        sourcePath = FilePath(self.mktemp())
-        sourcePath.setContent('')
-        sourcePath.chmod(0)
-        count, errors = self.getErrors(sourcePath.path)
+        sourcePath = self.makeTempFile('')
+        os.chmod(sourcePath, 0)
+        count, errors = self.getErrors(sourcePath)
         self.assertEquals(count, 1)
         self.assertEquals(
             errors,
-            [('unexpectedError', sourcePath.path, "Permission denied")])
+            [('unexpectedError', sourcePath, "Permission denied")])
 
 
     def test_pyflakesWarning(self):
@@ -425,59 +424,25 @@
         L{checkRecursive} descends into each directory, finding Python files
         and reporting problems.
         """
-        tempdir = FilePath(self.mktemp())
-        tempdir.createDirectory()
-        tempdir.child('foo').createDirectory()
-        file1 = tempdir.child('foo').child('bar.py')
-        file1.setContent("import baz\n")
-        file2 = tempdir.child('baz.py')
-        file2.setContent("import contraband")
+        tempdir = tempfile.mkdtemp()
+        os.mkdir(os.path.join(tempdir, 'foo'))
+        file1 = os.path.join(tempdir, 'foo', 'bar.py')
+        fd = open(file1, 'wb')
+        fd.write("import baz\n")
+        fd.close()
+        file2 = os.path.join(tempdir, 'baz.py')
+        fd = open(file2, 'wb')
+        fd.write("import contraband")
+        fd.close()
         log = []
         reporter = LoggingReporter(log)
-        warnings = checkRecursive([tempdir.path], reporter)
+        warnings = checkRecursive([tempdir], reporter)
         self.assertEqual(warnings, 2)
         self.assertEqual(
             sorted(log),
-            sorted([('flake', str(UnusedImport(file1.path, 1, 'baz'))),
+            sorted([('flake', str(UnusedImport(file1, 1, 'baz'))),
                     ('flake',
-                     str(UnusedImport(file2.path, 1, 'contraband')))]))
-
-
-
-class _EverythingGetterWithStdin(protocol.ProcessProtocol):
-    """
-    C{ProcessProtocol} that writes to stdin and gathers exit code, stdout and
-    stderr.
-    """
-
-    # Although Twisted provides a helper that spawns a subprocess, gathers its
-    # exit code, standard output and standard error
-    # (i.e. getProcessOutputAndValue), it does *not* provide one that data to
-    # be written to stdin first.
-
-    def __init__(self, deferred, stdin):
-        self.deferred = deferred
-        self.outBuf = StringIO()
-        self.errBuf = StringIO()
-        self.outReceived = self.outBuf.write
-        self.errReceived = self.errBuf.write
-        self.stdin = stdin
-
-
-    def connectionMade(self):
-        self.transport.write(self.stdin)
-        self.transport.closeStdin()
-
-
-    def processEnded(self, reason):
-        out = self.outBuf.getvalue()
-        err = self.errBuf.getvalue()
-        e = reason.value
-        code = e.exitCode
-        if e.signal:
-            code = -e.signal
-        self.deferred.callback((out, err, code))
-
+                     str(UnusedImport(file2, 1, 'contraband')))]))
 
 
 class IntegrationTests(TestCase):
@@ -485,13 +450,20 @@
     Tests of the pyflakes script that actually spawn the script.
     """
 
+    def setUp(self):
+        self.tempdir = tempfile.mkdtemp()
+        self.tempfilepath = os.path.join(self.tempdir, 'temp')
+
+    def tearDown(self):
+        shutil.rmtree(self.tempdir)
+
     def getPyflakesBinary(self):
         """
         Return the path to the pyflakes binary.
         """
         import pyflakes
-        package_dir = FilePath(pyflakes.__file__).parent()
-        return package_dir.sibling('bin').child('pyflakes').path
+        package_dir = os.path.dirname(pyflakes.__file__)
+        return os.path.join(package_dir, '..', 'bin', 'pyflakes')
 
 
     def runPyflakes(self, paths, stdin=None):
@@ -505,15 +477,18 @@
         """
         env = dict(os.environ)
         env['PYTHONPATH'] = os.pathsep.join(sys.path)
-        command = [self.getPyflakesBinary()]
+        command = [sys.executable, self.getPyflakesBinary()]
         command.extend(paths)
         if stdin:
-            d = _callProtocolWithDeferred(
-                lambda d: _EverythingGetterWithStdin(d, stdin),
-                sys.executable, command, env=env, path=None)
+            p = subprocess.Popen(command, env=env, stdin=subprocess.PIPE,
+                                 stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+            (stdout, stderr) = p.communicate(stdin)
         else:
-            d = getProcessOutputAndValue(sys.executable, command, env=env)
-        return d
+            p = subprocess.Popen(command, env=env,
+                                 stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+            (stdout, stderr) = p.communicate()
+        rv = p.wait()
+        return (stdout, stderr, rv)
 
 
     def test_goodFile(self):
@@ -521,10 +496,10 @@
         When a Python source file is all good, the return code is zero and no
         messages are printed to either stdout or stderr.
         """
-        tempfile = FilePath(self.mktemp())
-        tempfile.touch()
-        d = self.runPyflakes([tempfile.path])
-        return d.addCallback(self.assertEqual, ('', '', 0))
+        fd = open(self.tempfilepath, 'a')
+        fd.close()
+        d = self.runPyflakes([self.tempfilepath])
+        self.assertEqual(d, ('', '', 0))
 
 
     def test_fileWithFlakes(self):
@@ -532,12 +507,11 @@
         When a Python source file has warnings, the return code is non-zero
         and the warnings are printed to stdout.
         """
-        tempfile = FilePath(self.mktemp())
-        tempfile.setContent("import contraband\n")
-        d = self.runPyflakes([tempfile.path])
-        return d.addCallback(
-            self.assertEqual,
-            ("%s\n" % UnusedImport(tempfile.path, 1, 'contraband'), '', 1))
+        fd = open(self.tempfilepath, 'wb')
+        fd.write("import contraband\n")
+        fd.close()
+        d = self.runPyflakes([self.tempfilepath])
+        self.assertEqual(d, ("%s\n" % UnusedImport(self.tempfilepath, 1, 'contraband'), '', 1))
 
 
     def test_errors(self):
@@ -546,11 +520,8 @@
         exist, say), then the return code is non-zero and the errors are
         printed to stderr.
         """
-        tempfile = FilePath(self.mktemp())
-        d = self.runPyflakes([tempfile.path])
-        return d.addCallback(
-            self.assertEqual,
-            ('', '%s: No such file or directory\n' % (tempfile.path,), 1))
+        d = self.runPyflakes([self.tempfilepath])
+        self.assertEqual(d, ('', '%s: No such file or directory\n' % (self.tempfilepath,), 1))
 
 
     def test_readFromStdin(self):
@@ -558,6 +529,4 @@
         If no arguments are passed to C{pyflakes} then it reads from stdin.
         """
         d = self.runPyflakes([], stdin='import contraband')
-        return d.addCallback(
-            self.assertEqual,
-            ("%s\n" % UnusedImport('<stdin>', 1, 'contraband'), '', 1))
+        self.assertEqual(d, ("%s\n" % UnusedImport('<stdin>', 1, 'contraband'), '', 1))

=== modified file 'pyflakes/test/test_undefined_names.py'
--- pyflakes/test/test_undefined_names.py	2011-12-04 20:27:45 +0000
+++ pyflakes/test/test_undefined_names.py	2013-01-08 22:58:19 +0000
@@ -1,7 +1,7 @@
 
 from _ast import PyCF_ONLY_AST
 
-from twisted.trial.unittest import TestCase
+from unittest2 import skip, TestCase
 
 from pyflakes import messages as m, checker
 from pyflakes.test import harness
@@ -87,13 +87,13 @@
             bar; baz
         ''')
 
+    @skip("todo")
     def test_definedByGlobal(self):
         '''"global" can make an otherwise undefined name in another function defined'''
         self.flakes('''
         def a(): global fu; fu = 1
         def b(): fu
         ''')
-    test_definedByGlobal.todo = ''
 
     def test_globalInGlobalScope(self):
         """