Handle unused recipe expectations in new 'test' command

recipe_engine/test.py

 # Copyright 2017 The LUCI Authors. All rights reserved.
 # Use of this source code is governed under the Apache License, Version 2.0
 # that can be found in the LICENSE file.
 
 from __future__ import print_function
 
 import argparse
 import cStringIO
 import contextlib
 import copy
 import coverage
 import datetime
 import difflib
 import functools
 import json
 import multiprocessing
 import os
 import pprint
 import re
+import shutil
 import signal
 import sys
 import tempfile
 import traceback
 
 from . import checker
 from . import config_types
 from . import loader
 from . import run
 from . import step_runner
@@ skipping 66 matching lines @@
   def __init__(self, check):
     self.check = check
 
   def format(self):
     return self.check.format(indent=4)
 
 
 class TestResult(object):
   """Result of running a test."""
 
-  def __init__(self, test_description, failures, coverage_data):
+  def __init__(self, test_description, failures, coverage_data,
+               recipe_run_result):
     self.test_description = test_description
     self.failures = failures
     self.coverage_data = coverage_data
+    self.recipe_run_result = recipe_run_result
 
 
 class TestDescription(object):
   """Identifies a specific test.
 
   Deliberately small and picklable for use with multiprocessing."""
 
   def __init__(self, recipe_name, test_name, expect_dir, covers):
     self.recipe_name = recipe_name
     self.test_name = test_name
     self.expect_dir = expect_dir
     self.covers = covers
 
   @property
   def full_name(self):
     return '%s.%s' % (self.recipe_name, self.test_name)
 
+  @property
+  def expectation_path(self):
+    return os.path.join(self.expect_dir, self.test_name + '.json')
+
 
 def run_test(test_description, train=False):
   """Runs a test. Returns TestResults object."""
   expected = None
-  expectation_path = os.path.join(
-      test_description.expect_dir, test_description.test_name + '.json')
-  if os.path.exists(expectation_path):
-    with open(expectation_path) as f:
+  if os.path.exists(test_description.expectation_path):
+    with open(test_description.expectation_path) as f:
       # TODO(phajdan.jr): why do we need to re-encode golden data files?
       expected = re_encode(json.load(f))
 
   actual, failed_checks, coverage_data = run_recipe(
       test_description.recipe_name, test_description.test_name,
       test_description.covers)
   actual = re_encode(actual)
 
   failures = []
 
   # TODO(phajdan.jr): handle exception (errors) in the recipe execution.
   if failed_checks:
     sys.stdout.write('C')
     failures.extend([CheckFailure(c) for c in failed_checks])
   elif actual != expected:
     if train:
-      expectation_dir = os.path.dirname(expectation_path)
+      expectation_dir = os.path.dirname(test_description.expectation_path)
       if not os.path.exists(expectation_dir):
         os.makedirs(expectation_dir)
-      with open(expectation_path, 'w') as f:
+      with open(test_description.expectation_path, 'w') as f:

[iannucci, 2017/03/17 22:04:19]

I think you may want to open 'wb' to prevent windows line endings

[Paweł Hajdan Jr., 2017/03/20 11:29:14]

Done.

         json.dump(
             re_encode(actual), f, sort_keys=True, indent=2,
             separators=(',', ': '))
       sys.stdout.write('D')
     else:
       diff = '\n'.join(difflib.unified_diff(
           pprint.pformat(expected).splitlines(),
           pprint.pformat(actual).splitlines(),
           fromfile='expected', tofile='actual',
           n=4, lineterm=''))
 
       failures.append(DiffFailure(diff))
       sys.stdout.write('F')
   else:
     sys.stdout.write('.')
   sys.stdout.flush()
 
-  return TestResult(test_description, failures, coverage_data)
+  return TestResult(test_description, failures, coverage_data, actual)

[iannucci, 2017/03/17 22:04:19]

'actual' is probably large and will be slow to pickle; is there a subset of it
you can move over the multiprocess ipc?

edit: it looks like you're using it as a boolean?

[Paweł Hajdan Jr., 2017/03/20 11:29:14]

This didn't seem to have measurable effect on the performance (testing with
build repo). Done anyway.

FWIW, scanning for unused expectations does incur some hit, so if we are going
to be nitpicky about that we may want to put that behind a flag (enabled on the
bots of course).

 
 
 def run_recipe(recipe_name, test_name, covers):
   """Runs the recipe under test in simulation mode.
 
   Returns a tuple:
     - expectation data
     - failed post-process checks (if any)
     - coverage data
   """
