Only test targets that are affected by change

Summary:
This diff implements a part of our selective testing picture. If `selective_testing_policy` is set to `enabled` for a build, only affected targets for that build will be tested. The other targets will be created, but marked as unaffected `ResultSource.from_parent`).

Note that this diff does not enable selective testing - the variable `selective_testing_policy` on the build object is never set and defaults to `disabled`.

This diff also does not do any result propagation / revision result creation.

Test Plan: unit tests

Reviewers: anupc

Reviewed By: anupc

Subscribers: changesbot, kylec, wwu

Differential Revision: https://tails.corp.dropbox.com/D229863

changes/api/command_details.py

@@ -141,7 +141,8 @@ def expand_command(self, command, expander, data):
         _, buildstep = JobPlan.get_build_step_for_job(jobstep.job_id)
 
         results = []
-        for future_jobstep in expander.expand(max_executors=jobstep.data['max_executors'],
+        for future_jobstep in expander.expand(job=jobstep.job,
+                                              max_executors=jobstep.data['max_executors'],
                                               test_stats_from=buildstep.get_test_stats_from()):
             new_jobstep = buildstep.create_expanded_jobstep(jobstep, new_jobphase, future_jobstep)
             results.append(new_jobstep)

changes/artifacts/bazel_target.py

@@ -2,7 +2,7 @@
 
 from changes.artifacts.xunit import XunitHandler
 from changes.config import db
-from changes.constants import Status
+from changes.constants import ResultSource, Status
 from changes.db.utils import get_or_create
 from changes.models.bazeltarget import BazelTarget
 from changes.models.testresult import TestResultManager
@@ -19,6 +19,7 @@ def process(self, fp, artifact):
             'step_id': self.step.id,
             'job_id': self.step.job.id,
             'name': target_name,
+            'result_source': ResultSource.from_self,
         })
         test_suites = self.get_test_suites(fp)
         tests = self.aggregate_tests_from_suites(test_suites)

changes/buildsteps/default.py

@@ -16,7 +16,7 @@
 from changes.artifacts.xunit import XunitHandler
 from changes.buildsteps.base import BuildStep, LXCConfig
 from changes.config import db, statsreporter
-from changes.constants import Cause, Result, Status, DEFAULT_CPUS, DEFAULT_MEMORY_MB
+from changes.constants import Cause, Result, ResultSource, Status, DEFAULT_CPUS, DEFAULT_MEMORY_MB
 from changes.db.utils import get_or_create
 from changes.jobs.sync_job_step import sync_job_step
 from changes.models.bazeltarget import BazelTarget
@@ -498,6 +498,7 @@ def create_expanded_jobstep(self, base_jobstep, new_jobphase, future_jobstep, sk
                     name=target_name,
                     status=Status.in_progress,
                     result=Result.unknown,
+                    result_source=ResultSource.from_self,
                 )
                 db.session.add(target)
 

changes/expanders/base.py

@@ -17,7 +17,7 @@ def validate(self):
         """
         raise NotImplementedError
 
-    def expand(self, max_executors, **kwargs):
+    def expand(self, job, max_executors, **kwargs):
         """
         Yield up to `max_executors` expanded jobsteps to be run, based on the
         collection phase's data.

changes/expanders/bazel_targets.py

@@ -4,6 +4,7 @@
 
 from changes.api.client import api_client
 from changes.config import db, statsreporter
+from changes.constants import ResultSource, SelectiveTestingPolicy
 from changes.expanders.base import Expander
 from changes.models.bazeltarget import BazelTarget
 from changes.models.command import FutureCommand
@@ -39,7 +40,7 @@ def validate(self):
         assert '{target_names}' in self.data[
             'cmd'], 'Missing ``{target_names}`` in command'
 
-    def expand(self, max_executors, test_stats_from=None):
+    def expand(self, job, max_executors, test_stats_from=None):
         target_stats, avg_time = self.get_target_stats(
             test_stats_from or self.project.slug)
 
@@ -48,7 +49,22 @@ def expand(self, max_executors, test_stats_from=None):
         all_targets = affected_targets + unaffected_targets
         statsreporter.stats().set_gauge('{}_bazel_affected_targets_count'.format(self.project.slug), len(affected_targets))
         statsreporter.stats().set_gauge('{}_bazel_all_targets_count'.format(self.project.slug), len(all_targets))
