Merge lp:~jml/divmod.org/pyflakes-reporter into lp:divmod.org

[Jonathan Lange]

A sketch for a reporter in pyflakes. This moves pyflakes toward having an interface that can be called from Python to gather errors without requiring stderr and stdout to be captured. It also separates the format of output from the means of checking.

I was inspired to do this while trying to add a test to my own projects to guarantee that it is pyflakes-clean. I didn't want to do stdout/err trapping, and thought that something like this would be useful.

The pyflakes script wasn't exactly the best tested code. I've tried to add tests to cover the behaviour. The integration tests in particular would bear careful scrutiny. I've doubtless missed many opportunities to remove duplicated code in the tests.

I have also tried to preserve API compatibility. That's why 'reporter' is left as an optional parameter for existing methods.

[Glyph Lefkowitz (glyph)]

For what it's worth, seems like a good idea to me.

Tests, please, though.

[Jonathan Jacobs (jjacobs)]

I, too, like this idea. I'd love to see it merged.

[Jonathan Lange (jml)]

On 27 June 2012 19:51, Glyph Lefkowitz <email address hidden> wrote:
> For what it's worth, seems like a good idea to me.
>

Cool, thanks.

> Tests, please, though.

Sure. I'll probably throw this away & start something again TDD. This
seemed the shortest way of describing the change I wished to make.

jml

[Jean-Paul Calderone (exarkun)]

Thanks. It will be interesting to see what kind of custom reporters people build for pyflakes once this lands.

  0. Perhaps the reporter shouldn't be defined in the scripts module.
  1. These three methods of the reporter, ioError, problemDecodingSource, and syntaxError seems bad to me. These are all just reasons pyflakes couldn't get an AST. They're all quite well described by some exception object that pyflakes catches. And it's entirely possible we'll find other problems that fall into the same category. It would be better to have a single method that dealt with these errors and could be expanded to handle new errors rather than having a method for each and have to add a new method to the reporter interface.
  2. Can we consider accepting (and explicitly documenting) text streams to the reporter instead of byte streams? All of the output pyflakes generates should be text.
  3. Seems like there is an excessive use of `os.path` for a project that already depends on Twisted and even uses FilePath in a few places.
  4. Some things are missing docstrings: test_flake, CheckTests.getErrors,
  5. Thanks for the makeTempFile / assertHasErrors refactoring. That's nice.
  6. popen is deadly poison. I think it would be really great if pyflakes tests didn't need to use it, somehow. At a absolute minimum, it would be nice if none of the details of its use leaked out of the single helper method for using it. eg, no use of `communicate` in individual test methods.

review: Needs Fixing

[lp:~jml/divmod.org/pyflakes-reporter]

2712. By Jonathan Lange on 2012-10-19

0. Move Reporter to new module, pyflakes.reporter.

2713. By Jonathan Lange on 2012-10-19

3. Use FilePath rather than os.path (in tests)

2714. By Jonathan Lange on 2012-10-19

4. Add a bunch of docstrings.

2715. By Jonathan Lange on 2012-10-19

6 (partial). Rename 'popenPyflakes' to 'runPyflakes'.

2716. By Jonathan Lange on 2012-10-19

6 (partial). Contain the poison by returning returncode, out, err from runPyflakes6 (partial). Contain the poison by returning returncode, out, err from runPyflakes6 (partial). Contain the poison by returning returncode, out, err from runPyflakes6 (partial). Contain the poison by returning returncode, out, err from runPyflakes6 (partial). Contain the poison by returning returncode, out, err from runPyflakes6 (partial). Contain the poison by returning returncode, out, err from runPyflakes

2717. By Jonathan Lange on 2012-10-19

6 (partial). Change the integration tests to expect a Deferred from runPyflakes.

2718. By Jonathan Lange on 2012-10-19

6 (partial). Use Twisted to spawn the process, not subprocess.

2719. By Jonathan Lange on 2012-10-19

Change the test assertions to more naturally follow getProcessOutputAndValue's return

2720. By Jonathan Lange on 2012-10-19

Remove unnecessary parens

2721. By Jonathan Lange on 2012-10-19

Explain what's going on.

[Jonathan Lange (jml)]

Sorry about the partial reply.

