Fix exlucde_target_regexp breakage in test-changed and --files option…

… breakage in changed with diffspec

This review addresses 2 issues:
1. Currently, on v1 engine, exclude_target_regexp does not have effect in test-changed (and also compile-changed since they have same base class).
2. In "changed" task, if both --diffspec and --files options are given, no output is printed.

This review fixes the above 2 issues and adds test coverage. I also add "ensure_engine" decorator to some tests in changed_integration.py which are currently only tested on v1 engine.

Testing Done:
https://travis-ci.org/pantsbuild/pants/builds/168473439

Bugs closed: 3977, 3981, 3982

Reviewed at https://rbcommons.com/s/twitter/r/4321/

src/python/pants/scm/change_calculator.py

@@ -28,6 +28,7 @@ def __init__(self, scm, workspace=None, changes_since=None, diffspec=None):
 
   def changed_files(self, changes_since=None, diffspec=None):
     """Determines the files changed according to SCM/workspace and options."""
+    diffspec = diffspec or self._diffspec
     if diffspec:
       return self._workspace.changes_in(diffspec)
 

src/python/pants/task/changed_target_task.py

@@ -49,7 +49,8 @@ def register_options(cls, register):
   def alternate_target_roots(cls, options, address_mapper, build_graph):
     changed = Changed.Factory.global_instance().create(options)
     change_calculator = changed.change_calculator(build_graph=build_graph,
-                                                  address_mapper=address_mapper)
+                                                  address_mapper=address_mapper,
+                                                  exclude_target_regexp=options.exclude_target_regexp)
     changed_addresses = change_calculator.changed_target_addresses()
     readable = ''.join(sorted('\n\t* {}'.format(addr.reference()) for addr in changed_addresses))
     logger.info('Operating on changed {} target(s): {}'.format(len(changed_addresses), readable))

tests/python/pants_test/engine/legacy/test_changed_integration.py

@@ -7,14 +7,15 @@
 
 import os
 import shutil
+import subprocess
 from contextlib import contextmanager
 from textwrap import dedent
 
 from pants.base.build_environment import get_buildroot
 from pants.util.contextutil import environment_as, temporary_dir
 from pants.util.dirutil import safe_mkdir, safe_open, touch
 from pants_test.base_test import TestGenerator
-from pants_test.pants_run_integration_test import PantsRunIntegrationTest
+from pants_test.pants_run_integration_test import PantsRunIntegrationTest, ensure_engine
 from pants_test.testutils.git_util import initialize_repo
 
 
@@ -267,6 +268,7 @@ def assert_changed_new_equals_old(self, extra_args, success=True, test_list=Fals
     # If we get to here without asserting, we know all copies of stdout are identical - return one.
     return changed_run.stdout_data
 
+  @ensure_engine
   def test_changed_options_scope_shadowing(self):
     """Tests that the `test-changed` scope overrides `changed` scope."""
     changed_src = 'src/python/python_targets/test_library.py'
@@ -294,6 +296,7 @@ def test_changed_options_scope_shadowing(self):
           continue  # Ignore subset matches.
         self.assertNotIn(not_expected_item, pants_run.stdout_data)
 
+  @ensure_engine
   def test_changed_options_scope_positional(self):
     changed_src = 'src/python/python_targets/test_library.py'
     expected_set = set(self.TEST_MAPPING[changed_src]['transitive'])
@@ -311,6 +314,33 @@ def test_changed_options_scope_positional(self):
       for expected_item in expected_set:
         self.assertIn(expected_item, pants_run.stdout_data)
 
+  @ensure_engine
+  def test_test_changed_exclude_target(self):
+    changed_src = 'src/python/python_targets/test_library.py'
+    exclude_target_regexp = r'_[0-9]'
+    excluded_set = {'src/python/python_targets:test_library_transitive_dependee_2',
+                    'src/python/python_targets:test_library_transitive_dependee_3',
+                    'src/python/python_targets:test_library_transitive_dependee_4'}
+    expected_set = set(self.TEST_MAPPING[changed_src]['transitive']) - excluded_set
+
+    with create_isolated_git_repo() as worktree:
+      with mutated_working_copy([os.path.join(worktree, changed_src)]):
+        pants_run = self.run_pants([
+          '-ldebug',   # This ensures the changed target names show up in the pants output.
+          '--exclude-target-regexp={}'.format(exclude_target_regexp),
+          'test-changed',
+          '--changes-since=HEAD',
+          '--include-dependees=transitive'
+        ])
+
+      self.assert_success(pants_run)
+      for expected_item in expected_set:
+        self.assertIn(expected_item, pants_run.stdout_data)
+
+      for excluded_item in excluded_set:
+        self.assertNotIn(excluded_item, pants_run.stdout_data)
+
+  @ensure_engine
   def test_changed_changed_since_and_files(self):
     with create_isolated_git_repo():
       stdout = self.assert_changed_new_equals_old(['--changed-since=HEAD^^', '--files'])
@@ -324,6 +354,21 @@ def test_changed_changed_since_and_files(self):
          '3rdparty/BUILD'}
       )
 
+  @ensure_engine
+  def test_changed_diffspec_and_files(self):
+    with create_isolated_git_repo():
+      git_sha = subprocess.check_output(['git', 'rev-parse', 'HEAD^^']).strip()
+      stdout = self.assert_changed_new_equals_old(['--changed-diffspec={}'.format(git_sha), '--files'])
+
+      # The output should be the files added in the last 2 commits.
+      self.assertEqual(
+        lines_to_set(stdout),
+        {'src/python/python_targets/BUILD',
+         'src/python/python_targets/test_binary.py',
+         'src/python/python_targets/test_library.py',
+         'src/python/python_targets/test_unclaimed_src.py'}
+      )
+
   # Following 4 tests do not run in isolated repo because they don't mutate working copy.
   def test_changed(self):
     self.assert_changed_new_equals_old([])