Add automatic linting. (#622)

* Make pylint and yapf happy with test_xsrftool.py.

* Make pylint and yapf happy with test_searcher.py.

* Update lint tool.

* Add partial linting to Travis tests.

* leave TODOs alone

* add comment on separate yapf and pylint lists

* Run lint tool under Python 2.

* Add python-setuptools to Travis install step.

Seems to be required with the older version of Python 2.7.

* Revert "Add python-setuptools to Travis install step."

This reverts commit c6360b4.

It didn't help.

* Add lint script to tools/all_tests.

* Run style checks first for Travis.

* Try updating setuptools.

Suggested here for someone else with a similar (or identical) problem:
pylint-dev/astroid#358 (comment)

* Use pylint 1.9.2, because it supports Python 2.

.travis.yml

@@ -28,7 +28,7 @@ install:
       unzip
   - wget https://storage.googleapis.com/appengine-sdks/featured/google_appengine_1.9.83.zip -O /tmp/appengine.zip
   - unzip -qq /tmp/appengine.zip
-  - pip install cssselect lxml mock modernize pillow==4.1.0 pytest
+  - pip install cssselect lxml mock modernize pillow==4.1.0 pylint==1.9.2 pytest yapf
   - npm --prefix ui install
 
 before_script:

tests/test_searcher.py

@@ -1,10 +1,15 @@
-from mock import patch, MagicMock
+"""Tests for the Searcher."""
+
 import unittest
 
+import mock
+
 from search.searcher import Searcher
 
 
 class SearcherTests(unittest.TestCase):
+    # pylint: disable=no-self-use
+    """Test cases for searcher.Searcher."""
 
     # Argument values
     REPO_NAME = 'haiti'
@@ -15,33 +20,31 @@ class SearcherTests(unittest.TestCase):
 
     def test_full_text_search_results(self):
         """Use full_text_search.search results when enabled."""
-        with patch('full_text_search.search') as full_text_search_mock, \
-             patch('indexing.search') as indexing_mock:
+        with mock.patch('full_text_search.search') as full_text_search_mock:
             full_text_search_mock.return_value = SearcherTests.FULLTEXT_RETURN_VALUE
             searcher = Searcher(
                 SearcherTests.REPO_NAME,
                 enable_fulltext_search=True,
                 max_results=SearcherTests.MAX_RESULTS)
-            assert (searcher.search('matt') ==
-                    SearcherTests.FULLTEXT_RETURN_VALUE)
+            assert (
+                searcher.search('matt') == SearcherTests.FULLTEXT_RETURN_VALUE)
             assert len(full_text_search_mock.call_args_list) == 1
             call_args, _ = full_text_search_mock.call_args_list[0]
             assert call_args[0] == SearcherTests.REPO_NAME
             assert call_args[1] == {'name': 'matt'}
             assert call_args[2] == SearcherTests.MAX_RESULTS
 
-    def test_full_text_search_results_with_location(self):
+    def test_full_text_results_with_loc(self):
         """Use full_text_search.search results when enabled, including location.
         """
-        with patch('full_text_search.search') as full_text_search_mock, \
-             patch('indexing.search') as indexing_mock:
+        with mock.patch('full_text_search.search') as full_text_search_mock:
             full_text_search_mock.return_value = SearcherTests.FULLTEXT_RETURN_VALUE
             searcher = Searcher(
                 SearcherTests.REPO_NAME,
                 enable_fulltext_search=True,
                 max_results=SearcherTests.MAX_RESULTS)
-            assert (searcher.search('matt', 'schenectady') ==
-                    SearcherTests.FULLTEXT_RETURN_VALUE)
+            assert (searcher.search(
+                'matt', 'schenectady') == SearcherTests.FULLTEXT_RETURN_VALUE)
             assert len(full_text_search_mock.call_args_list) == 1
             call_args, _ = full_text_search_mock.call_args_list[0]
             assert call_args[0] == SearcherTests.REPO_NAME
@@ -53,35 +56,33 @@ def test_indexing_results(self):
 
         When full-text search is disabled, fall back to indexing.search.
         """
-        with patch('full_text_search.search') as full_text_search_mock, \
-             patch('indexing.search') as indexing_mock:
+        with mock.patch('indexing.search') as indexing_mock:
             indexing_mock.return_value = SearcherTests.INDEXING_RETURN_VALUE
             searcher = Searcher(
                 SearcherTests.REPO_NAME,
                 enable_fulltext_search=False,
                 max_results=SearcherTests.MAX_RESULTS)
-            assert (searcher.search('matt') ==
-                    SearcherTests.INDEXING_RETURN_VALUE)
+            assert (
+                searcher.search('matt') == SearcherTests.INDEXING_RETURN_VALUE)
             assert len(indexing_mock.call_args_list) == 1
             call_args, _ = indexing_mock.call_args_list[0]
             assert call_args[0] == SearcherTests.REPO_NAME
             assert call_args[1].query == 'matt'
             assert call_args[2] == SearcherTests.MAX_RESULTS
 
-    def test_indexing_results_with_location(self):
+    def test_indexing_results_with_loc(self):
         """Fall back to indexing.search results, including a location value.
 
         When full-text search is disabled, fall back to indexing.search.
         """
-        with patch('full_text_search.search') as full_text_search_mock, \
-             patch('indexing.search') as indexing_mock:
+        with mock.patch('indexing.search') as indexing_mock:
             indexing_mock.return_value = SearcherTests.INDEXING_RETURN_VALUE
             searcher = Searcher(
                 SearcherTests.REPO_NAME,
                 enable_fulltext_search=False,
                 max_results=SearcherTests.MAX_RESULTS)
-            assert (searcher.search('matt', 'schenectady') ==
-                    SearcherTests.INDEXING_RETURN_VALUE)
+            assert (searcher.search(
+                'matt', 'schenectady') == SearcherTests.INDEXING_RETURN_VALUE)
             assert len(indexing_mock.call_args_list) == 1
             call_args, _ = indexing_mock.call_args_list[0]
             assert call_args[0] == SearcherTests.REPO_NAME

tests/test_xsrftool.py

@@ -1,3 +1,5 @@
+"""Tests for the XSRF tool."""
+
 import datetime
 import unittest
 
@@ -6,46 +8,62 @@
 
 
 class XsrfToolTests(unittest.TestCase):
+    """Test cases for utils.XsrfTool."""
 
     TEST_NOW = datetime.datetime(2010, 1, 31, 18, 0, 0)
 
     def setUp(self):
         utils.set_utcnow_for_test(XsrfToolTests.TEST_NOW)
 
-    def testGenerateAndVerifyGoodToken(self):
+    def test_gen_and_verify_good_token(self):
+        """Tests generating and verifying a good token."""
         config.set(xsrf_token_key='abcdef')
         tool = utils.XsrfTool()
         token = tool.generate_token(12345, 'test_action')
         self.assertTrue(tool.verify_token(token, 12345, 'test_action'))
 
-    def testRejectsInvalidToken(self):
+    def test_rejects_invalid_token(self):
+        """Tests that an invalid token is rejected."""
         config.set(xsrf_token_key='abcdef')
         tool = utils.XsrfTool()
         timestamp = utils.get_timestamp(XsrfToolTests.TEST_NOW)
-        self.assertFalse(tool.verify_token(
-            'NotTheRightDigest/%f' % timestamp, 12345, 'test_action'))
+        self.assertFalse(
+            tool.verify_token('NotTheRightDigest/%f' % timestamp, 12345,
+                              'test_action'))
 
-    def testRejectsExpiredToken(self):
+    def test_rejects_expired_token(self):
+        """Tests that an expired token is rejected."""
         config.set(xsrf_token_key='abcdef')
         tool = utils.XsrfTool()
         token = tool.generate_token(12345, 'test_action')
-        utils.set_utcnow_for_test(
-            XsrfToolTests.TEST_NOW + datetime.timedelta(hours=4, minutes=1))
+        utils.set_utcnow_for_test(XsrfToolTests.TEST_NOW +
+                                  datetime.timedelta(hours=4, minutes=1))
         self.assertFalse(tool.verify_token(token, 12345, 'test_action'))
 
-    def testGoodTokenWithNoPriorTokenKey(self):
+    def test_good_with_no_prior_key(self):
+        """Tests a good token when a token key has to be autogenerated.
+
+        If the config doesn't already have an XSRF token key set, the XSRF tool
+        will generate one automatically.
+        """
         # config seems to be shared across tests, so we have to specifically set
         # it to None.
         config.set(xsrf_token_key=None)
         tool = utils.XsrfTool()
         token = tool.generate_token(12345, 'test_action')
         self.assertTrue(tool.verify_token(token, 12345, 'test_action'))
 
-    def testBadTokenWithNoPriorTokenKey(self):
+    def test_bad_with_no_prior_key(self):
+        """Tests a bad token when a token key has to be autogenerated.
+
+        If the config doesn't already have an XSRF token key set, the XSRF tool
+        will generate one automatically.
+        """
         # config seems to be shared across tests, so we have to specifically set
         # it to None.
         config.set(xsrf_token_key=None)
         tool = utils.XsrfTool()
         timestamp = utils.get_timestamp(XsrfToolTests.TEST_NOW)
-        self.assertFalse(tool.verify_token(
-            'NotTheRightDigest/%f' % timestamp, 12345, 'test_action'))
+        self.assertFalse(
+            tool.verify_token('NotTheRightDigest/%f' % timestamp, 12345,
+                              'test_action'))

tools/all_tests

@@ -17,7 +17,8 @@
 
 pushd "$(dirname $0)" >/dev/null && source common.sh && popd >/dev/null
 
-$TOOLS_DIR/unit_tests -q && \
+$TOOLS_DIR/lint pylint-check && $TOOLS_DIR/lint yapf-check && \
+    $TOOLS_DIR/unit_tests -q && \
     $TOOLS_DIR/py3_unit_tests -q && \
     $TOOLS_DIR/server_tests -q && \
     $TOOLS_DIR/python3_compatibility_check && \

tools/lint

@@ -1,15 +1,38 @@
-#!/bin/sh
-
-# use pylint to check the args.
-# requires installing pylint http://www.logilab.org/project/pylint
-# short form: easy_install pylint (may require root)
-LINT=$(which pylint)
-REPORTS="no"
-DEPRECATED=regsub,string,TERMIOS,Bastion,rexec
-if [ -z "$LINT" ]; then
-   echo "pylint not found, please install."
-   exit 1
-fi
+#!/bin/bash
 
 pushd "$(dirname $0)" >/dev/null && source common.sh && popd >/dev/null
-${LINT} --deprecated-module=${DEPRECATED} --reports=${REPORTS} "$@" 
+
+# yapf can fix things automatically, but pylint is more thorough. I expect the
+# set of things yapf is happy with will grow faster than the set of things
+# pylint is happy with, so we separate these lists.
+DEFAULT_YAPF_FILES="tests/test_searcher.py \
+                    tests/test_xsrftool.py"
+DEFAULT_PYLINT_FILES="tests/test_searcher.py \
+                      tests/test_xsrftool.py"
+
+# Call through python -m instead of directly, so that you're sure to be able to
+# use it even if you installed the tools through pip.
+YAPF="python -m yapf --style google"
+# pylint will complain about TODOs, so disable that with "--disable fixme"
+PYLINT="python -m pylint --disable fixme"
+
+command="$1"
+file_list="${@:2:999}"
+
+if [ "$file_list" == "" ]; then
+    if [ "$command" == "yapf-fix" -o "$command" == "yapf-check" ]; then
+        file_list=$DEFAULT_YAPF_FILES
+    elif [ "$command" == "pylint-check" ]; then
+        file_list=$DEFAULT_PYLINT_FILES
+    fi
+fi
+
+if [ "$command" == "yapf-fix" ]; then
+    $YAPF -i $file_list
+elif [ "$command" == "yapf-check" ]; then
+    $YAPF -d $file_list
+elif [ "$command" == "pylint-check" ]; then
+    $PYLINT $file_list
+else
+    echo "Usage: tools/lint [yapf-fix|yapf-check|pylint-check]"
+fi

tools/travis_tests

@@ -23,15 +23,16 @@ pushd "$(dirname $0)" >/dev/null && source common.sh && popd >/dev/null
 case "${TRAVIS_PYTHON_VERSION:0:1}" in
 
     "2")
-        $TOOLS_DIR/unit_tests -q && \
+        $TOOLS_DIR/lint pylint-check && $TOOLS_DIR/lint yapf-check && \
+            $TOOLS_DIR/unit_tests -q && \
             $TOOLS_DIR/server_tests -q && \
             $TOOLS_DIR/python3_compatibility_check && \
             $TOOLS_DIR/ui test && \
-            echo "All tests passed."
+            echo "All Python 2.7 tests and checks passed."
         ;;
     "3")
         $TOOLS_DIR/py3_unit_tests -q &&
-            echo "All Python 3.7 tests passed."
+            echo "All Python 3.7 tests and checks passed."
         ;;
     *)
         echo "Unrecognized Python version from Travis: $TRAVIS_PYTHON_VERSION"