0. Reporter moved to new module, pyflakes.reporter.
1. Not sure these can be combined into one method while still preserving current output. e.g. IOError doesn't include filename. TODO: Investigate further, try a couple of options.
2. TODO
3. Done, but for tests only. AFAICT, only the tests import Twisted. Happy to change non-test code if you don't mind having it depend on Twisted.
4. Docstrings added
5. My pleasure.
6. I've killed subprocess.Popen usage. Made an ugly compromise for spawning a subprocess and sending stdin. Think it's the least of four evils (the other three being continue w/ subprocess.Popen, duplicate _EverythingGetter in pyflakes, or block this branch until Twisted provides such a helper).

[Jean-Paul Calderone (exarkun)]

> Reporter moved to new module, pyflakes.reporter.

Great, thanks.

> Not sure these can be combined into one method while still preserving current output.

Okay, that makes sense. I guess pyflakes doesn't offer any API compatibility promises anyway, so we can change this as needed later. ;)

> Done, but for tests only. AFAICT, only the tests import Twisted. Happy to change non-test code if you don't mind having it depend on Twisted.

Just making the tests nice is fine for now, as far as I'm concerned.

> Docstrings added

Great.

> I've killed subprocess.Popen usage. Made an ugly compromise for spawning a subprocess and sending stdin. Think it's the least of four evils (the other three being continue w/ subprocess.Popen, duplicate _EverythingGetter in pyflakes, or block this branch until Twisted provides such a helper).

Hmm. This is going to break hard at some point, though. Though, it will only break the unit tests, not pyflakes itself... Still, `_EverythingGetter` is barely any longer than the subclass being added to pyflakes. Duplicating it in pyflakes seems better than waiting for pyflakes to break when `twisted.internet.utils` changes. Also, `_EverythingGetter` is awful (for example, errbacking with a tuple if a signal ends the process). I'd lean towards copying it (and simplifying it by fixing it to get rid of the nasty signal behavior, at least) instead of importing the private name. If you're not convinced, fine (but I might ask you to fix it when Twisted breaks it ;).

Thanks! Do you plan to do point 2 (text streams) or do you want to delay that for a separate ticket (is there even a ticket associated with this merge proposal)?

review: Needs Information

[Jean-Paul Calderone (exarkun)]

Oh. Another new comment. `mod_reporter` doesn't conform to the naming convention and `__import__` is obsoleted by `__future__.absolute_import`. Thanks!

[Jonathan Lange (jml)]

Happy to continue working on this patch. Just needed to cap my time on it yesterday.

When I get cycles to devote to this, will:

 1. duplicate & simplify _EverythingGetter, rather than import it
 2. change mod_reactor import to modReactor (or similar), and change both __import__'s to __future__.absolute_import
 3. attempt to make the reporter interface all text, documenting it as such
 4. attempt to unify the multiple import methods

Thanks for the review.

jml

[lp:~jml/divmod.org/pyflakes-reporter]

2722. By Jonathan Lange on 2012-10-23

New method for errors.

2723. By Jonathan Lange on 2012-10-23

Collapse problemDecodingSource and ioError into the one method.

2724. By Jonathan Lange on 2012-10-23

Document that we expect text, not bytes.

2725. By Jonathan Lange on 2012-10-23

Better ptype

2726. By Jonathan Lange on 2012-10-23

Pass unicode

2727. By Jonathan Lange on 2012-10-23

Correct coding convention

2728. By Jonathan Lange on 2012-10-23

Use absolute_import.

2729. By Jonathan Lange on 2012-10-23

Duplicate _EverythingGetter, rather than import it.

[Jonathan Lange (jml)]

1. duplicate & simplify _EverythingGetter, rather than import it
 Done.

2. change mod_reactor import to modReactor (or similar), and change both __import__'s to __future__.absolute_import
 Done (except I should have said "mod_reporter", not "mod_reactor")

3. attempt to make the reporter interface all text, documenting it as such
 Done, I think.

4. attempt to unify the multiple import methods
 Done, sort of. Combined ioError and problemDecodingSource into one unexpectedError method. Syntax errors have special needs re output, so have left as-is.

I think we're good to merge now.

[lp:~jml/divmod.org/pyflakes-reporter]

2730. By Jonathan Lange on 2012-10-23

Wrap types in C{}

[Glyph Lefkowitz (glyph)]

This change looks good to me. Thanks, Jonathan.

[Jonathan Lange (jml)]

Thanks for the review!

I can't merge this branch, as I'm not in ~divmod-dev and thus cannot commit to lp:divmod.org. Would you mind merging it?