@@ skipping 194 matching lines @@
       return (False, traceback.format_exc())
   return wrapper
 
 
 @worker
 def run_worker(test, train=False):
   """Worker for 'run' command (note decorator above)."""
   return run_test(test, train=train)
 
 
+def scan_for_expectations(root):
+  """Returns set of expectation paths recursively under |root|."""
+  collected_expectations = set()
+  for entry in os.listdir(root):
+    full_entry = os.path.join(root, entry)
+    if os.path.isdir(full_entry):
+      collected_expectations.update(scan_for_expectations(full_entry))
+    if entry.endswith('.expected'):
+      collected_expectations.add(full_entry)
+      if os.path.isdir(full_entry):
+        for subentry in os.listdir(full_entry):
+          full_subentry = os.path.join(full_entry, subentry)

[iannucci, 2017/03/17 22:04:19]

only want subentry if it ends with .json. Otherwise you might pick up OWNERS
files, README.md, etc.

[Paweł Hajdan Jr., 2017/03/20 11:29:14]

Good point, done. Also added tests.

+          collected_expectations.add(full_subentry)
+  return collected_expectations
+
+
 def run_run(train, jobs):
   """Implementation of the 'run' command."""
   start_time = datetime.datetime.now()
 
   report_coverage_version()
 
   tests, coverage_data, uncovered_modules = get_tests()
   if uncovered_modules:
     raise Exception('The following modules lack test coverage: %s' % (
         ','.join(sorted(uncovered_modules))))
 
   with kill_switch():
     pool = multiprocessing.Pool(jobs)
     results = pool.map(functools.partial(run_worker, train=train), tests)
 
   print()
 
+  used_expectations = set()
+
   rc = 0
   for success, details in results:
     if success:
       assert isinstance(details, TestResult)
       if details.failures:
         rc = 1
         print('%s failed:' % details.test_description.full_name)
         for failure in details.failures:
           print(failure.format())
       coverage_data.update(details.coverage_data)
+      if details.recipe_run_result is not None:
+        used_expectations.add(details.test_description.expectation_path)
+        used_expectations.add(
+            os.path.dirname(details.test_description.expectation_path))
     else:
       rc = 1
       print('Internal failure:')
       print(details)
 
   try:
     # TODO(phajdan.jr): Add API to coverage to load data from memory.
     with tempfile.NamedTemporaryFile(delete=False) as coverage_file:
       coverage_data.write_file(coverage_file.name)
 
     cov = coverage.coverage(
         data_file=coverage_file.name, config_file=False, omit=cover_omit())
     cov.load()
     outf = cStringIO.StringIO()
     percentage = cov.report(file=outf, show_missing=True, skip_covered=True)
     if int(percentage) != 100:
       rc = 1
       print(outf.getvalue())
       print('FATAL: Insufficient coverage (%.f%%)' % int(percentage))
   finally:
     os.unlink(coverage_file.name)
 
+  actual_expectations = set()
+  if os.path.exists(_UNIVERSE_VIEW.recipe_dir):
+    actual_expectations.update(scan_for_expectations(_UNIVERSE_VIEW.recipe_dir))
+  if os.path.exists(_UNIVERSE_VIEW.module_dir):
+    for module_entry in os.listdir(_UNIVERSE_VIEW.module_dir):
+      if os.path.isdir(module_entry):
+        actual_expectations.update(scan_for_expectations(
+            os.path.join(_UNIVERSE_VIEW.module_dir, module_entry)))
+  unused_expectations = actual_expectations.difference(used_expectations)
+  if unused_expectations:
+    if train:
+      for entry in unused_expectations:
+        if not os.path.exists(entry):
+          continue
+        if os.path.isdir(entry):
+          shutil.rmtree(entry)
+        else:
+          os.unlink(entry)
+    else:
+      rc = 1
+      print('FATAL: unused expectations found:')
+      print('\n'.join(sorted(unused_expectations)))
+
   finish_time = datetime.datetime.now()
   print('-' * 70)
   print('Ran %d tests in %0.3fs' % (
       len(tests), (finish_time - start_time).total_seconds()))
   print()
   print('OK' if rc == 0 else 'FAILED')
 
   return rc
 
 
@@ skipping 90 matching lines @@
   Returns:
     Exit code
   """
   global _UNIVERSE_VIEW
   _UNIVERSE_VIEW = universe_view
   global _ENGINE_FLAGS
   _ENGINE_FLAGS = engine_flags
 
   args = parse_args(raw_args)
   return args.func(args)