test_planner: Only consider builds with testable artifacts

Also, drop a gitiles_commit check that doesn't match reality about how
the gitiles_commit is supposed to be populated.

BUG=chromium:971753
TEST=local runs, unit tests

Change-Id: Ieac3ab10b3d5156b9305939b31bdc1c67cc8fac6
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/infra/test_planner/+/1648261
Reviewed-by: Dhanya Ganesh <dhanyaganesh@chromium.org>
Commit-Queue: Sean Abraham <seanabraham@chromium.org>
Tested-by: Sean Abraham <seanabraham@chromium.org>
diff --git a/deploy_cipd.json b/deploy_cipd.json
index 5189c98..4e2bef7 100644
--- a/deploy_cipd.json
+++ b/deploy_cipd.json
@@ -1,6 +1,6 @@
 {
   "result": {
     "package": "chromiumos/infra/test_planner",
-    "instance_id": "7ITHMgBTjjSrmha67MQYljB-w7bd5Vb1XDOXS1G5PEMC"
+    "instance_id": "rg7uZMjHiaUP2IrPUc5kyj_lH4XzP7fF7G4-AlEDImkC"
   }
 }
\ No newline at end of file
diff --git a/src/testplans/internal/generator/generator.go b/src/testplans/internal/generator/generator.go
index 22c949f..bda4563 100644
--- a/src/testplans/internal/generator/generator.go
+++ b/src/testplans/internal/generator/generator.go
@@ -6,7 +6,6 @@
 import (
 	"errors"
 	"fmt"
-	"github.com/golang/protobuf/proto"
 	"github.com/golang/protobuf/ptypes/wrappers"
 	"go.chromium.org/chromiumos/infra/proto/go/chromiumos"
 	"go.chromium.org/chromiumos/infra/proto/go/testplans"
@@ -59,8 +58,11 @@
 	for _, bb := range unfilteredBbBuilds {
 		bt := getBuildTarget(bb)
 		if len(bt) == 0 {
-			log.Printf("filtering out build without a build target:\n%s",
-				proto.MarshalTextString(bb.Builder))
+			log.Printf("filtering out build without a build target: %s", bb.GetBuilder().GetBuilder())
+		} else if isPointlessBuild(bb) {
+			log.Printf("filtering out because marked as pointless: %s", bb.GetBuilder().GetBuilder())
+		} else if !hasTestArtifacts(bb) {
+			log.Printf("filtering out with missing test artifacts: %s", bb.GetBuilder().GetBuilder())
 		} else {
 			btBuildReports[BuildTarget(bt)] = *bb
 			filteredBbBuilds = append(filteredBbBuilds, bb)
@@ -91,6 +93,39 @@
 	return createResponse(targetBuildResults, skippableTests)
 }
 
+func isPointlessBuild(bb *bbproto.Build) bool {
+	pointlessBuild, ok := bb.GetOutput().GetProperties().GetFields()["pointless_build"]
+	return ok && pointlessBuild.GetBoolValue()
+}
+
+func hasTestArtifacts(b *bbproto.Build) bool {
+	art, ok := b.GetOutput().GetProperties().GetFields()["artifacts"]
+	if !ok {
+		return false
+	}
+	fba, ok := art.GetStructValue().GetFields()["files_by_artifact"]
+	if !ok {
+		return false
+	}
+
+	// The presence of any one of these artifacts is enough to tell us that this
+	// build should be considered for testing.
+	testArtifacts := []string{
+		"AUTOTEST_FILES",
+		"IMAGE_ZIP",
+		"PINNED_GUEST_IMAGES",
+		"TAST_FILES",
+		"TEST_UPDATE_PAYLOAD",
+	}
+	fileToArtifact := fba.GetStructValue().GetFields()
+	for _, ta := range testArtifacts {
+		if _, ok := fileToArtifact[ta]; ok {
+			return true
+		}
+	}
+	return false
+}
+
 // getBuildTarget returns the build target from the given build, or empty string if none is found.
 func getBuildTarget(bb *bbproto.Build) string {
 	btStruct, ok := bb.Output.Properties.Fields["build_target"]
@@ -112,12 +147,6 @@
 	resp := &testplans.GenerateTestPlanResponse{}
 targetLoop:
 	for _, tbr := range targetBuildResults {
-		pointlessBuild, ok := tbr.buildReport.Output.Properties.Fields["pointless_build"]
-		if ok && pointlessBuild.GetBoolValue() {
-			// Build terminated early and successfully. No need to test it.
-			log.Printf("Skipping build %s because it's marked as pointless", tbr.buildTarget)
-			continue targetLoop
-		}
 		art, ok := tbr.buildReport.Output.Properties.Fields["artifacts"]
 		if !ok {
 			return nil, fmt.Errorf("found no artifacts output property for build_target %s", tbr.buildTarget)
@@ -278,16 +307,6 @@
 			log.Printf("No build found for BuildTarget %s", bt)
 			continue
 		}
-
-		// TODO: Handle early termination case. As of 2019-04-05, it's not defined yet how the builder
-		// will report that it terminated early.
-		// Below is what the logic might look like:
-		//
-		//if br.EarlyTerminationStatus != testplans.BuildReport_NOT_TERMINATED_EARLY &&
-		//	br.EarlyTerminationStatus != testplans.BuildReport_EARLY_TERMINATION_STATUS_UNSPECIFIED {
-		//	log.Printf("Disregarding %s because its EarlyTerminationStatus is %v", br.BuildTarget, br.EarlyTerminationStatus)
-		//	continue
-		//}
 		relevantReports[bt] = br
 	}
 	if len(relevantReports) == 0 {
@@ -313,11 +332,9 @@
 	changeRevs *git.ChangeRevData,
 	repoToSrcRoot map[string]string,
 	tt testType) (bool, error) {
-	if len(buildResult.Input.GerritChanges) == 0 && buildResult.Input.GitilesCommit != nil {
-		// In this case, the build is being performed at a particular commit, rather than for a range
-		// of unmerged Gerrit CLs. The disabling of testing in this method is only applicable in the
-		// Gerrit CLs case.
-		log.Printf("Found a Gitiles-based build for %s, so no tests will be skipped", getBuildTarget(buildResult))
+	if len(buildResult.Input.GerritChanges) == 0 {
+		// This happens during postsubmit runs, for example.
+		log.Printf("build doesn't contain gerrit_changes %s, so no tests will be skipped", getBuildTarget(buildResult))
 		return false, nil
 	}
 	fileCount := 0
diff --git a/src/testplans/internal/generator/generator_test.go b/src/testplans/internal/generator/generator_test.go
index bbd34e9..0b404ea 100644
--- a/src/testplans/internal/generator/generator_test.go
+++ b/src/testplans/internal/generator/generator_test.go
@@ -45,6 +45,11 @@
 							Fields: map[string]*_struct.Value{
 								"gs_bucket": {Kind: &_struct.Value_StringValue{StringValue: GS_BUCKET}},
 								"gs_path":   {Kind: &_struct.Value_StringValue{StringValue: GS_PATH_PREFIX + buildTarget}},
+								"files_by_artifact": {Kind: &_struct.Value_StructValue{StructValue: &_struct.Struct{
+									Fields: map[string]*_struct.Value{
+										"AUTOTEST_FILES": {Kind: &_struct.Value_ListValue{}},
+									},
+								}}},
 							},
 						}},
 					},
@@ -59,7 +64,52 @@
 	return b
 }
 
-func TestCreateCombinedTestPlan_success(t *testing.T) {
+func TestCreateCombinedTestPlan_oneUnitSuccess(t *testing.T) {
+	kevinHWTestCfg := &testplans.HwTestCfg{HwTest: []*testplans.HwTestCfg_HwTest{
+		{
+			Suite:       "HW kevin",
+			SkylabBoard: "kev",
+		},
+	}}
+	testReqs := &testplans.TargetTestRequirementsCfg{
+		PerTargetTestRequirements: []*testplans.PerTargetTestRequirements{
+			{TargetCriteria: &testplans.TargetCriteria{
+				TargetType: &testplans.TargetCriteria_BuildTarget{BuildTarget: "kevin"}},
+				HwTestCfg: kevinHWTestCfg},
+		},
+	}
+	sourceTreeTestCfg := &testplans.SourceTreeTestCfg{
+		SourceTreeTestRestriction: []*testplans.SourceTreeTestRestriction{
+			{SourceTree: &testplans.SourceTree{Path: "hw/tests/not/needed/here"},
+				TestRestriction: &testplans.TestRestriction{DisableHwTests: true}}}}
+	bbBuilds := []*bbproto.Build{
+		makeBuildbucketBuild("kevin", bbproto.Status_SUCCESS, []*bbproto.GerritChange{}, true),
+	}
+	chRevData := git.GetChangeRevsForTest([]*git.ChangeRev{})
+	repoToSrcRoot := map[string]string{"chromiumos/repo/name": "src/to/file"}
+
+	actualTestPlan, err := CreateTestPlan(testReqs, sourceTreeTestCfg, bbBuilds, chRevData, repoToSrcRoot)
+	if err != nil {
+		t.Error(err)
+	}
+
+	expectedTestPlan := &testplans.GenerateTestPlanResponse{
+		HwTestUnits: []*testplans.HwTestUnit{
+			{Common: &testplans.TestUnitCommon{
+				BuildPayload: &testplans.BuildPayload{
+					ArtifactsGsBucket: GS_BUCKET,
+					ArtifactsGsPath:   GS_PATH_PREFIX + "kevin",
+				},
+				BuildTarget: &chromiumos.BuildTarget{Name: "kevin"}},
+				HwTestCfg: kevinHWTestCfg},
+		},
+	}
+	if diff := cmp.Diff(expectedTestPlan, actualTestPlan, cmpopts.EquateEmpty()); diff != "" {
+		t.Errorf("CreateCombinedTestPlan bad result (-want/+got)\n%s", diff)
+	}
+}
+
+func TestCreateCombinedTestPlan_manyUnitSuccess(t *testing.T) {
 	reefGceTestCfg := &testplans.GceTestCfg{GceTest: []*testplans.GceTestCfg_GceTest{
 		{TestType: "GCE reef", Common: &testplans.TestSuiteCommon{Critical: &wrappers.BoolValue{Value: true}}},
 	}}
@@ -413,3 +463,43 @@
 		t.Errorf("CreateCombinedTestPlan bad result (-want/+got)\n%s", diff)
 	}
 }
+
+func TestCreateCombinedTestPlan_ignoresNonArtifactBuild(t *testing.T) {
+	kevinHWTestCfg := &testplans.HwTestCfg{HwTest: []*testplans.HwTestCfg_HwTest{
+		{
+			Suite:       "HW kevin",
+			SkylabBoard: "kev",
+		},
+	}}
+	testReqs := &testplans.TargetTestRequirementsCfg{
+		PerTargetTestRequirements: []*testplans.PerTargetTestRequirements{
+			{TargetCriteria: &testplans.TargetCriteria{
+				TargetType: &testplans.TargetCriteria_BuildTarget{BuildTarget: "kevin"}},
+				HwTestCfg: kevinHWTestCfg},
+		},
+	}
+	sourceTreeTestCfg := &testplans.SourceTreeTestCfg{
+		SourceTreeTestRestriction: []*testplans.SourceTreeTestRestriction{
+			{SourceTree: &testplans.SourceTree{Path: "hw/tests/not/needed/here"},
+				TestRestriction: &testplans.TestRestriction{DisableHwTests: true}}}}
+	build := makeBuildbucketBuild("kevin", bbproto.Status_SUCCESS, []*bbproto.GerritChange{}, true)
+
+	// Remove the AUTOTEST_FILES files_by_artifact key, thus making this whole
+	// build unusable for testing.
+	delete(
+		build.GetOutput().GetProperties().GetFields()["artifacts"].GetStructValue().GetFields()["files_by_artifact"].GetStructValue().GetFields(),
+		"AUTOTEST_FILES")
+	bbBuilds := []*bbproto.Build{build}
+	chRevData := git.GetChangeRevsForTest([]*git.ChangeRev{})
+	repoToSrcRoot := map[string]string{"chromiumos/repo/name": "src/to/file"}
+
+	actualTestPlan, err := CreateTestPlan(testReqs, sourceTreeTestCfg, bbBuilds, chRevData, repoToSrcRoot)
+	if err != nil {
+		t.Error(err)
+	}
+
+	expectedTestPlan := &testplans.GenerateTestPlanResponse{}
+	if diff := cmp.Diff(expectedTestPlan, actualTestPlan, cmpopts.EquateEmpty()); diff != "" {
+		t.Errorf("CreateCombinedTestPlan bad result (-want/+got)\n%s", diff)
+	}
+}