[Glyph Lefkowitz (glyph)]

I'd better not, but I've just added you to divmod-dev; I'm somewhat surprised you weren't a member already, as you are officially a Divmod alumnus too ;-).

=== modified file 'Pyflakes/bin/pyflakes'
--- Pyflakes/bin/pyflakes	2008-08-28 14:33:07 +0000
+++ Pyflakes/bin/pyflakes	2012-10-23 13:09:20 +0000
@@ -1,4 +1,3 @@
 #!/usr/bin/python
-
 from pyflakes.scripts.pyflakes import main
 main()

=== added file 'Pyflakes/pyflakes/reporter.py'
--- Pyflakes/pyflakes/reporter.py	1970-01-01 00:00:00 +0000
+++ Pyflakes/pyflakes/reporter.py	2012-10-23 13:09:20 +0000
@@ -0,0 +1,79 @@
+# (c) 2005-2012 Divmod, Inc.
+# See LICENSE file for details
+
+import sys
+
+
+class Reporter(object):
+    """
+    Formats the results of pyflakes checks to users.
+    """
+
+    def __init__(self, warningStream, errorStream):
+        """
+        Construct a L{Reporter}.
+
+        @param warningStream: A file-like object where warnings will be
+            written to.  The stream's C{write} method must accept unicode.
+            C{sys.stdout} is a good value.
+        @param errorStream: A file-like object where error output will be
+            written to.  The stream's C{write} method must accept unicode.
+            C{sys.stderr} is a good value.
+        """
+        self._stdout = warningStream
+        self._stderr = errorStream
+
+
+    def unexpectedError(self, filename, msg):
+        """
+        An unexpected error occurred trying to process C{filename}.
+
+        @param filename: The path to a file that we could not process.
+        @ptype filename: C{unicode}
+        @param msg: A message explaining the problem.
+        @ptype msg: C{unicode}
+        """
+        self._stderr.write(u"%s: %s\n" % (filename, msg))
+
+
+    def syntaxError(self, filename, msg, lineno, offset, text):
+        """
+        There was a syntax errror in C{filename}.
+
+        @param filename: The path to the file with the syntax error.
+        @ptype filename: C{unicode}
+        @param msg: An explanation of the syntax error.
+        @ptype msg: C{unicode}
+        @param lineno: The line number where the syntax error occurred.
+        @ptype lineno: C{int}
+        @param offset: The column on which the syntax error occurred.
+        @ptype offset: C{int}
+        @param text: The source code containing the syntax error.
+        @ptype text: C{unicode}
+        """
+        line = text.splitlines()[-1]
+        if offset is not None:
+            offset = offset - (len(text) - len(line))
+        self._stderr.write(u'%s:%d: %s\n' % (filename, lineno, msg))
+        self._stderr.write(line)
+        self._stderr.write(u'\n')
+        if offset is not None:
+            self._stderr.write(u" " * (offset + 1) + u"^\n")
+
+
+    def flake(self, message):
+        """
+        pyflakes found something wrong with the code.
+
+        @param: A L{pyflakes.messages.Message}.
+        """
+        self._stdout.write(unicode(message))
+        self._stdout.write(u'\n')
+
+
+
+def _makeDefaultReporter():
+    """
+    Make a reporter that can be used when no reporter is specified.
+    """
+    return Reporter(sys.stdout, sys.stderr)

=== modified file 'Pyflakes/pyflakes/scripts/pyflakes.py'
--- Pyflakes/pyflakes/scripts/pyflakes.py	2010-04-13 14:53:04 +0000
+++ Pyflakes/pyflakes/scripts/pyflakes.py	2012-10-23 13:09:20 +0000
@@ -1,15 +1,18 @@
-
 """
 Implementation of the command-line I{pyflakes} tool.
 """
 
+from __future__ import absolute_import
+
 import sys
 import os
 import _ast
 
-checker = __import__('pyflakes.checker').checker
-
-def check(codeString, filename):
+from pyflakes import checker
+from pyflakes import reporter as modReporter
+
+
+def check(codeString, filename, reporter=None):
     """
     Check the Python source given by C{codeString} for flakes.
 
@@ -20,9 +23,14 @@
         errors.
     @type filename: C{str}
 
+    @param reporter: A L{Reporter} instance, where errors and warnings will be
+        reported.
+
     @return: The number of warnings emitted.
     @rtype: C{int}
     """