-        groups = shard(all_targets, max_executors,
+        to_shard = all_targets
+
+        # NOTE: null values for selective testing policy implies `disabled`
+        if job.build.selective_testing_policy is SelectiveTestingPolicy.enabled:
+            to_shard = affected_targets
+            for target in unaffected_targets:
+                # TODO(naphat) should we check if the target exists in the parent revision?
+                # it should be impossible for it not to exist by our collect-targets script
+                target_object = BazelTarget(
+                    job=job,
+                    name=target,
+                    result_source=ResultSource.from_parent,
+                )
+                db.session.add(target_object)
+
+        groups = shard(to_shard, max_executors,
                        target_stats, avg_time)
 
         for weight, target_list in groups:

changes/expanders/commands.py

@@ -21,7 +21,7 @@ class CommandsExpander(Expander):
     def validate(self):
         assert 'commands' in self.data, 'Missing ``commands`` attribute'
 
-    def expand(self, max_executors, **kwargs):
+    def expand(self, job, max_executors, **kwargs):
         for cmd_data in self.data['commands']:
             # TODO: group commands with jobsteps so as to respect max_executors
             future_command = FutureCommand(**cmd_data)

changes/expanders/tests.py

@@ -33,7 +33,7 @@ def validate(self):
         assert 'cmd' in self.data, 'Missing ``cmd`` attribute'
         assert '{test_names}' in self.data['cmd'], 'Missing ``{test_names}`` in command'
 
-    def expand(self, max_executors, test_stats_from=None):
+    def expand(self, job, max_executors, test_stats_from=None):
         test_stats, avg_test_time = self.get_test_stats(test_stats_from or self.project.slug)
 
         groups = shard(

changes/jobs/sync_job_step.py

@@ -8,11 +8,11 @@
 
 from flask import current_app
 from requests.exceptions import ConnectionError, HTTPError, Timeout, SSLError
-from sqlalchemy import distinct
+from sqlalchemy import distinct, or_
 from sqlalchemy.exc import IntegrityError
 from sqlalchemy.sql import func
 
-from changes.constants import Status, Result
+from changes.constants import Status, Result, ResultSource
 from changes.config import db, statsreporter
 from changes.db.utils import try_create
 from changes.jobs.sync_artifact import sync_artifact
@@ -138,6 +138,12 @@ def has_missing_targets(step):
     return db.session.query(BazelTarget.query.filter(
         BazelTarget.step_id == step.id,
         BazelTarget.status == Status.in_progress,
+        or_(
+            BazelTarget.result_source == ResultSource.from_self,
+
+            # None value for result_source implies `from_self`
+            BazelTarget.result_source.is_(None),
+        ),
     ).exists()).scalar()
 
 

tests/changes/api/test_command_details.py

@@ -140,7 +140,8 @@ def dummy_create_expanded_jobstep(jobstep, new_jobphase, future_jobstep):
             data={'foo': 'bar'},
         )
         dummy_expander.validate.assert_called_once_with()
-        dummy_expander.expand.assert_called_once_with(max_executors=10,
+        dummy_expander.expand.assert_called_once_with(job=job,
+                                                      max_executors=10,
                                                       test_stats_from=mock_buildstep.get_test_stats_from.return_value)
 
         phase2 = JobPhase.query.filter(
@@ -192,7 +193,8 @@ def test_expander_no_commands(self, mock_get_expander, mock_get_build_step_for_j
             data={'foo': 'bar'},
         )
         empty_expander.validate.assert_called_once_with()
-        empty_expander.expand.assert_called_once_with(max_executors=10,
+        empty_expander.expand.assert_called_once_with(job=job,
+                                                      max_executors=10,
                                                       test_stats_from=mock_buildstep.get_test_stats_from.return_value)
 
         phase2 = JobPhase.query.filter(

tests/changes/buildsteps/test_default.py

@@ -10,7 +10,7 @@
     DefaultBuildStep
 )
 from changes.config import db
-from changes.constants import Result, Status, Cause
+from changes.constants import Result, ResultSource, Status, Cause
 from changes.models.command import CommandType, FutureCommand
 from changes.models.jobstep import FutureJobStep
 from changes.models.repository import Repository
@@ -525,6 +525,7 @@ def test_create_expanded_jobstep_with_targets(self, get_vcs):
             assert target.job == new_jobstep.job
             assert target.status is Status.in_progress
             assert target.result is Result.unknown
+            assert target.result_source is ResultSource.from_self
 
     @mock.patch.object(Repository, 'get_vcs')
     def test_create_replacement_jobstep_expanded(self, get_vcs):

tests/changes/expanders/test_bazel_targets.py

@@ -5,8 +5,9 @@
 from mock import patch
 
 from changes.config import db
-from changes.constants import Status, Result
+from changes.constants import Status, Result, ResultSource, SelectiveTestingPolicy
 from changes.expanders.bazel_targets import BazelTargetsExpander
+from changes.models.bazeltarget import BazelTarget
 from changes.testutils import TestCase
 
 
@@ -75,6 +76,9 @@ def test_get_target_stats(self):
 
     @patch.object(BazelTargetsExpander, 'get_target_stats')
     def test_expand(self, mock_get_target_stats):
+        project = self.create_project()
+        build = self.create_build(project)
+        job = self.create_job(build)
         mock_get_target_stats.return_value = {
             '//foo/bar:test': 50,
             '//foo/baz:target': 15,
@@ -93,7 +97,7 @@ def test_expand(self, mock_get_target_stats):
                 '//foo/bar/test_buz:test',
             ],
             'artifact_search_path': 'artifacts/'
-        }).expand(max_executors=2))
+        }).expand(job=job, max_executors=2))
 
         results.sort(key=lambda x: x.data['weight'], reverse=True)
 
