Description should be wrapped at 72 characters?

Double quotes are used in some places where it should be single quotes

Single quotes?

Review Bot instead of reviewbot

Just being picky but is it not in the same order as the credentials check test (i.e. review and expected_comments)

"Review Bot" is the modern titling for the product, so the description should be reflective of that. (The repository has the old name for legacy purposes.)

This doesn't appear related to the change.

nose is a third-party module, so it should be part of its own import group.

First line of the file should be a docstring summary of the file.

unittest is part of Python, so it should be in its own import group (which would come before mock's).

/ isn't a safe assumption for path separators. Instead, whenever we need a path, we should build it with os.path.join

There's too much happening in this file and class. It'll get unwieldy as we go.
What we should do is have:

A testcase.py file in bot/reviewbot/tools/tests/ that contains a base BaseToolTestCase class that tool-specific tests can inherit from. This should only contain generic methods that are useful for tool-related test suites.
A file per tool, named after the tool, that contains a test case class subclassing BaseToolTestCase that only contains functionality specific to that test.

We use this pattern in most of our code elsewhere.

"Review Bot"
Missing a period.

The space at the beginning of the summary must be removed.

These must be valid Python types, so: list of dict

This doesn't quite match the way we do argument summaries. Something more like:
Each dictionary represents a file in the review. It must have the following keys:

``dest_file`` (:py:class:`unicode`):
    The filename of the diff being reviewed.

 ``patched_file_contents`` (:py:class:`bytes`):
     The contents of the diff for the file.

Something like that.

It would be really nice not to use Mock. While mocks can be useful, they can also make it increasingly harder to test real-world stuff. Instead, since we control the Review and File classes, we should be using those instead. We can always use KGB to spy on any API requests to avoid those requests being made.
By having the proper objects, we get increased testing of those objects, and we don't have to start going through and mocking more and more and more behavior as we add tests (diverging even further from core functionality and behavior and more toward mock-specific behavior).

Blank line between statements and blocks.

Blank line between statements and blocks.

Blank line between blocks.

Blank line between statements and blocks.

Blank line between statements and blocks.

Docstring summaries must be confined to one line. This can be:
Assert that comments in a review match expected comments.

dict

For the tuples, turn them into literals, like:
... and value is a tuple in the form of
``(comment, line, issue)`` or ``(comment, line)``

so it's more clear.

Needs to be bool, optional, since the argument doesn't have to be provided.
The content is also indented too far.

When we get rid of mocks, and use kgb, we'll be able to get rid of the if and just have the one loop and assertion, since kgb does the right thing with these calls and argument defaults.

Must be one line.

Make sure blank lines, indentation, types, and formatting are correct. Also, don't forget the , optional optional arguments.
Types that are classes must be the absolute module path to the class.

unicode, not string.

Please use kgb instead of mocks here.
Also, these are very specific to flake8, which means this function is useless for every other type of tool. Either this should exist in a Flake8-specific test case, or should be made generic.

This must exist in a test case (and file) specific to this tool.

The format for testing docstrings should be in the form of:

"""Testing <toolname> with <condition>"""


This helps keep the output consistent and readable.
Also, no period at the end.

This must be in a Flake8-specific test case and file.