+    if reporter is None:
+        reporter = modReporter._makeDefaultReporter()
     # First, compile into an AST and handle syntax errors.
     try:
         tree = compile(codeString, filename, "exec", _ast.PyCF_ONLY_AST)
@@ -36,55 +44,79 @@
             # Avoid using msg, since for the only known case, it contains a
             # bogus message that claims the encoding the file declared was
             # unknown.
-            print >> sys.stderr, "%s: problem decoding source" % (filename, )
+            reporter.unexpectedError(filename, u'problem decoding source')
         else:
-            line = text.splitlines()[-1]
-
-            if offset is not None:
-                offset = offset - (len(text) - len(line))
-
-            print >> sys.stderr, '%s:%d: %s' % (filename, lineno, msg)
-            print >> sys.stderr, line
-
-            if offset is not None:
-                print >> sys.stderr, " " * offset, "^"
-
+            reporter.syntaxError(filename, msg, lineno, offset, text)
         return 1
     else:
         # Okay, it's syntactically valid.  Now check it.
         w = checker.Checker(tree, filename)
         w.messages.sort(lambda a, b: cmp(a.lineno, b.lineno))
         for warning in w.messages:
-            print warning
+            reporter.flake(warning)
         return len(w.messages)
 
 
-def checkPath(filename):
+def checkPath(filename, reporter=None):
     """
     Check the given path, printing out any warnings detected.
 
+    @param reporter: A L{Reporter} instance, where errors and warnings will be
+        reported.
+
     @return: the number of warnings printed
     """
+    if reporter is None:
+        reporter = modReporter._makeDefaultReporter()
     try:
-        return check(file(filename, 'U').read() + '\n', filename)
+        return check(file(filename, 'U').read() + '\n', filename, reporter)
     except IOError, msg:
-        print >> sys.stderr, "%s: %s" % (filename, msg.args[1])
+        reporter.unexpectedError(filename, msg.args[1])
         return 1
 
 
+
+def iterSourceCode(paths):
+    """
+    Iterate over all Python source files in C{paths}.
+
+    @param paths: A list of paths.  Directories will be recursed into and
+        any .py files found will be yielded.  Any non-directories will be
+        yielded as-is.
+    """
+    for path in paths:
+        if os.path.isdir(path):
+            for dirpath, dirnames, filenames in os.walk(path):
+                for filename in filenames:
+                    if filename.endswith('.py'):
+                        yield os.path.join(dirpath, filename)
+        else:
+            yield path
+
+
+
+def checkRecursive(paths, reporter):
+    """
+    Recursively check all source files in C{paths}.
+
+    @param paths: A list of paths to Python source files and directories
+        containing Python source files.
+    @param reporter: A L{Reporter} where all of the warnings and errors
+        will be reported to.
+    @return: The number of warnings found.
+    """
+    warnings = 0
+    for sourcePath in iterSourceCode(paths):
+        warnings += checkPath(sourcePath, reporter)
+    return warnings
+
+
+
 def main():
-    warnings = 0
     args = sys.argv[1:]
+    reporter = modReporter._makeDefaultReporter()
     if args:
-        for arg in args:
-            if os.path.isdir(arg):
-                for dirpath, dirnames, filenames in os.walk(arg):
-                    for filename in filenames:
-                        if filename.endswith('.py'):
-                            warnings += checkPath(os.path.join(dirpath, filename))
-            else:
-                warnings += checkPath(arg)
+        warnings = checkRecursive(args, reporter)
     else:
-        warnings += check(sys.stdin.read(), '<stdin>')
-
+        warnings = check(sys.stdin.read(), '<stdin>', reporter)
     raise SystemExit(warnings > 0)

=== modified file 'Pyflakes/pyflakes/test/test_script.py'
--- Pyflakes/pyflakes/test/test_script.py	2009-06-17 20:58:48 +0000
+++ Pyflakes/pyflakes/test/test_script.py	2012-10-23 13:09:20 +0000
@@ -1,51 +1,296 @@
-
 """
 Tests for L{pyflakes.scripts.pyflakes}.
 """
 
+import os
 import sys
 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 pyflakes.scripts.pyflakes import checkPath