@@ -117,8 +121,107 @@ def test_expand(self, mock_get_target_stats):
         assert results[1].commands[0].label == results[1].label
         assert results[1].commands[0].script == results[1].label
 
+    @patch.object(BazelTargetsExpander, 'get_target_stats')
+    def test_expand_with_selective_testing(self, mock_get_target_stats):
+        project = self.create_project()
+        build = self.create_build(project, selective_testing_policy=SelectiveTestingPolicy.enabled)
+        job = self.create_job(build)
+        mock_get_target_stats.return_value = {
+            '//foo/bar:test': 50,
+            '//foo/baz:target': 15,
+            '//foo/bar/test_biz:test': 10,
+            '//foo/bar/test_buz:test': 200,
+        }, 68
+
+        results = list(self.get_expander({
+            'cmd': 'bazel test {target_names}',
+            'affected_targets': [
+                '//foo/bar:test',
+                '//foo/baz:target',
+            ],
+            'unaffected_targets': [
+                '//foo/bar/test_biz:test',
+                '//foo/bar/test_buz:test',
+            ],
+            'artifact_search_path': 'artifacts/'
+        }).expand(job=job, max_executors=2))
+
+        results.sort(key=lambda x: x.data['weight'], reverse=True)
+
+        assert len(results) == 2
+        assert results[0].label == 'bazel test //foo/bar:test'
+        assert results[0].data['weight'] == 51
+        assert set(results[0].data['targets']) == set(
+            ['//foo/bar:test'])
+        assert results[0].data['shard_count'] == 2
+        assert results[0].data['artifact_search_path'] == 'artifacts/'
+        assert results[0].commands[0].label == results[0].label
+        assert results[0].commands[0].script == results[0].label
+
+        assert results[
+            1].label == 'bazel test //foo/baz:target'
+        assert results[1].data['weight'] == 16
+        assert set(results[1].data['targets']) == set(
+            ['//foo/baz:target'])
+        assert results[1].data['shard_count'] == 2
+        assert results[1].data['artifact_search_path'] == 'artifacts/'
+        assert results[1].commands[0].label == results[1].label
+        assert results[1].commands[0].script == results[1].label
+
+        db.session.commit()
+        for unaffected in ['//foo/bar/test_biz:test', '//foo/bar/test_buz:test']:
+            assert BazelTarget.query.filter(
+                BazelTarget.name == unaffected,
+                BazelTarget.job == job,
+                BazelTarget.result == Result.unknown,
+                BazelTarget.status == Status.unknown,
+                BazelTarget.result_source == ResultSource.from_parent,
+            ).count() == 1
+
+    @patch.object(BazelTargetsExpander, 'get_target_stats')
+    def test_expand_with_selective_testing_None(self, mock_get_target_stats):
+        # test that selective_testing_policy = None implies disabled
+        project = self.create_project()
+        build = self.create_build(project)
+        build.selective_testing_policy = None
+        db.session.add(build)
+        db.session.commit()
+        assert build.selective_testing_policy is None
+        job = self.create_job(build)
+        mock_get_target_stats.return_value = {
+            '//foo/bar:test': 50,
+            '//foo/baz:target': 15,
+            '//foo/bar/test_biz:test': 10,
+            '//foo/bar/test_buz:test': 200,
+        }, 68
+
+        results = list(self.get_expander({
+            'cmd': 'bazel test {target_names}',
+            'affected_targets': [
+                '//foo/bar:test',
+                '//foo/baz:target',
+            ],
+            'unaffected_targets': [
+                '//foo/bar/test_biz:test',
+                '//foo/bar/test_buz:test',
+            ],
+            'artifact_search_path': 'artifacts/'
+        }).expand(job=job, max_executors=2))
+
+        results.sort(key=lambda x: x.data['weight'], reverse=True)
+
+        assert len(results) == 2
+        assert set(results[0].data['targets']) == set(
+            ['//foo/bar/test_buz:test'])
+
+        assert set(results[1].data['targets']) == set(
+            ['//foo/bar:test', '//foo/baz:target', '//foo/bar/test_biz:test'])
+
     @patch.object(BazelTargetsExpander, 'get_target_stats')
     def test_expand_no_duration(self, mock_get_target_stats):
