Blimp presubmit to check for unit tests on new .cc files.

blimp/PRESUBMIT.py

 # 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.
 
 """Top-level presubmit script for blimp.
 
 See http://dev.chromium.org/developers/how-tos/depottools/presubmit-scripts
 for more details about the presubmit API built into depot_tools.
 """
 
 import re
 
 BLIMP_SOURCE_FILES=(r'^blimp[\\/].*\.(cc|h)$',)
 
 def CheckChangeLintsClean(input_api, output_api):
   source_filter = lambda x: input_api.FilterSourceFile(
     x, white_list=BLIMP_SOURCE_FILES, black_list=None)
 
   return input_api.canned_checks.CheckChangeLintsClean(
       input_api, output_api, source_filter, lint_filters=[], verbose_level=1)
 
+def CheckNewFilesHaveTests(input_api, output_api):
+  unittest_files = set()

[Kevin M, 2016/05/23 23:57:39]

Does this check files that live outside of blimp/ ?

[Wez, 2016/05/24 00:44:54]

path/PRESUBMIT.py only gets called for files that live under path/, I think?

[Brian Goldman, 2016/06/01 22:56:57]

I tested this. It only checks files under blimp.

+  files_needing_unittest = set()

[Wez, 2016/05/24 00:44:54]

nit: How about new_unittest_files and new_source_files for consistency?

[Brian Goldman, 2016/06/01 22:56:57]

I'm going to leave the names as-is even though I don't particularly like them.
source_file is a name used elsewhere in this function to mean any
new/modified/deleted file in the changelist, so I think using it to mean
specifically a .cc file that needs a unittest.cc file would be confusing.

+

[Kevin M, 2016/05/23 23:57:39]

Also need something analogous for Java tests, which have a different directory
structure and file naming convention.

[Wez, 2016/05/24 00:44:54]

Feels like a follow-up CL?

[Brian Goldman, 2016/06/01 22:56:57]

Yes :)

+  for source_file in input_api.AffectedFiles():
+    if source_file.Action() == 'A':
+      name = source_file.LocalPath()
+      if re.search(r'_unittest\.cc$', name):

[Kevin M, 2016/05/23 23:57:39]

Can we be consistent with search vs match?

[Kevin M, 2016/05/23 23:57:39]

Should we also exclude anything with "test" in the name? Otherwise a
foo_browser_test.cc will need a foo_browser_test_unittest.cc . Turtles all the
way down~

[Wez, 2016/05/24 00:44:54]

And use re.compile() to compile the REs once each, rather than once-per-file.

[Wez, 2016/05/24 00:44:54]

This is only generating a warning, though, so it won't stop CQ processing the
CL; question is how many *test*.cc files are complex enough test classes that
they need dedicated tests themselves. ;)  Presumably not very many..?

[Brian Goldman, 2016/06/01 22:56:57]

I ended up getting rid of regex for this section entirely and just startswith
and endswith.

[Brian Goldman, 2016/06/01 22:56:57]

I'm going with the assumption that if it ends in "test.cc" then it's a test, and
skipping those from the check, since we will have browser tests (although I'm
not sure if they will always live with the things they are testing?) Other
things that support testing might have "test" in the name, but not likely at the
end, if I understand the convention for this repo.

[Brian Goldman, 2016/06/01 22:56:57]

No more regexes in this section. I do use one for substitution a little further
down, but I'm going to go ahead and say regex compilation is no big deal here
due to very very small N, and inlining the pattern makes it easier to read.

+        unittest_files.add(name)
+      elif re.match(r'^blimp[\\/].*\.cc$', name):
+        files_needing_unittest.add(name)
+
+  missing = []

[Wez, 2016/05/24 00:44:54]

nit: missing_unittest_files?

+
+  for name in files_needing_unittest:
+    unittest_name = re.sub(r'\.cc$', '_unittest.cc', name)
+    if unittest_name not in unittest_files:
+      missing.append(name)
+
+  if missing:
+    message = 'The following new files are missing unit tests:'
+    return [output_api.PresubmitPromptWarning(message, missing)]
+  else:
+    return []
+
 def CheckChangeOnUpload(input_api, output_api):
   results = []
   results += CheckChangeLintsClean(input_api, output_api)
+  results += CheckNewFilesHaveTests(input_api, output_api)
   return results