-
-def withStderrTo(stderr, f):
+from pyflakes.messages import UnusedImport
+from pyflakes.reporter import Reporter
+from pyflakes.scripts.pyflakes import (
+    checkPath,
+    checkRecursive,
+    iterSourceCode,
+    )
+
+
+def withStderrTo(stderr, f, *args, **kwargs):
     """
     Call C{f} with C{sys.stderr} redirected to C{stderr}.
     """
     (outer, sys.stderr) = (sys.stderr, stderr)
     try:
-        return f()
+        return f(*args, **kwargs)
     finally:
         sys.stderr = outer
 
 
 
+class LoggingReporter(object):
+    """
+    Implementation of Reporter that just appends any errors to a list.
+    """
+
+    def __init__(self, log):
+        """
+        Construct a C{LoggingReporter}.
+
+        @param log: A list to append log messages to.
+        """
+        self.log = log
+
+
+    def flake(self, message):
+        self.log.append(('flake', str(message)))
+
+
+    def unexpectedError(self, filename, message):
+        self.log.append(('unexpectedError', filename, message))
+
+
+    def syntaxError(self, filename, msg, lineno, offset, line):
+        self.log.append(('syntaxError', filename, msg, lineno, offset, line))
+
+
+
+class TestIterSourceCode(TestCase):
+    """
+    Tests for L{iterSourceCode}.
+    """
+
+    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])), [])
+
+
+    def test_singleFile(self):
+        """
+        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])
+
+
+    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])), [])
+
+
+    def test_recurses(self):
+        """
+        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()
+        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]))
+
+
+    def test_multipleDirectories(self):
+        """
+        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()
+        self.assertEqual(
+            sorted(iterSourceCode([foo.path, bar.path])),
+            sorted([foo.child('a.py').path,
+                    bar.child('b.py').path]))
+
+
+    def test_explicitFiles(self):
+        """
+        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])
+
+
+
+class TestReporter(TestCase):
+    """
+    Tests for L{Reporter}.
+    """
+
+    def test_syntaxError(self):
+        """
+        C{syntaxError} reports that there was a syntax error in the source
+        file.  It reports to the error stream and includes the filename, line
+        number, error message, actual line of source and a caret pointing to
+        where the error is.
+        """
+        err = StringIO()
+        reporter = Reporter(None, err)
+        reporter.syntaxError('foo.py', 'a problem', 3, 4, 'bad line of source')
+        self.assertEquals(
+            ("foo.py:3: a problem\n"
+             "bad line of source\n"
+             "     ^\n"),
+            err.getvalue())
+
+
+    def test_syntaxErrorNoOffset(self):
+        """
+        C{syntaxError} doesn't include a caret pointing to the error if
+        C{offset} is passed as C{None}.
+        """
+        err = StringIO()
+        reporter = Reporter(None, err)
+        reporter.syntaxError('foo.py', 'a problem', 3, None,
+                             'bad line of source')
+        self.assertEquals(
+            ("foo.py:3: a problem\n"
+             "bad line of source\n"),
+            err.getvalue())
+
+
+    def test_multiLineSyntaxError(self):
+        """
+        If there's a multi-line syntax error, then we only report the last
+        line.  The offset is adjusted so that it is relative to the start of
+        the last line.
+        """
+        err = StringIO()
+        lines = [
+            'bad line of source',
+            'more bad lines of source',
+            ]
+        reporter = Reporter(None, err)
+        reporter.syntaxError('foo.py', 'a problem', 3, len(lines[0]) + 5,
+                             '\n'.join(lines))
+        self.assertEquals(
+            ("foo.py:3: a problem\n" +
+             lines[-1] + "\n" +
+             "     ^\n"),
+            err.getvalue())
+
+
+    def test_unexpectedError(self):
+        """
+        C{unexpectedError} reports an error processing a source file.
+        """
+        err = StringIO()
+        reporter = Reporter(None, err)
+        reporter.unexpectedError(u'source.py', u'error message')
+        self.assertEquals(u'source.py: error message\n', err.getvalue())
+
+
+    def test_flake(self):
+        """
+        C{flake} reports a code warning from Pyflakes.  It is exactly the
+        str() of a L{pyflakes.messages.Message}.
+        """
+        out = StringIO()
+        reporter = Reporter(out, None)
+        message = UnusedImport('foo.py', 42, 'bar')
+        reporter.flake(message)
+        self.assertEquals(out.getvalue(), "%s\n" % (message,))
+
+
+
 class CheckTests(TestCase):
     """
     Tests for L{check} and L{checkPath} which check a file for flakes.
     """
