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

blimp/PRESUBMIT.py

Index: blimp/PRESUBMIT.py
diff --git a/blimp/PRESUBMIT.py b/blimp/PRESUBMIT.py
index ae90d459bc91a181eb154ae1f6652b91c5eb80a5..c8c4224d1bce3b2b8113f5aa173f65dbf2012892 100644
--- a/blimp/PRESUBMIT.py
+++ b/blimp/PRESUBMIT.py
@@ -19,7 +19,33 @@ def CheckChangeLintsClean(input_api, output_api):
   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