Fixing the PRESUBMIT include check some more.

The previous version didn't take into account that source files
can have a platform specific suffix.

BUG=NONE
TBR=ben@chromium.org

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

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

3 files changed, 194 insertions, 20 deletions

diff --git a/OWNERS b/OWNERS
index e9650c1..1ef6c18 100644
--- a/OWNERS
+++ b/OWNERS
@@ -6,5 +6,5 @@ per-file WATCHLISTS=*
 per-file Android.mk=benm@chromium.org
 per-file Android.mk=joth@chromium.org
 per-file Android.mk=torne@chromium.org
-per-file PRESUBMIT.py=maruel@chromium.org
-per-file PRESUBMIT.py=joi@chromium.org
+per-file PRESUBMIT*.py=maruel@chromium.org
+per-file PRESUBMIT*.py=joi@chromium.org
diff --git a/PRESUBMIT.py b/PRESUBMIT.py
index b3f7989..5c532d3 100644
--- a/PRESUBMIT.py
+++ b/PRESUBMIT.py
@@ -554,11 +554,11 @@ def _CheckIncludeOrderForScope(scope, input_api, file_path, changed_linenums):
   return warnings
 
 
-def _CheckIncludeOrderInFile(input_api, output_api, f, is_source,
-                             changed_linenums):
+def _CheckIncludeOrderInFile(input_api, f, is_source, changed_linenums):
   """Checks the #include order for the given file f."""
 
-  include_pattern = input_api.re.compile(r'\s*#include.*')
+  system_include_pattern = input_api.re.compile(r'\s*#include \<.*')
+  custom_include_pattern = input_api.re.compile(r'\s*#include "(?P<FILE>.*)"')
   if_pattern = input_api.re.compile(r'\s*#if.*')
   endif_pattern = input_api.re.compile(r'\s*#endif.*')
 