+
+    def makeTempFile(self, content):
+        """
+        Make a temporary file containing C{content} and return a path to it.
+        """
+        path = FilePath(self.mktemp())
+        path.setContent(content)
+        return path.path
+
+
+    def assertHasErrors(self, path, errorList):
+        """
+        Assert that C{path} causes errors.
+
+        @param path: A path to a file to check.
+        @param errorList: A list of errors expected to be printed to stderr.
+        """
+        err = StringIO()
+        count = withStderrTo(err, checkPath, path)
+        self.assertEquals(
+            (count, err.getvalue()), (len(errorList), ''.join(errorList)))
+
+
+    def getErrors(self, path):
+        """
+        Get any warnings or errors reported by pyflakes for the file at C{path}.
+
+        @param path: The path to a Python file on disk that pyflakes will check.
+        @return: C{(count, log)}, where C{count} is the number of warnings or
+            errors generated, and log is a list of those warnings, presented
+            as structured data.  See L{LoggingReporter} for more details.
+        """
+        log = []
+        reporter = LoggingReporter(log)
+        count = checkPath(path, reporter)
+        return count, log
+
+
     def test_missingTrailingNewline(self):
         """
         Source which doesn't end with a newline shouldn't cause any
         exception to be raised nor an error indicator to be returned by
         L{check}.
         """
-        fName = self.mktemp()
-        FilePath(fName).setContent("def foo():\n\tpass\n\t")
-        self.assertFalse(checkPath(fName))
+        fName = self.makeTempFile("def foo():\n\tpass\n\t")
+        self.assertHasErrors(fName, [])
 
 
     def test_checkPathNonExisting(self):
         """
         L{checkPath} handles non-existing files.
         """
-        err = StringIO()
-        count = withStderrTo(err, lambda: checkPath('extremo'))
-        self.assertEquals(err.getvalue(), 'extremo: No such file or directory\n')
+        count, errors = self.getErrors('extremo')
         self.assertEquals(count, 1)
+        self.assertEquals(
+            errors,
+            [('unexpectedError', 'extremo', 'No such file or directory')])
 
 
     def test_multilineSyntaxError(self):
@@ -72,19 +317,14 @@
         exc = self.assertRaises(SyntaxError, evaluate, source)
         self.assertTrue(exc.text.count('\n') > 1)
 
-        sourcePath = FilePath(self.mktemp())
-        sourcePath.setContent(source)
-        err = StringIO()
-        count = withStderrTo(err, lambda: checkPath(sourcePath.path))
-        self.assertEqual(count, 1)
-
-        self.assertEqual(
-            err.getvalue(),
-            """\
+        sourcePath = self.makeTempFile(source)
+        self.assertHasErrors(
+            sourcePath, ["""\
 %s:8: invalid syntax
     '''quux'''
            ^
-""" % (sourcePath.path,))
+"""
+                % (sourcePath,)])
 
 
     def test_eofSyntaxError(self):
@@ -92,19 +332,14 @@
         The error reported for source files which end prematurely causing a
         syntax error reflects the cause for the syntax error.
         """
-        source = "def foo("
-        sourcePath = FilePath(self.mktemp())
-        sourcePath.setContent(source)
-        err = StringIO()
-        count = withStderrTo(err, lambda: checkPath(sourcePath.path))
-        self.assertEqual(count, 1)
-        self.assertEqual(
-            err.getvalue(),
-            """\
+        sourcePath = self.makeTempFile("def foo(")
+        self.assertHasErrors(
+            sourcePath,
+            ["""\
 %s:1: unexpected EOF while parsing
 def foo(
          ^
-""" % (sourcePath.path,))
+""" % (sourcePath,)])
 
 
     def test_nonDefaultFollowsDefaultSyntaxError(self):
@@ -117,17 +352,13 @@
 def foo(bar=baz, bax):
     pass
 """
-        sourcePath = FilePath(self.mktemp())
-        sourcePath.setContent(source)
-        err = StringIO()
-        count = withStderrTo(err, lambda: checkPath(sourcePath.path))
-        self.assertEqual(count, 1)
-        self.assertEqual(
-            err.getvalue(),
-            """\
+        sourcePath = self.makeTempFile(source)
+        self.assertHasErrors(
+            sourcePath,
+            ["""\
 %s:1: non-default argument follows default argument
 def foo(bar=baz, bax):
-""" % (sourcePath.path,))
+""" % (sourcePath,)])
 
 
     def test_nonKeywordAfterKeywordSyntaxError(self):
