From f163c85fb5858fcd21591eb35fe7615c02f7e549 Mon Sep 17 00:00:00 2001 From: nednguyen Date: Mon, 11 May 2015 21:15:43 -0700 Subject: [PATCH] Add PRESUBMIT with pylint check for web-page-replay. Review URL: https://codereview.appspot.com/234420043 --- PRESUBMIT.py | 28 ++++++++++++++++++++++++++++ certutils.py | 6 +++--- customhandlers.py | 3 ++- daemonserver.py | 3 ++- dnsproxy.py | 13 +++++-------- httparchive.py | 4 ++-- httparchive_test.py | 3 --- httpclient.py | 2 +- httpproxy.py | 11 ++++++++--- httpproxy_test.py | 2 +- platformsettings.py | 13 ++++++++++--- platformsettings_test.py | 1 + proxyshaper_test.py | 12 ++++++------ pylintrc | 17 +++++++++++++++++ replay.py | 5 ++--- servermanager.py | 5 +++-- sslproxy.py | 1 + test_runner.py | 1 - third_party/__init__.py | 1 + trafficshaper.py | 4 ++-- trafficshaper_test.py | 14 ++++++-------- 21 files changed, 101 insertions(+), 48 deletions(-) create mode 100644 PRESUBMIT.py create mode 100644 pylintrc diff --git a/PRESUBMIT.py b/PRESUBMIT.py new file mode 100644 index 0000000..5948576 --- /dev/null +++ b/PRESUBMIT.py @@ -0,0 +1,28 @@ +# Copyright 2015 The Chromium Authors. All rights reserved. +# Use of this source code is governed by a BSD-style license that can be +# found in the LICENSE file. + +"""Presubmit script for changes affecting tools/perf/. + +See http://dev.chromium.org/developers/how-tos/depottools/presubmit-scripts +for more details about the presubmit API built into depot_tools. +""" + +def _CommonChecks(input_api, output_api): + """Performs common checks, which includes running pylint.""" + results = [] + results.extend(input_api.canned_checks.RunPylint( + input_api, output_api, black_list=[], pylintrc='pylintrc')) + return results + + +def CheckChangeOnUpload(input_api, output_api): + report = [] + report.extend(_CommonChecks(input_api, output_api)) + return report + + +def CheckChangeOnCommit(input_api, output_api): + report = [] + report.extend(_CommonChecks(input_api, output_api)) + return report diff --git a/certutils.py b/certutils.py index 0daa331..33236e0 100644 --- a/certutils.py +++ b/certutils.py @@ -53,7 +53,7 @@ def get_ssl_context(method=SSL_METHOD): # One of: One of SSLv2_METHOD, SSLv3_METHOD, SSLv23_METHOD, or TLSv1_METHOD if openssl_import_error: - raise openssl_import_error + raise openssl_import_error # pylint: disable=raising-bad-type return SSL.Context(method) @@ -112,7 +112,7 @@ def generate_dummy_ca_cert(subject='_WebPageReplayCert'): certificate """ if openssl_import_error: - raise openssl_import_error + raise openssl_import_error # pylint: disable=raising-bad-type key = crypto.PKey() key.generate_key(crypto.TYPE_RSA, 1024) @@ -228,7 +228,7 @@ def generate_cert(root_ca_cert_str, server_cert_str, server_host): a PEM formatted certificate string """ if openssl_import_error: - raise openssl_import_error + raise openssl_import_error # pylint: disable=raising-bad-type common_name = server_host if server_cert_str: diff --git a/customhandlers.py b/customhandlers.py index 17e8756..14166af 100644 --- a/customhandlers.py +++ b/customhandlers.py @@ -28,7 +28,6 @@ import base64 import httparchive -import httplib import json import logging import os @@ -63,6 +62,7 @@ def __init__(self, options, http_archive): options: original options passed to the server. http_archive: reference to the HttpArchive object. """ + self.server_manager = None self.options = options self.http_archive = http_archive self.handlers = [ @@ -104,6 +104,7 @@ def get_generator_url_response_code(self, request, url_suffix): On a match, an ArchivedHttpResponse. Otherwise, None. """ + del request try: response_code = int(url_suffix) return SimpleResponse(response_code) diff --git a/daemonserver.py b/daemonserver.py index a0c98a4..371c654 100644 --- a/daemonserver.py +++ b/daemonserver.py @@ -25,7 +25,8 @@ def __enter__(self): # the components do not need to communicate with each other. On Linux, # "taskset" could be used to assign each process to specific CPU/core. # Of course, only bother with this if the processing speed is an issue. - # Some related discussion: http://stackoverflow.com/questions/990102/python-global-interpreter-lock-gil-workaround-on-multi-core-systems-using-tasks + # Some related discussion: http://stackoverflow.com/questions/990102/python- + # global-interpreter-lock-gil-workaround-on-multi-core-systems-using-tasks thread = threading.Thread(target=self.serve_forever) thread.daemon = True # Python exits when no non-daemon threads are left. thread.start() diff --git a/dnsproxy.py b/dnsproxy.py index dbf4fee..1a6dba4 100644 --- a/dnsproxy.py +++ b/dnsproxy.py @@ -21,13 +21,10 @@ import threading import time -import third_party -import dns.flags -import dns.message -import dns.rcode -import dns.resolver -import dns.rdatatype -import ipaddr +from third_party import dns +from third_party.dns import rdatatype +from third_party import ipaddr + class DnsProxyException(Exception): @@ -52,7 +49,7 @@ def _IsIPAddress(hostname): except socket.error: return False - def __call__(self, hostname, rdtype=dns.rdatatype.A): + def __call__(self, hostname, rdtype=rdatatype.A): """Return real IP for a host. Args: diff --git a/httparchive.py b/httparchive.py index 8d32c4c..80c403c 100755 --- a/httparchive.py +++ b/httparchive.py @@ -67,7 +67,7 @@ def wrapped(self, *args, **kwargs): try: return fn(self, *args, **kwargs) finally: - run_time = (time.time() - start_time) * 1000.0; + run_time = (time.time() - start_time) * 1000.0 logging.debug('%s: %dms', fn.__name__, run_time) return wrapped @@ -92,7 +92,7 @@ class HttpArchive(dict, persistentmixin.PersistentMixin): the archive to find potential matches. """ - def __init__(self): + def __init__(self): # pylint: disable=super-init-not-called self.responses_by_host = defaultdict(dict) def __setstate__(self, state): diff --git a/httparchive_test.py b/httparchive_test.py index 5bee333..cc6929c 100755 --- a/httparchive_test.py +++ b/httparchive_test.py @@ -13,12 +13,9 @@ # See the License for the specific language governing permissions and # limitations under the License. -import ast import calendar import email.utils import httparchive -import os -import time import unittest diff --git a/httpclient.py b/httpclient.py index c559e44..c650dc4 100644 --- a/httpclient.py +++ b/httpclient.py @@ -251,7 +251,7 @@ def _ToTuples(headers): logging.warning( 'Response header in wrong format [%s]', line) continue - name, value = name_value + name, value = name_value # pylint: disable=unpacking-non-sequence all_headers.append((name, value)) return all_headers diff --git a/httpproxy.py b/httpproxy.py index 4d04ac7..1e3a5b6 100644 --- a/httpproxy.py +++ b/httpproxy.py @@ -72,9 +72,14 @@ def setup(self): self.server.traffic_shaping_down_bps) # Make request handler logging match our logging format. - def log_request(self, code='-', size='-'): pass - def log_error(self, format, *args): logging.error(format, *args) - def log_message(self, format, *args): logging.info(format, *args) + def log_request(self, code='-', size='-'): + pass + + def log_error(self, format, *args): # pylint:disable=redefined-builtin + logging.error(format, *args) + + def log_message(self, format, *args): # pylint:disable=redefined-builtin + logging.info(format, *args) def read_request_body(self): request_body = None diff --git a/httpproxy_test.py b/httpproxy_test.py index d2f76d7..490cbb0 100644 --- a/httpproxy_test.py +++ b/httpproxy_test.py @@ -18,7 +18,6 @@ import httplib import httpproxy import threading -import time import unittest import util @@ -33,6 +32,7 @@ def __init__(self, response): self._response = response def handle(self, request): + del request return self._response diff --git a/platformsettings.py b/platformsettings.py index 626bcdd..7be1e86 100644 --- a/platformsettings.py +++ b/platformsettings.py @@ -37,7 +37,6 @@ import stat import subprocess import sys -import tempfile import time import urlparse @@ -65,8 +64,9 @@ class NotAdministratorError(PlatformSettingsError): class CalledProcessError(PlatformSettingsError): """Raised when a _check_output() process returns a non-zero exit status.""" def __init__(self, returncode, cmd): - self.returncode = returncode - self.cmd = cmd + super(CalledProcessError, self).__init__() + self.returncode = returncode + self.cmd = cmd def __str__(self): return 'Command "%s" returned non-zero exit status %d' % ( @@ -166,6 +166,7 @@ def get_httpproxy_ip_address(self, is_server_mode=False): def get_system_proxy(self, use_ssl): """Returns the system HTTP(S) proxy host, port.""" + del use_ssl return SystemProxy(None, None) def _ipfw_cmd(self): @@ -281,6 +282,9 @@ def set_temporary_primary_nameserver(self, nameserver): class _PosixPlatformSettings(_BasePlatformSettings): + # pylint: disable=abstract-method + # Suppress lint check for _get_primary_nameserver & _set_primary_nameserver + def rerun_as_administrator(self): """If needed, rerun the program with administrative privileges. @@ -573,6 +577,9 @@ def _set_primary_nameserver(self, dns): class _WindowsPlatformSettings(_BasePlatformSettings): + # pylint: disable=abstract-method + # Suppress lint check for _ipfw_cmd + def get_system_logging_handler(self): """Return a handler for the logging module (optional). diff --git a/platformsettings_test.py b/platformsettings_test.py index fa0e351..f95a67f 100755 --- a/platformsettings_test.py +++ b/platformsettings_test.py @@ -180,6 +180,7 @@ def test_has_sni(self): platformsettings.HasSniSupport() +# pylint: disable=abstract-method class Win7Settings(platformsettings._WindowsPlatformSettings): @classmethod def _ipconfig(cls, *args): diff --git a/proxyshaper_test.py b/proxyshaper_test.py index 744f482..3e8d664 100755 --- a/proxyshaper_test.py +++ b/proxyshaper_test.py @@ -44,7 +44,7 @@ class TimedTestCase(unittest.TestCase): - def assertAlmostEqual(self, expected, actual, tolerance=0.05): + def assertValuesAlmostEqual(self, expected, actual, tolerance=0.05): """Like the following with nicer default message: assertTrue(expected <= actual + tolerance && expected >= actual - tolerance) @@ -66,7 +66,7 @@ def testReadLimitedBasic(self): self.assertEqual(num_bytes, len(limited_f.read())) expected_ms = 8.0 * num_bytes / bps * 1000.0 actual_ms = (proxyshaper.TIMER() - start) * 1000.0 - self.assertAlmostEqual(expected_ms, actual_ms) + self.assertValuesAlmostEqual(expected_ms, actual_ms) def testReadlineLimitedBasic(self): num_bytes = 1024 * 8 + 512 @@ -78,7 +78,7 @@ def testReadlineLimitedBasic(self): self.assertEqual(num_bytes, len(limited_f.readline())) expected_ms = 8.0 * num_bytes / bps * 1000.0 actual_ms = (proxyshaper.TIMER() - start) * 1000.0 - self.assertAlmostEqual(expected_ms, actual_ms) + self.assertValuesAlmostEqual(expected_ms, actual_ms) def testReadLimitedSlowedByMultipleRequests(self): num_bytes = 1024 @@ -92,7 +92,7 @@ def testReadLimitedSlowedByMultipleRequests(self): self.assertEqual(num_bytes, len(num_read_bytes)) expected_ms = 8.0 * num_bytes / (bps / float(request_count)) * 1000.0 actual_ms = (proxyshaper.TIMER() - start) * 1000.0 - self.assertAlmostEqual(expected_ms, actual_ms) + self.assertValuesAlmostEqual(expected_ms, actual_ms) def testWriteLimitedBasic(self): num_bytes = 1024 * 10 + 350 @@ -105,7 +105,7 @@ def testWriteLimitedBasic(self): self.assertEqual(num_bytes, len(limited_f.getvalue())) expected_ms = 8.0 * num_bytes / bps * 1000.0 actual_ms = (proxyshaper.TIMER() - start) * 1000.0 - self.assertAlmostEqual(expected_ms, actual_ms) + self.assertValuesAlmostEqual(expected_ms, actual_ms) def testWriteLimitedSlowedByMultipleRequests(self): num_bytes = 1024 * 10 @@ -119,7 +119,7 @@ def testWriteLimitedSlowedByMultipleRequests(self): self.assertEqual(num_bytes, len(limited_f.getvalue())) expected_ms = 8.0 * num_bytes / (bps / float(request_count)) * 1000.0 actual_ms = (proxyshaper.TIMER() - start) * 1000.0 - self.assertAlmostEqual(expected_ms, actual_ms) + self.assertValuesAlmostEqual(expected_ms, actual_ms) class GetBitsPerSecondTest(unittest.TestCase): diff --git a/pylintrc b/pylintrc new file mode 100644 index 0000000..c861f7a --- /dev/null +++ b/pylintrc @@ -0,0 +1,17 @@ +[MESSAGES CONTROL] + +# Disable the message, report, category or checker with the given id(s). +# TODO(wpr-owners): Reduce this list to as small as possible. +disable=I0010,I0011,abstract-class-little-used,abstract-class-not-used,anomalous-backslash-in-string,bad-builtin,bad-context-manager,bad-continuation,bad-indentation,bad-str-strip-call,bad-whitespace,broad-except,cell-var-from-loop,deprecated-lambda,deprecated-module,duplicate-code,eval-used,exec-used,fixme,function-redefined,global-statement,interface-not-implemented,invalid-name,locally-enabled,logging-not-lazy,missing-docstring,missing-final-newline,no-init,no-member,no-name-in-module,no-self-use,no-self-use,not-callable,old-style-class,reimported,star-args,super-on-old-class,too-few-public-methods,too-many-ancestors,too-many-arguments,too-many-branches,too-many-function-args,too-many-instance-attributes,too-many-lines,too-many-locals,too-many-public-methods,too-many-return-statements,too-many-statements,trailing-whitespace,useless-else-on-loop,unused-variable,attribute-defined-outside-init,protected-access + + +[REPORTS] + +# Don't write out full reports, just messages. +reports=no + + +[FORMAT] + +# We use two spaces for indents, instead of the usual four spaces or tab. +indent-string=' ' diff --git a/replay.py b/replay.py index 67d38ef..873ab03 100755 --- a/replay.py +++ b/replay.py @@ -47,7 +47,6 @@ import sys import traceback -import certutils import customhandlers import dnsproxy import httparchive @@ -202,7 +201,7 @@ def _CheckValidIp(self, name): if value: try: socket.inet_aton(value) - except: + except Exception: self._parser.error('Option --%s must be a valid IPv4 address.' % name) def _CheckFeatureSupport(self): @@ -346,7 +345,7 @@ def replay(options, replay_filename): platformsettings.DnsUpdateError) as e: logging.critical('%s: %s', e.__class__.__name__, e) exit_status = 1 - except: + except Exception: logging.critical(traceback.format_exc()) exit_status = 2 diff --git a/servermanager.py b/servermanager.py index d9c3623..8bb9b3a 100644 --- a/servermanager.py +++ b/servermanager.py @@ -123,14 +123,15 @@ def Run(self): time.sleep(1) if self.should_exit: break - except: + except Exception: exception_info = sys.exc_info() finally: for server_exit in server_exits: try: if server_exit(*exception_info): exception_info = (None, None, None) - except: + except Exception: exception_info = sys.exc_info() if exception_info != (None, None, None): + # pylint: disable=raising-bad-type raise exception_info[0], exception_info[1], exception_info[2] diff --git a/sslproxy.py b/sslproxy.py index dd586d7..b284ff6 100755 --- a/sslproxy.py +++ b/sslproxy.py @@ -71,6 +71,7 @@ def finish(self): def wrap_handler(handler_class): """Wraps a BaseHTTPHandler with SSL MITM certificates.""" if certutils.openssl_import_error: + # pylint: disable=raising-bad-type raise certutils.openssl_import_error class WrappedHandler(SslHandshakeHandler, handler_class): diff --git a/test_runner.py b/test_runner.py index 4153e75..c8ca89f 100644 --- a/test_runner.py +++ b/test_runner.py @@ -2,7 +2,6 @@ # Copyright (c) 2014 The Chromium Authors. All rights reserved. # Use of this source code is governed by a BSD-style license that can be # found in the LICENSE file. -import inspect import unittest import sys import os diff --git a/third_party/__init__.py b/third_party/__init__.py index 4c25eda..f7ba211 100644 --- a/third_party/__init__.py +++ b/third_party/__init__.py @@ -22,4 +22,5 @@ third_party_dir = os.path.dirname(os.path.abspath(__file__)) ipaddr_dir = os.path.join(third_party_dir, "ipaddr") sys.path.append(ipaddr_dir) # workaround for no __init__.py +import ipaddr sys.path.append(third_party_dir) diff --git a/trafficshaper.py b/trafficshaper.py index 4642f9c..9af70da 100644 --- a/trafficshaper.py +++ b/trafficshaper.py @@ -28,7 +28,7 @@ class TrafficShaperException(Exception): class BandwidthValueError(TrafficShaperException): - def __init__(self, value): + def __init__(self, value): # pylint: disable=super-init-not-called self.value = value def __str__(self): @@ -98,7 +98,7 @@ def __enter__(self): if not ipfw_list.startswith('65535 '): logging.warn('ipfw has existing rules:\n%s', ipfw_list) self._delete_rules(ipfw_list) - except: + except Exception: pass if (self.up_bandwidth == '0' and self.down_bandwidth == '0' and self.delay_ms == '0' and self.packet_loss_rate == '0'): diff --git a/trafficshaper_test.py b/trafficshaper_test.py index f0461e1..9076ac6 100755 --- a/trafficshaper_test.py +++ b/trafficshaper_test.py @@ -21,14 +21,12 @@ import daemonserver import logging -import multiprocessing import platformsettings import socket import SocketServer import trafficshaper import unittest - RESPONSE_SIZE_KEY = 'response-size:' TEST_DNS_PORT = 5555 TEST_HTTP_PORT = 8888 @@ -134,7 +132,7 @@ def __exit__(self, *args): class TimedTestCase(unittest.TestCase): - def assertAlmostEqual(self, expected, actual, tolerance=0.05): + def assertValuesAlmostEqual(self, expected, actual, tolerance=0.05): """Like the following with nicer default message: assertTrue(expected <= actual + tolerance && expected >= actual - tolerance) @@ -197,7 +195,7 @@ def testTcpConnectToIp(self): start_time = self.timer() with self.tcp_socket_creator: connect_time = GetElapsedMs(start_time, self.timer()) - self.assertAlmostEqual(delay_ms, connect_time, tolerance=0.12) + self.assertValuesAlmostEqual(delay_ms, connect_time, tolerance=0.12) def testTcpUploadShaping(self): """Verify that 'up' bandwidth is shaped on TCP connections.""" @@ -209,7 +207,7 @@ def testTcpUploadShaping(self): expected_ms = 8.0 * num_bytes / bandwidth_kbits with TimedTcpServer(self.host, self.port): with self.TrafficShaper(up_bandwidth='%sKbit/s' % bandwidth_kbits): - self.assertAlmostEqual(expected_ms, self.GetTcpSendTimeMs(num_bytes)) + self.assertValuesAlmostEqual(expected_ms, self.GetTcpSendTimeMs(num_bytes)) def testTcpDownloadShaping(self): """Verify that 'down' bandwidth is shaped on TCP connections.""" @@ -221,7 +219,7 @@ def testTcpDownloadShaping(self): expected_ms = 8.0 * num_bytes / bandwidth_kbits with TimedTcpServer(self.host, self.port): with self.TrafficShaper(down_bandwidth='%sKbit/s' % bandwidth_kbits): - self.assertAlmostEqual(expected_ms, self.GetTcpReceiveTimeMs(num_bytes)) + self.assertValuesAlmostEqual(expected_ms, self.GetTcpReceiveTimeMs(num_bytes)) def testTcpInterleavedDownloads(self): # TODO(slamm): write tcp interleaved downloads test @@ -257,8 +255,8 @@ def testUdpDelay(self): with TimedUdpServer(self.host, self.dns_port): with self.TrafficShaper(delay_ms=delay_ms): send_ms, receive_ms = self.GetUdpSendReceiveTimesMs() - self.assertAlmostEqual(expected_ms, send_ms, tolerance=0.10) - self.assertAlmostEqual(expected_ms, receive_ms, tolerance=0.10) + self.assertValuesAlmostEqual(expected_ms, send_ms, tolerance=0.10) + self.assertValuesAlmostEqual(expected_ms, receive_ms, tolerance=0.10) def testUdpInterleavedDelay(self):