@@ -566,19 +566,26 @@ def _CheckIncludeOrderInFile(input_api, output_api, f, is_source,
   warnings = []
   line_num = 0
 
-  # Handle the special first include for source files.
+  # Handle the special first include for source files. If the header file is
+  # some/path/file.h, the corresponding source file can be some/path/file.cc,
+  # some/other/path/file.cc, some/path/file_platform.cc etc. It's also possible
+  # that no special first include exists.
   if is_source:
     for line in contents:
       line_num += 1
-      if include_pattern.match(line):
-        # The file name for the first include needs to be the same as for the
-        # file checked; the path can differ.
-        expected_ending = (
-            input_api.os_path.basename(f.LocalPath()).replace('.cc', '.h') +
-            '"')
-        if not line.endswith(expected_ending):
-          # Maybe there was no special first include, and that's fine. Process
-          # the line again along with the normal includes.
+      if system_include_pattern.match(line):
+        # No special first include -> process the line again along with normal
+        # includes.
+        line_num -= 1
+        break
+      match = custom_include_pattern.match(line)
+      if match:
+        match_dict = match.groupdict()
+        header_basename = input_api.os_path.basename(
+            match_dict['FILE']).replace('.h', '')
+        if header_basename not in input_api.os_path.basename(f.LocalPath()):
+          # No special first include -> process the line again along with normal
+          # includes.
           line_num -= 1
         break
 
@@ -590,7 +597,8 @@ def _CheckIncludeOrderInFile(input_api, output_api, f, is_source,
     if if_pattern.match(line) or endif_pattern.match(line):
       scopes.append(current_scope)
       current_scope = []
-    elif include_pattern.match(line):
+    elif (system_include_pattern.match(line) or
+          custom_include_pattern.match(line)):
       current_scope.append((line_num, line))
   scopes.append(current_scope)
 
@@ -615,11 +623,11 @@ def _CheckIncludeOrder(input_api, output_api):
   for f in input_api.AffectedFiles():
     changed_linenums = set([line_num for line_num, _ in f.ChangedContents()])
     if f.LocalPath().endswith('.cc'):
-      warnings = _CheckIncludeOrderInFile(input_api, output_api, f, True,
-                                          changed_linenums)
+      warnings.extend(_CheckIncludeOrderInFile(input_api, f, True,
+                                               changed_linenums))
     elif f.LocalPath().endswith('.h'):
-      warnings = _CheckIncludeOrderInFile(input_api, output_api, f, False,
-                                          changed_linenums)
+      warnings.extend(_CheckIncludeOrderInFile(input_api, f, False,
+                                               changed_linenums))
 
   results = []
   if warnings:
@@ -647,6 +655,12 @@ def _CommonChecks(input_api, output_api):
   results.extend(_CheckFilePermissions(input_api, output_api))
   results.extend(_CheckNoAuraWindowPropertyHInHeaders(input_api, output_api))
   results.extend(_CheckIncludeOrder(input_api, output_api))
+
+  if any('PRESUBMIT.py' == f.LocalPath() for f in input_api.AffectedFiles()):
+    results.extend(input_api.canned_checks.RunUnitTestsInDirectory(
+        input_api, output_api,
+        input_api.PresubmitLocalPath(),
+        whitelist=[r'.+_test\.py$']))
   return results
 
 
diff --git a/PRESUBMIT_test.py b/PRESUBMIT_test.py
new file mode 100755
index 0000000..1965d33
--- /dev/null
+++ b/PRESUBMIT_test.py
@@ -0,0 +1,160 @@
+#!/usr/bin/env python
+# Copyright (c) 2012 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 os
+import re
+import unittest
+
+import PRESUBMIT
+
+
+class MockInputApi(object):
+  def __init__(self):
+    self.re = re
+    self.os_path = os.path
+
+
+class MockFile(object):
+  def __init__(self, local_path, new_contents):
+    self._local_path = local_path
+    self._new_contents = new_contents
+
+  def NewContents(self):
+    return self._new_contents
+
+  def LocalPath(self):
+    return self._local_path
+
+
+class IncludeOrderTest(unittest.TestCase):
+  def testSystemHeaderOrder(self):
+    scope = [(1, '#include <csystem.h>'),
+             (2, '#include <cppsystem>'),
+             (3, '#include "acustom.h"')]
+    all_linenums = [linenum for (linenum, _) in scope]
+    mock_input_api = MockInputApi()
+    warnings = PRESUBMIT._CheckIncludeOrderForScope(scope, mock_input_api,
+                                                    '', all_linenums)
+    self.assertEqual(0, len(warnings))
+
+  def testSystemHeaderOrderMismatch1(self):
+    scope = [(10, '#include <cppsystem>'),
+             (20, '#include <csystem.h>'),
+             (30, '#include "acustom.h"')]
+    all_linenums = [linenum for (linenum, _) in scope]
+    mock_input_api = MockInputApi()
+    warnings = PRESUBMIT._CheckIncludeOrderForScope(scope, mock_input_api,
+                                                    '', all_linenums)
+    self.assertEqual(1, len(warnings))
+    self.assertTrue('20' in warnings[0])
+
+  def testSystemHeaderOrderMismatch2(self):
+    scope = [(10, '#include <cppsystem>'),
+             (20, '#include "acustom.h"'),
+             (30, '#include <csystem.h>')]
+    all_linenums = [linenum for (linenum, _) in scope]
+    mock_input_api = MockInputApi()
+    warnings = PRESUBMIT._CheckIncludeOrderForScope(scope, mock_input_api,
+                                                    '', all_linenums)
+    self.assertEqual(1, len(warnings))
+    self.assertTrue('30' in warnings[0])
+
+  def testSystemHeaderOrderMismatch3(self):
+    scope = [(10, '#include "acustom.h"'),
+             (20, '#include <csystem.h>'),
+             (30, '#include <cppsystem>')]
+    all_linenums = [linenum for (linenum, _) in scope]
+    mock_input_api = MockInputApi()
+    warnings = PRESUBMIT._CheckIncludeOrderForScope(scope, mock_input_api,
+                                                    '', all_linenums)
+    self.assertEqual(2, len(warnings))
+    self.assertTrue('20' in warnings[0])
+    self.assertTrue('30' in warnings[1])
+
+  def testAlphabeticalOrderMismatch(self):
+    scope = [(10, '#include <csystem.h>'),
+             (15, '#include <bsystem.h>'),
+             (20, '#include <cppsystem>'),
+             (25, '#include <bppsystem>'),
+             (30, '#include "bcustom.h"'),
+             (35, '#include "acustom.h"')]
+    all_linenums = [linenum for (linenum, _) in scope]
+    mock_input_api = MockInputApi()
+    warnings = PRESUBMIT._CheckIncludeOrderForScope(scope, mock_input_api,
+                                                    '', all_linenums)
+    self.assertEqual(3, len(warnings))
+    self.assertTrue('15' in warnings[0])
+    self.assertTrue('25' in warnings[1])
+    self.assertTrue('35' in warnings[2])
+
+  def testSpecialFirstInclude1(self):
+    mock_input_api = MockInputApi()
+    contents = ['#include "some/path/foo.h"',
+                '#include "a/header.h"']
+    mock_file = MockFile('some/path/foo.cc', contents)
+    warnings = PRESUBMIT._CheckIncludeOrderInFile(
+        mock_input_api, mock_file, True, range(1, len(contents) + 1))
+    self.assertEqual(0, len(warnings))
+
+  def testSpecialFirstInclude2(self):
+    mock_input_api = MockInputApi()
+    contents = ['#include "some/other/path/foo.h"',
+                '#include "a/header.h"']
+    mock_file = MockFile('some/path/foo.cc', contents)
+    warnings = PRESUBMIT._CheckIncludeOrderInFile(
+        mock_input_api, mock_file, True, range(1, len(contents) + 1))
+    self.assertEqual(0, len(warnings))
+
+  def testSpecialFirstInclude3(self):
+    mock_input_api = MockInputApi()
+    contents = ['#include "some/path/foo.h"',
+                '#include "a/header.h"']
+    mock_file = MockFile('some/path/foo_platform.cc', contents)
+    warnings = PRESUBMIT._CheckIncludeOrderInFile(
+        mock_input_api, mock_file, True, range(1, len(contents) + 1))
+    self.assertEqual(0, len(warnings))
+
+  def testSpecialFirstInclude4(self):
+    mock_input_api = MockInputApi()
+    contents = ['#include "some/path/bar.h"',
+                '#include "a/header.h"']
+    mock_file = MockFile('some/path/foo_platform.cc', contents)
+    warnings = PRESUBMIT._CheckIncludeOrderInFile(
+        mock_input_api, mock_file, True, range(1, len(contents) + 1))
+    self.assertEqual(1, len(warnings))
+    self.assertTrue('2' in warnings[0])
+
+  def testOrderAlreadyWrong(self):
+    scope = [(1, '#include "b.h"'),
+             (2, '#include "a.h"'),
+             (3, '#include "c.h"')]
+    mock_input_api = MockInputApi()
+    warnings = PRESUBMIT._CheckIncludeOrderForScope(scope, mock_input_api,
+                                                    '', [3])
+    self.assertEqual(0, len(warnings))
+
+  def testConflictAdded1(self):
+    scope = [(1, '#include "a.h"'),
+             (2, '#include "c.h"'),
+             (3, '#include "b.h"')]
+    mock_input_api = MockInputApi()
+    warnings = PRESUBMIT._CheckIncludeOrderForScope(scope, mock_input_api,
+                                                    '', [2])
+    self.assertEqual(1, len(warnings))
+    self.assertTrue('3' in warnings[0])
+
+  def testConflictAdded2(self):
+    scope = [(1, '#include "c.h"'),
+             (2, '#include "b.h"'),
+             (3, '#include "d.h"')]
+    mock_input_api = MockInputApi()
+    warnings = PRESUBMIT._CheckIncludeOrderForScope(scope, mock_input_api,
+                                                    '', [2])
+    self.assertEqual(1, len(warnings))
+    self.assertTrue('2' in warnings[0])
+
+
+if __name__ == '__main__':
+  unittest.main()