@@ -139,32 +370,40 @@
         source = """\
 foo(bar=baz, bax)
 """
-        sourcePath = FilePath(self.mktemp())
-        sourcePath.setContent(source)
-        err = StringIO()
-        count = withStderrTo(err, lambda: checkPath(sourcePath.path))
-        self.assertEqual(count, 1)
-        self.assertEqual(
-            err.getvalue(),
-            """\
+        sourcePath = self.makeTempFile(source)
+        self.assertHasErrors(
+            sourcePath,
+            ["""\
 %s:1: non-keyword arg after keyword arg
 foo(bar=baz, bax)
-""" % (sourcePath.path,))
+""" % (sourcePath,)])
 
 
     def test_permissionDenied(self):
         """
-        If the a source file is not readable, this is reported on standard
+        If the source file is not readable, this is reported on standard
         error.
         """
         sourcePath = FilePath(self.mktemp())
         sourcePath.setContent('')
         sourcePath.chmod(0)
-        err = StringIO()
-        count = withStderrTo(err, lambda: checkPath(sourcePath.path))
-        self.assertEquals(count, 1)
-        self.assertEquals(
-            err.getvalue(), "%s: Permission denied\n" % (sourcePath.path,))
+        count, errors = self.getErrors(sourcePath.path)
+        self.assertEquals(count, 1)
+        self.assertEquals(
+            errors,
+            [('unexpectedError', sourcePath.path, "Permission denied")])
+
+
+    def test_pyflakesWarning(self):
+        """
+        If the source file has a pyflakes warning, this is reported as a
+        'flake'.
+        """
+        sourcePath = self.makeTempFile("import foo")
+        count, errors = self.getErrors(sourcePath)
+        self.assertEquals(count, 1)
+        self.assertEquals(
+            errors, [('flake', str(UnusedImport(sourcePath, 1, 'foo')))])
 
 
     def test_misencodedFile(self):
@@ -176,10 +415,149 @@
 # coding: ascii
 x = "\N{SNOWMAN}"
 """.encode('utf-8')
-        sourcePath = FilePath(self.mktemp())
-        sourcePath.setContent(source)
-        err = StringIO()
-        count = withStderrTo(err, lambda: checkPath(sourcePath.path))
-        self.assertEquals(count, 1)
-        self.assertEquals(
-            err.getvalue(), "%s: problem decoding source\n" % (sourcePath.path,))
+        sourcePath = self.makeTempFile(source)
+        self.assertHasErrors(
+            sourcePath, ["%s: problem decoding source\n" % (sourcePath,)])
+
+
+    def test_checkRecursive(self):
+        """
+        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")
+        log = []
+        reporter = LoggingReporter(log)
+        warnings = checkRecursive([tempdir.path], reporter)
+        self.assertEqual(warnings, 2)
+        self.assertEqual(
+            sorted(log),
+            sorted([('flake', str(UnusedImport(file1.path, 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))
+
+
+
+class IntegrationTests(TestCase):
+    """
+    Tests of the pyflakes script that actually spawn the script.
+    """
+
+    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
+
+
+    def runPyflakes(self, paths, stdin=None):
+        """
+        Launch a subprocess running C{pyflakes}.
+
+        @param args: Command-line arguments to pass to pyflakes.
+        @param kwargs: Options passed on to C{subprocess.Popen}.
+        @return: C{(returncode, stdout, stderr)} of the completed pyflakes
+            process.
+        """
+        env = dict(os.environ)
+        env['PYTHONPATH'] = os.pathsep.join(sys.path)
+        command = [self.getPyflakesBinary()]
+        command.extend(paths)
+        if stdin:
+            d = _callProtocolWithDeferred(
+                lambda d: _EverythingGetterWithStdin(d, stdin),
+                sys.executable, command, env=env, path=None)
+        else:
+            d = getProcessOutputAndValue(sys.executable, command, env=env)
+        return d
+
+
+    def test_goodFile(self):
+        """
+        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))
+
+
+    def test_fileWithFlakes(self):
+        """
+        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))
+
+
+    def test_errors(self):
+        """
+        When pyflakes finds errors with the files it's given, (if they don't
+        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))
+
+
+    def test_readFromStdin(self):
+        """
+        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))