+        project = self.create_project()
+        build = self.create_build(project)
+        job = self.create_job(build)
         mock_get_target_stats.return_value = {
             '//foo/bar:test': 50,
             '//foo/baz:target': 15,
@@ -141,7 +244,7 @@ def test_expand_no_duration(self, mock_get_target_stats):
                 '//foo/bar/test_buz:test',
             ],
             'artifact_search_path': 'artifacts/'
-        }).expand(max_executors=2))
+        }).expand(job=job, max_executors=2))
 
         results.sort(key=lambda x: x.data['weight'], reverse=True)
 

tests/changes/expanders/test_commands.py

@@ -21,10 +21,13 @@ def test_validate(self):
         self.get_expander({'commands': []}).validate()
 
     def test_expand(self):
+        project = self.create_project()
+        build = self.create_build(project)
+        job = self.create_job(build)
         results = list(self.get_expander({'commands': [
             {'script': 'echo 1'},
             {'script': 'echo 2', 'label': 'foo'}
-        ]}).expand(max_executors=10))
+        ]}).expand(job=job, max_executors=10))
 
         assert len(results) == 2
         assert results[0].label == 'echo 1'

tests/changes/expanders/test_tests.py

@@ -61,6 +61,9 @@ def test_get_test_stats(self):
 
     @patch.object(TestsExpander, 'get_test_stats')
     def test_expand(self, mock_get_test_stats):
+        project = self.create_project()
+        build = self.create_build(project)
+        job = self.create_job(build)
         mock_get_test_stats.return_value = {
             ('foo', 'bar'): 50,
             ('foo', 'baz'): 15,
@@ -76,7 +79,7 @@ def test_expand(self, mock_get_test_stats):
                 'foo.bar.test_biz',
                 'foo.bar.test_buz',
             ],
-        }).expand(max_executors=2))
+        }).expand(job=job, max_executors=2))
 
         results.sort(key=lambda x: x.data['weight'], reverse=True)
 
@@ -99,6 +102,9 @@ def test_expand(self, mock_get_test_stats):
 
     @patch.object(TestsExpander, 'get_test_stats')
     def test_expand_with_artifact_search_path(self, mock_get_test_stats):
+        project = self.create_project()
+        build = self.create_build(project)
+        job = self.create_job(build)
         mock_get_test_stats.return_value = {
             ('foo', 'bar'): 50,
             ('foo', 'baz'): 15,
@@ -115,7 +121,7 @@ def test_expand_with_artifact_search_path(self, mock_get_test_stats):
                 'foo.bar.test_buz',
             ],
             'artifact_search_path': '/tmp/artifacts',
-        }).expand(max_executors=2))
+        }).expand(job=job, max_executors=2))
 
         results.sort(key=lambda x: x.data['weight'], reverse=True)
 
@@ -138,6 +144,9 @@ def test_expand_with_artifact_search_path(self, mock_get_test_stats):
 
     @patch.object(TestsExpander, 'get_test_stats')
     def test_expand_with_artifact_search_path_empty(self, mock_get_test_stats):
+        project = self.create_project()
+        build = self.create_build(project)
+        job = self.create_job(build)
         mock_get_test_stats.return_value = {
             ('foo', 'bar'): 50,
             ('foo', 'baz'): 15,
@@ -154,7 +163,7 @@ def test_expand_with_artifact_search_path_empty(self, mock_get_test_stats):
                 'foo.bar.test_buz',
             ],
             'artifact_search_path': '',
-        }).expand(max_executors=2))
+        }).expand(job=job, max_executors=2))
 
         results.sort(key=lambda x: x.data['weight'], reverse=True)