Mojo: Move mojom (python) unittests to mojom_tests package and combine errors.

Adds a base class for errors, so that the top-level doesn't have to know
about different kinds of errors. (Still to do: Factor out ProcessFile(),
so that it can throw appropriate errors too. This will make it properly
unit-test-able.)

There are two reasons to move the unit tests:
* Relative imports: It's very convenient to be able to do "python
  foo_unittests.py". I want to be able to use a relative import (i.e.,
  from parser.py, do "from ..error import Error"), but this only works
  if Python thinks I'm in a package (which it won't, if
  parser_unittests.py lives right beside parser.py, and is just doing
  "import parser").
* "Hygiene": Probably, if one were foolish enough to install the mojom
  package in a "global" location, one wouldn't want to install the
  tests. (Moreover, one probably doesn't typically want to import unit
  tests.)
Note that Python unittests (using the unittest package) have to be
importable, so it's natural that they live under pylib.

Also fix/suppress various pylint warnings. (Note: generate/ still has
some nontrivial pylint warnings, which need more attention.)

R=darin@chromium.org

Review URL: https://codereview.chromium.org/247933004

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@265959 0039d316-1c4b-4281-b951-d872f2087c98

14 files changed, 178 insertions, 123 deletions

diff --git a/mojo/public/tools/bindings/mojom_bindings_generator.py b/mojo/public/tools/bindings/mojom_bindings_generator.py
index 643f468..c8bab9c 100755
--- a/mojo/public/tools/bindings/mojom_bindings_generator.py
+++ b/mojo/public/tools/bindings/mojom_bindings_generator.py
@@ -12,12 +12,15 @@ import os
 import pprint
 import sys
 
+# Disable lint check for finding modules:
+# pylint: disable=F0401
+
 script_dir = os.path.dirname(os.path.abspath(__file__))
 sys.path.insert(0, os.path.join(script_dir, "pylib"))
 
+from mojom.error import Error
 from mojom.generate.data import OrderedModuleFromData
-from mojom.parse.lexer import LexError
-from mojom.parse.parser import Parse, ParseError
+from mojom.parse.parser import Parse
 from mojom.parse.translate import Translate
 
 
@@ -54,16 +57,19 @@ def MakeImportStackMessage(imported_filename_stack):
                     zip(imported_filename_stack[1:], imported_filename_stack)]))
 
 
-def ProcessFile(args, generator_modules, filename, processed_files={},
-                imported_filename_stack=[]):
+# Disable Check for dangerous default arguments (they're "private" keyword
+# arguments):
+# pylint: disable=W0102
+def ProcessFile(args, generator_modules, filename, _processed_files={},
+                _imported_filename_stack=[]):
   # Memoized results.
-  if filename in processed_files:
-    return processed_files[filename]
+  if filename in _processed_files:
+    return _processed_files[filename]
 
   # Ensure we only visit each file once.
-  if filename in imported_filename_stack:
+  if filename in _imported_filename_stack:
     print "%s: Error: Circular dependency" % filename + \
-        MakeImportStackMessage(imported_filename_stack + [filename])
+        MakeImportStackMessage(_imported_filename_stack + [filename])
     sys.exit(1)
 
   try:
@@ -71,13 +77,13 @@ def ProcessFile(args, generator_modules, filename, processed_files={},
       source = f.read()
   except IOError as e:
     print "%s: Error: %s" % (e.filename, e.strerror) + \
-        MakeImportStackMessage(imported_filename_stack + [filename])
+        MakeImportStackMessage(_imported_filename_stack + [filename])
     sys.exit(1)
 
   try:
     tree = Parse(source, filename)
-  except (LexError, ParseError) as e:
-    print str(e) + MakeImportStackMessage(imported_filename_stack + [filename])
+  except Error as e:
+    print str(e) + MakeImportStackMessage(_imported_filename_stack + [filename])
     sys.exit(1)
 
   dirname, name = os.path.split(filename)
@@ -91,8 +97,8 @@ def ProcessFile(args, generator_modules, filename, processed_files={},
     import_filename = os.path.join(dirname, import_data['filename'])
     import_data['module'] = ProcessFile(
         args, generator_modules, import_filename,
-        processed_files=processed_files,
-        imported_filename_stack=imported_filename_stack + [filename])
+        _processed_files=_processed_files,
+        _imported_filename_stack=_imported_filename_stack + [filename])
 
   module = OrderedModuleFromData(mojom)
 
@@ -108,8 +114,9 @@ def ProcessFile(args, generator_modules, filename, processed_files={},
     generator.GenerateFiles()
 
   # Save result.
-  processed_files[filename] = module
+  _processed_files[filename] = module
   return module
+# pylint: enable=W0102
 
 
 def Main():
diff --git a/mojo/public/tools/bindings/pylib/mojom/error.py b/mojo/public/tools/bindings/pylib/mojom/error.py
new file mode 100644
index 0000000..99522b9
--- /dev/null
+++ b/mojo/public/tools/bindings/pylib/mojom/error.py
@@ -0,0 +1,27 @@
+# Copyright 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.
+
+class Error(Exception):
+  """Base class for Mojo IDL bindings parser/generator errors."""
+
+  def __init__(self, filename, message, lineno=None, addenda=None, **kwargs):
+    """|filename| is the (primary) file which caused the error, |message| is the
+    error message, |lineno| is the 1-based line number (or |None| if not
+    applicable/available), and |addenda| is a list of additional lines to append
+    to the final error message."""
+    Exception.__init__(self, **kwargs)
+    self.filename = filename
+    self.message = message
+    self.lineno = lineno
+    self.addenda = addenda
+
+  def __str__(self):
+    if self.lineno:
+      s = "%s:%d: Error: %s" % (self.filename, self.lineno, self.message)
+    else:
+      s = "%s: Error: %s" % (self.filename, self.message)
+    return "\n".join([s] + self.addenda) if self.addenda else s
+
+  def __repr__(self):
+    return str(self)
diff --git a/mojo/public/tools/bindings/pylib/mojom/generate/generator.py b/mojo/public/tools/bindings/pylib/mojom/generate/generator.py
index 1984204..eba26683 100644
--- a/mojo/public/tools/bindings/pylib/mojom/generate/generator.py
+++ b/mojo/public/tools/bindings/pylib/mojom/generate/generator.py
@@ -5,8 +5,7 @@
 """Code shared by the various language-specific code generators."""
 
 from functools import partial
-import os
-import re
+import os.path
 
 import module as mojom
 import pack
diff --git a/mojo/public/tools/bindings/pylib/mojom/generate/module.py b/mojo/public/tools/bindings/pylib/mojom/generate/module.py
index d24d36f..9287777 100644
--- a/mojo/public/tools/bindings/pylib/mojom/generate/module.py
+++ b/mojo/public/tools/bindings/pylib/mojom/generate/module.py
@@ -149,7 +149,7 @@ class Interface(Kind):
   def AddMethod(self, name, ordinal = None):
     method = Method(name, ordinal)
     self.methods.append(method)
-    return method;
+    return method
 
 
 class EnumField(object):
@@ -179,11 +179,11 @@ class Module(object):
     self.interfaces = []
 
   def AddInterface(self, name):
-    interface = Interface(name);
+    interface = Interface(name)
     self.interfaces.append(interface)
-    return interface;
+    return interface
 
   def AddStruct(self, name):
     struct = Struct(name)
     self.structs.append(struct)
-    return struct;
+    return struct
diff --git a/mojo/public/tools/bindings/pylib/mojom/generate/pack.py b/mojo/public/tools/bindings/pylib/mojom/generate/pack.py
index 84378d7..52caa0f 100644
--- a/mojo/public/tools/bindings/pylib/mojom/generate/pack.py
+++ b/mojo/public/tools/bindings/pylib/mojom/generate/pack.py
@@ -116,11 +116,11 @@ class PackedStruct(object):
 
   def GetTotalSize(self):
     if not self.packed_fields:
-      return 0;
+      return 0
     last_field = self.packed_fields[-1]
     offset = last_field.offset + last_field.size
     pad = GetPad(offset, 8)
-    return offset + pad;
+    return offset + pad
 
 
 class ByteInfo(object):
diff --git a/mojo/public/tools/bindings/pylib/mojom/generate/template_expander.py b/mojo/public/tools/bindings/pylib/mojom/generate/template_expander.py
index 00c388e..0e7332b 100644
--- a/mojo/public/tools/bindings/pylib/mojom/generate/template_expander.py
+++ b/mojo/public/tools/bindings/pylib/mojom/generate/template_expander.py
@@ -5,20 +5,28 @@
 # Based on:
 # http://src.chromium.org/viewvc/blink/trunk/Source/build/scripts/template_expander.py
 
+import imp
 import inspect
-import os
+import os.path
 import sys
 
-# jinja2 is in the third_party directory.
-# Insert at front to override system libraries, and after path[0] == script dir.
-path = os.path.abspath(__file__)
-while True:
-  path, tail = os.path.split(path)
-  assert tail
-  if tail == "mojo":
-    break
-sys.path.insert(1, os.path.join(path, "third_party"))
-del path, tail
+# Disable lint check for finding modules:
+# pylint: disable=F0401
+
+def _GetDirAbove(dirname):
+  """Returns the directory "above" this file containing |dirname| (which must
+  also be "above" this file)."""
+  path = os.path.abspath(__file__)
+  while True:
+    path, tail = os.path.split(path)
+    assert tail
+    if tail == dirname:
+      return path
+
+try:
+  imp.find_module("jinja2")
+except ImportError:
+  sys.path.append(os.path.join(_GetDirAbove("mojo"), "third_party"))
 import jinja2
 
 
diff --git a/mojo/public/tools/bindings/pylib/mojom/generate/test_support.py b/mojo/public/tools/bindings/pylib/mojom/generate/test_support.py
index 93c8739..eb39461 100644
--- a/mojo/public/tools/bindings/pylib/mojom/generate/test_support.py
+++ b/mojo/public/tools/bindings/pylib/mojom/generate/test_support.py
@@ -179,7 +179,6 @@ def EXPECT_TRUE(a):
 
 def RunTest(fn):
   sys.stdout.write('Running %s...' % fn.__name__)
-  success = True;
   try:
     errors = fn()
   except:
diff --git a/mojo/public/tools/bindings/pylib/mojom/parse/lexer.py b/mojo/public/tools/bindings/pylib/mojom/parse/lexer.py
index 10abedd..49c6f5e 100644
--- a/mojo/public/tools/bindings/pylib/mojom/parse/lexer.py
+++ b/mojo/public/tools/bindings/pylib/mojom/parse/lexer.py
@@ -2,44 +2,43 @@
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
+import imp
 import os.path
-import re
 import sys
 
-# Try to load the ply module, if not, then assume it is in the third_party
-# directory.
-try:
-  # Disable lint check which fails to find the ply module.
-  # pylint: disable=F0401
-  from ply.lex import TOKEN
-except ImportError:
-  # Work our way up to the directory containing mojo/ (usually src/). (Note:
-  # Some builds don't check out into a directory called src/.)
+# Disable lint check for finding modules:
+# pylint: disable=F0401
+
+def _GetDirAbove(dirname):
+  """Returns the directory "above" this file containing |dirname| (which must
+  also be "above" this file)."""
   path = os.path.abspath(__file__)
   while True:
     path, tail = os.path.split(path)
     assert tail
-    if tail == "mojo":
-      break
-  sys.path.append(os.path.join(path, "third_party"))
-  del path, tail
-  # pylint: disable=F0401
-  from ply.lex import TOKEN
+    if tail == dirname:
+      return path
 
+try:
+  imp.find_module("ply")
+except ImportError:
+  sys.path.append(os.path.join(_GetDirAbove("mojo"), "third_party"))
+from ply.lex import TOKEN
+
+from ..error import Error
 
-class LexError(Exception):
-  def __init__(self, filename, lineno, msg):
-    self.filename = filename
-    self.lineno = lineno
-    self.msg = msg
 
-  def __str__(self):
-    return "%s:%d: Error: %s" % (self.filename, self.lineno, self.msg)
+# Disable lint check for exceptions deriving from Exception:
+# pylint: disable=W0710
+class LexError(Error):
+  """Class for errors from the lexer."""
 
-  def __repr__(self):
-    return str(self)
+  def __init__(self, filename, message, lineno):
+    Error.__init__(self, filename, message, lineno=lineno)
 
 
+# We have methods which look like they could be functions:
+# pylint: disable=R0201
 class Lexer(object):
 
   def __init__(self, filename):
@@ -51,7 +50,7 @@ class Lexer(object):
   ## Internal auxiliary methods
   ##
   def _error(self, msg, token):
-    raise LexError(self.filename, token.lineno, msg)
+    raise LexError(self.filename, msg, token.lineno)
 
   ##
   ## Reserved keywords
diff --git a/mojo/public/tools/bindings/pylib/mojom/parse/parser.py b/mojo/public/tools/bindings/pylib/mojom/parse/parser.py
index 33af7d9..e75b28a 100755
--- a/mojo/public/tools/bindings/pylib/mojom/parse/parser.py
+++ b/mojo/public/tools/bindings/pylib/mojom/parse/parser.py
@@ -5,32 +5,31 @@
 
 """Generates a syntax tree from a Mojo IDL file."""
 
-
+import imp
 import os.path
 import sys
 
-# Try to load the ply module, if not, then assume it is in the third_party
-# directory.
-try:
-  # Disable lint check which fails to find the ply module.
-  # pylint: disable=F0401
-  from ply import lex
-  from ply import yacc
-except ImportError:
-  # Work our way up to the directory containing mojo/ (usually src/). (Note:
-  # Some builds don't check out into a directory called src/.)
+# Disable lint check for finding modules:
+# pylint: disable=F0401
+
+def _GetDirAbove(dirname):
+  """Returns the directory "above" this file containing |dirname| (which must
+  also be "above" this file)."""
   path = os.path.abspath(__file__)
   while True:
     path, tail = os.path.split(path)
     assert tail
-    if tail == "mojo":
-      break
-  sys.path.append(os.path.join(path, "third_party"))
-  del path, tail
-  # pylint: disable=F0401
-  from ply import lex
-  from ply import yacc
+    if tail == dirname:
+      return path
 
+try:
+  imp.find_module("ply")
+except ImportError:
+  sys.path.append(os.path.join(_GetDirAbove("mojo"), "third_party"))
+from ply import lex
+from ply import yacc
+
+from ..error import Error
 import ast
 from lexer import Lexer
 
@@ -52,25 +51,18 @@ def _ListFromConcat(*items):
   return itemsout
 
 
-class ParseError(Exception):
+# Disable lint check for exceptions deriving from Exception:
+# pylint: disable=W0710
+class ParseError(Error):
+  """Class for errors from the parser."""
 
   def __init__(self, filename, message, lineno=None, snippet=None):
-    self.filename = filename
-    self.message = message
-    self.lineno = lineno
-    self.snippet = snippet
-
-  def __str__(self):
-    if self.lineno is None:
-      return "%s: Error: %s" % (self.filename, self.message)
-
-    s = "%s:%d: Error: %s" % (self.filename, self.lineno, self.message)
-    return s if self.snippet is None else s + "\n" + self.snippet
-
-  def __repr__(self):
-    return str(self)
+    Error.__init__(self, filename, message, lineno=lineno,
+                   addenda=([snippet] if snippet else None))
 
 
+# We have methods which look like they could be functions:
+# pylint: disable=R0201
 class Parser(object):
 
   def __init__(self, lexer, source, filename):
diff --git a/mojo/public/tools/bindings/pylib/mojom_tests/__init__.py b/mojo/public/tools/bindings/pylib/mojom_tests/__init__.py
new file mode 100644
index 0000000..e69de29
--- /dev/null
+++ b/mojo/public/tools/bindings/pylib/mojom_tests/__init__.py
diff --git a/mojo/public/tools/bindings/pylib/mojom_tests/parse/__init__.py b/mojo/public/tools/bindings/pylib/mojom_tests/parse/__init__.py
new file mode 100644
index 0000000..e69de29
--- /dev/null
+++ b/mojo/public/tools/bindings/pylib/mojom_tests/parse/__init__.py
diff --git a/mojo/public/tools/bindings/pylib/mojom/parse/lexer_unittest.py b/mojo/public/tools/bindings/pylib/mojom_tests/parse/lexer_unittest.py
index a3b0222..a450c14 100644
--- a/mojo/public/tools/bindings/pylib/mojom/parse/lexer_unittest.py
+++ b/mojo/public/tools/bindings/pylib/mojom_tests/parse/lexer_unittest.py
@@ -2,31 +2,35 @@
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
+import imp
 import os.path
 import sys
 import unittest
 
-# Try to load the ply module, if not, then assume it is in the third_party
-# directory.
-try:
-  # Disable lint check which fails to find the ply module.
-  # pylint: disable=F0401
-  from ply import lex
-except ImportError:
-  # Work our way up to the directory containing mojo/ (usually src/). (Note:
-  # Some builds don't check out into a directory called src/.)
+# Disable lint check for finding modules:
+# pylint: disable=F0401
+
+def _GetDirAbove(dirname):
+  """Returns the directory "above" this file containing |dirname| (which must
+  also be "above" this file)."""
   path = os.path.abspath(__file__)
   while True:
     path, tail = os.path.split(path)
     assert tail
-    if tail == "mojo":
-      break
-  sys.path.append(os.path.join(path, "third_party"))
-  del path, tail
-  # pylint: disable=F0401
-  from ply import lex
+    if tail == dirname:
+      return path
+
+try:
+  imp.find_module("ply")
+except ImportError:
+  sys.path.append(os.path.join(_GetDirAbove("mojo"), "third_party"))
+from ply import lex
 
-import lexer
+try:
+  imp.find_module("mojom")
+except ImportError:
+  sys.path.append(os.path.join(_GetDirAbove("pylib"), "pylib"))
+import mojom.parse.lexer
 
 
 # This (monkey-patching LexToken to make comparison value-based) is evil, but
@@ -38,11 +42,11 @@ def _LexTokenEq(self, other):
 setattr(lex.LexToken, '__eq__', _LexTokenEq)
 
 
-def _MakeLexToken(type, value, lineno=1, lexpos=0):
+def _MakeLexToken(token_type, value, lineno=1, lexpos=0):
   """Makes a LexToken with the given parameters. (Note that lineno is 1-based,
   but lexpos is 0-based.)"""
   rv = lex.LexToken()
-  rv.type, rv.value, rv.lineno, rv.lexpos = type, value, lineno, lexpos
+  rv.type, rv.value, rv.lineno, rv.lexpos = token_type, value, lineno, lexpos
   return rv
 
 
@@ -52,12 +56,12 @@ def _MakeLexTokenForKeyword(keyword, **kwargs):
 
 
 class LexerTest(unittest.TestCase):
-  """Tests |lexer.Lexer|."""
+  """Tests |mojom.parse.lexer.Lexer|."""
 
   def __init__(self, *args, **kwargs):
     unittest.TestCase.__init__(self, *args, **kwargs)
     # Clone all lexer instances from this one, since making a lexer is slow.
-    self._zygote_lexer = lex.lex(lexer.Lexer("my_file.mojom"))
+    self._zygote_lexer = lex.lex(mojom.parse.lexer.Lexer("my_file.mojom"))
 
   def testValidSingleKeywords(self):
     """Tests valid, single keywords."""
@@ -145,10 +149,10 @@ class LexerTest(unittest.TestCase):
     self.assertEquals(self._SingleTokenForInput("."),
                       _MakeLexToken("DOT", "."))
 
-  def _TokensForInput(self, input):
+  def _TokensForInput(self, input_string):
     """Gets a list of tokens for the given input string."""
     lexer = self._zygote_lexer.clone()
-    lexer.input(input)
+    lexer.input(input_string)
     rv = []
     while True:
       tok = lexer.token()
@@ -156,10 +160,10 @@ class LexerTest(unittest.TestCase):
         return rv
       rv.append(tok)
 
-  def _SingleTokenForInput(self, input):
+  def _SingleTokenForInput(self, input_string):
     """Gets the single token for the given input string. (Raises an exception if
     the input string does not result in exactly one token.)"""
-    toks = self._TokensForInput(input)
+    toks = self._TokensForInput(input_string)
     assert len(toks) == 1
     return toks[0]
 
diff --git a/mojo/public/tools/bindings/pylib/mojom/parse/parser_unittest.py b/mojo/public/tools/bindings/pylib/mojom_tests/parse/parser_unittest.py
index fdce26c..94b83f9 100644
--- a/mojo/public/tools/bindings/pylib/mojom/parse/parser_unittest.py
+++ b/mojo/public/tools/bindings/pylib/mojom_tests/parse/parser_unittest.py
@@ -2,11 +2,31 @@
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
+import imp
+import os.path
+import sys
 import unittest
 
-import ast
-import lexer
-import parser
+# Disable lint check for finding modules:
+# pylint: disable=F0401
+
+def _GetDirAbove(dirname):
+  """Returns the directory "above" this file containing |dirname| (which must
+  also be "above" this file)."""
+  path = os.path.abspath(__file__)
+  while True:
+    path, tail = os.path.split(path)
+    assert tail
+    if tail == dirname:
+      return path
+
+try:
+  imp.find_module("mojom")
+except ImportError:
+  sys.path.append(os.path.join(_GetDirAbove("pylib"), "pylib"))
+import mojom.parse.ast as ast
+import mojom.parse.lexer as lexer
+import mojom.parse.parser as parser
 
 
 class ParserTest(unittest.TestCase):
@@ -25,7 +45,7 @@ module my_module {
 
   def testSourceWithCrLfs(self):
     """Tests a .mojom source with CR-LFs instead of LFs."""
-    source = "// This is a comment.\r\n\r\nmodule my_module {\r\n}\r\n";
+    source = "// This is a comment.\r\n\r\nmodule my_module {\r\n}\r\n"
     self.assertEquals(parser.Parse(source, "my_file.mojom"),
                       [("MODULE", "my_module", None)])
 
diff --git a/mojo/tools/run_mojo_python_tests.py b/mojo/tools/run_mojo_python_tests.py
index 48b723c..2bee0ab 100755
--- a/mojo/tools/run_mojo_python_tests.py
+++ b/mojo/tools/run_mojo_python_tests.py
@@ -1,4 +1,4 @@
-#!/bin/python
+#!/usr/bin/env python
 # Copyright 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.
@@ -28,7 +28,7 @@ def main():
   loader = unittest.loader.TestLoader()
   print "Running Python unit tests under mojo/public/tools/bindings/pylib ..."
   suite = loader.discover(os.path.join(chromium_src_dir, 'mojo', 'public',
-                                      'tools', 'bindings', 'pylib'),
+                                       'tools', 'bindings', 'pylib'),
                           pattern='*_unittest.py')
 
   runner = unittest.runner.TextTestRunner(verbosity=(options.verbose+1))