Merge pull request #29291 from mpherman2/revert_gob_bug

Revert "Feat: skip presubmit tests for revisions without code change"

go.mod

@@ -29,7 +29,7 @@ require (
 	github.com/Azure/go-autorest/autorest/adal v0.9.20
 	github.com/GoogleCloudPlatform/testgrid v0.0.123
 	github.com/NYTimes/gziphandler v1.1.1
-	github.com/andygrunwald/go-gerrit v0.0.0-20230211083816-04e01d7217b2
+	github.com/andygrunwald/go-gerrit v0.0.0-20210709065208-9d38b0be0268
 	github.com/andygrunwald/go-jira v1.14.0
 	github.com/aws/aws-sdk-go v1.38.49
 	github.com/bazelbuild/buildtools v0.0.0-20200922170545-10384511ce98

go.sum

@@ -160,8 +160,8 @@ github.com/alecthomas/units v0.0.0-20190717042225-c3de453c63f4/go.mod h1:ybxpYRF
 github.com/alecthomas/units v0.0.0-20190924025748-f65c72e2690d/go.mod h1:rBZYJk541a8SKzHPHnH3zbiI+7dagKZ0cgpgrD7Fyho=
 github.com/alvaroaleman/httpcache v0.0.0-20210618195546-ab9a1a3f8a38 h1:ML+zGuJv9er8cg0N8D8edCNOwqI/Kb3WoV6VsIV93SU=
 github.com/alvaroaleman/httpcache v0.0.0-20210618195546-ab9a1a3f8a38/go.mod h1:R4sf/p+3TZ6dvs7BxjxjozS7lIpzKtuS4jmQt4egJfU=
-github.com/andygrunwald/go-gerrit v0.0.0-20230211083816-04e01d7217b2 h1:ohDU8MHAAE/FMlqs+KVlzLpVx3/gJ86rNO+8TZ9nea8=
-github.com/andygrunwald/go-gerrit v0.0.0-20230211083816-04e01d7217b2/go.mod h1:SeP12EkHZxEVjuJ2HZET304NBtHGG2X6w2Gzd0QXAZw=
+github.com/andygrunwald/go-gerrit v0.0.0-20210709065208-9d38b0be0268 h1:7gokoTWteZhP1t2f0OzrFFXlyL8o0+b0r4ZaRV9PXOs=
+github.com/andygrunwald/go-gerrit v0.0.0-20210709065208-9d38b0be0268/go.mod h1:aqcjwEnmLLSalFNYR0p2ttnEXOVVRctIzsUMHbEcruU=
 github.com/andygrunwald/go-jira v1.14.0 h1:7GT/3qhar2dGJ0kq8w0d63liNyHOnxZsUZ9Pe4+AKBI=
 github.com/andygrunwald/go-jira v1.14.0/go.mod h1:KMo2f4DgMZA1C9FdImuLc04x4WQhn5derQpnsuBFgqE=
 github.com/anmitsu/go-shlex v0.0.0-20161002113705-648efa622239 h1:kFOfPq6dUM1hTo4JG6LR5AXSUEsOjtdm0kw0FtQtMJA=

prow/gerrit/adapter/adapter.go

@@ -21,7 +21,6 @@ import (
 	"context"
 	"fmt"
 	"net/url"
-	"sort"
 	"strconv"
 	"strings"
 	"sync"
@@ -595,25 +594,23 @@ func (c *Controller) processChange(logger logrus.FieldLogger, instance string, c
 			logger.WithField("lastUpdate", lastUpdate).Warnf("lastUpdate not found, falling back to now")
 		}
 
-		currentRevision := change.Revisions[change.CurrentRevision]
-		failedJobs := failedJobs(account.AccountID, currentRevision.Number, change.Messages...)
+		revision := change.Revisions[change.CurrentRevision]
+		failedJobs := failedJobs(account.AccountID, revision.Number, change.Messages...)
 		failed, all := presubmitContexts(failedJobs, presubmits, logger)
 		messages := currentMessages(change, lastUpdate)
 		logger.WithField("failed", len(failed)).Debug("Failed jobs parsed from previous comments.")
 		filters := []pjutil.Filter{
 			messageFilter(messages, failed, all, triggerTimes, logger),
 		}
-		// Automatically trigger the Prow jobs if the change has a revision with code change
-		// since last update and the change is not in WorkInProgress.
-		lastRevisionWithChange := lastRevisionWithCodeChange(change, lastUpdate)
-		if lastRevisionWithChange != nil && !change.WorkInProgress {
+		// Automatically trigger the Prow jobs if the revision is new and the
+		// change is not in WorkInProgress.
+		if revision.Created.Time.After(lastUpdate) && !change.WorkInProgress {
 			filters = append(filters, &timeAnnotationFilter{
 				Filter:       pjutil.NewTestAllFilter(),
-				eventTime:    lastRevisionWithChange.Created.Time,
+				eventTime:    revision.Created.Time,
 				triggerTimes: triggerTimes,
 			})
 		}
-
 		toTrigger, err := pjutil.FilterPresubmits(pjutil.NewAggregateFilter(filters), client.ChangedFilesProvider(&change), change.Branch, presubmits, logger)
 		if err != nil {
 			return fmt.Errorf("filter presubmits: %w", err)
@@ -704,30 +701,6 @@ func (c *Controller) processChange(logger logrus.FieldLogger, instance string, c
 	return nil
 }
 
-// lastRevisionWithCodeChange tries to find a revision with code change since last update.
-// There is no point repeating tests if no code change happened since last update.
-func lastRevisionWithCodeChange(change client.ChangeInfo, lastUpdate time.Time) *client.RevisionInfo {
-	var revisionsWithCodeChange []client.RevisionInfo
-	for _, revision := range change.Revisions {
-		if revision.Kind == gerrit.NoCodeChange {
-			continue
-		}
-		revisionsWithCodeChange = append(revisionsWithCodeChange, revision)
-	}
-	// sorts in reverse chronological order
-	sort.Slice(revisionsWithCodeChange, func(i, j int) bool {
-		return revisionsWithCodeChange[i].Created.Time.After(revisionsWithCodeChange[j].Created.Time)
-	})
-
-	for _, revision := range revisionsWithCodeChange {
-		if revision.Created.Time.After(lastUpdate) {
-			return &revision
-		}
-	}
-
-	return nil
-}
-
 // isProjectOptOutHelp returns if the project is opt-out from getting help
 // information about how to run presubmit tests on their changes.
 func isProjectOptOutHelp(projectsOptOutHelp map[string]sets.String, instance, project string) bool {

prow/gerrit/adapter/adapter_test.go

@@ -19,7 +19,6 @@ package adapter
 import (
 	"errors"
 	"fmt"
-	"reflect"
 	"sync"
 	"testing"
 	"time"
@@ -304,124 +303,6 @@ func TestCreateRefs(t *testing.T) {
 	}
 }
 
-func TestFindLatestRevisionWithoutCodeChange(t *testing.T) {
-	now := time.Now()
-	cases := []struct {
-		name       string
-		change     gerrit.ChangeInfo
-		lastUpdate time.Time
-		expected   *gerrit.RevisionInfo
-	}{
-		{
-			name: "should return nil if no code change since last update",
-			change: client.ChangeInfo{
-				Project:         "foo",
-				ChangeID:        "33521",
-				CurrentRevision: "2",
-				Revisions: map[string]gerrit.RevisionInfo{
-					"1": {
-						Number:  1,
-						Created: makeStamp(now.Add(-time.Hour)),
-						Kind:    gerrit.MergeFirstParentUpdate,
-					},
-					"2": {
-						Number:  2,
-						Created: makeStamp(now.Add(time.Hour)),
-						Kind:    gerrit.NoCodeChange,
-					},
-				},
-			},
-			lastUpdate: now,
-			expected:   nil,
-		},
-		{
-			name: "should return last revision with code change since last update when the last revision has no code change",
-			change: client.ChangeInfo{
-				Project:         "foo",
-				ChangeID:        "33521",
-				CurrentRevision: "3",
-				Revisions: map[string]gerrit.RevisionInfo{
-					"1": {
-						Number:  1,
-						Created: makeStamp(now.Add(-time.Hour)),
-						Kind:    gerrit.MergeFirstParentUpdate,
-					},
-					"2": {
-						Number:  2,
-						Created: makeStamp(now.Add(time.Hour)),
-						Kind:    gerrit.Rework,
-					},
-					"3": {
-						Number:  3,
-						Created: makeStamp(now.Add(2 * time.Hour)),
-						Kind:    gerrit.NoCodeChange,
-					},
-				},
-			},
-			lastUpdate: now,
-			expected: &client.RevisionInfo{
-				Number:  2,
-				Created: makeStamp(now.Add(time.Hour)),
-				Kind:    gerrit.Rework,
-			},
-		},
-		{
-			name: "should return last revision with code change since last update when the last revision has code change",
-			change: client.ChangeInfo{
-				Project:         "foo",
-				ChangeID:        "33521",
-				CurrentRevision: "4",
-				Revisions: map[string]gerrit.RevisionInfo{
-					"1": {
-						Number:  1,
-						Created: makeStamp(now.Add(-time.Hour)),
-						Kind:    gerrit.MergeFirstParentUpdate,
-					},
-					"2": {
-						Number:  2,
-						Created: makeStamp(now.Add(time.Hour)),
-						Kind:    gerrit.NoCodeChange,
-					},
-					"3": {
-						Number:  3,
-						Created: makeStamp(now.Add(2 * time.Hour)),
-						Kind:    gerrit.MergeFirstParentUpdate,
-					},
-					"4": {
-						Number:  4,
-						Created: makeStamp(now.Add(3 * time.Hour)),
-						Kind:    gerrit.Rework,
-					},
-				},
-			},
-			lastUpdate: now,
-			expected: &client.RevisionInfo{
-				Number:  4,
-				Created: makeStamp(now.Add(3 * time.Hour)),
-				Kind:    gerrit.Rework,
-			},
-		},
-		{
-			name: "should return nil if the change list has no revision",
-			change: client.ChangeInfo{
-				Project:  "foo",
-				ChangeID: "33521",
-			},
-			lastUpdate: now,
-			expected:   nil,
-		},
-	}
-
-	for _, tc := range cases {
-		t.Run(tc.name, func(t *testing.T) {
-			rev := lastRevisionWithCodeChange(tc.change, tc.lastUpdate)
-			if !reflect.DeepEqual(rev, tc.expected) {
-				t.Errorf("expected revision: %+v, got: %+v", tc.expected, rev)
-			}
-		})
-	}
-}
-
 func TestFailedJobs(t *testing.T) {
 	const (
 		me      = 314159

prow/gerrit/client/client.go

@@ -621,7 +621,7 @@ func (h *gerritInstanceHandler) QueryChangesForProject(log logrus.FieldLogger, p
 
 	var opt gerrit.QueryChangeOptions
 	opt.Query = append(opt.Query, strings.Join(append(additionalFilters, "project:"+project), "+"))
-	opt.AdditionalFields = []string{"ALL_REVISIONS", "CURRENT_COMMIT", "CURRENT_FILES", "MESSAGES", "LABELS"}
+	opt.AdditionalFields = []string{"CURRENT_REVISION", "CURRENT_COMMIT", "CURRENT_FILES", "MESSAGES", "LABELS"}
 
 	log = log.WithFields(logrus.Fields{"query": opt.Query, "additional_fields": opt.AdditionalFields})
 	var start int