[bisect] create combined commit equal check & activity
- move all midpoint activies into its own midpoint.go file.
- checkcombinedcommitequal activity checks that two combined commits
are really equal. if one has modified deps, and the other doesn't,
their keys will naturally not be the same. This equal checks fills in
DEPS through gitiles to ensure that they are truly equal / not equal.
- activity unit tests for both find midpoint and check equal activities.
- activities are updated to create a client only when the context does
not have the midpoint handler already set.
Bug:
Change-Id: I635b5973bf7e848018bddfb118ebd41b708181da
Reviewed-on: https://skia-review.googlesource.com/c/buildbot/+/838697
Commit-Queue: Leina Sun <sunxiaodi@google.com>
Auto-Submit: Jeff Yoon <jeffyoon@google.com>
Reviewed-by: Leina Sun <sunxiaodi@google.com>
diff --git a/pinpoint/go/midpoint/midpoint.go b/pinpoint/go/midpoint/midpoint.go
index 46a1201..cde69d9 100644
--- a/pinpoint/go/midpoint/midpoint.go
+++ b/pinpoint/go/midpoint/midpoint.go
@@ -17,13 +17,17 @@
const (
GitilesEmptyResponseErr = "Gitiles returned 0 commits, which should not happen."
- chromiumSrcGit = "https://chromium.googlesource.com/chromium/src.git"
+ ChromiumSrcGit = "https://chromium.googlesource.com/chromium/src.git"
)
-func NewChromiumCommit(h string) *pb.Commit {
+func NewChromiumCommit(gitHash string) *pb.Commit {
+ return NewCommit(ChromiumSrcGit, gitHash)
+}
+
+func NewCommit(repository, gitHash string) *pb.Commit {
return &pb.Commit{
- GitHash: h,
- Repository: chromiumSrcGit,
+ GitHash: gitHash,
+ Repository: repository,
}
}
@@ -429,6 +433,21 @@
return midCommitFromDEPS, nil
}
+// Equal takes two combined commits and returns whether they are equal.
+//
+// Modified deps affects the equality of two combined commits. If the length of
+// both modified deps are not equal between first and second, this check will
+// backfill modified deps information from DEPS files such that they are equal
+// before calculating and comparing the key.
+func (m *MidpointHandler) Equal(ctx context.Context, first, second *CombinedCommit) (bool, error) {
+ err := m.fillModifiedDeps(ctx, first, second)
+ if err != nil {
+ return false, skerr.Fmt("Failed to sync modified deps for both commits.")
+ }
+
+ return first.Key() == second.Key(), nil
+}
+
// FindMidCombinedCommit searches for the median commit between two combined commits.
//
// The search takes place through Main if no ModifiedDeps are present.
diff --git a/pinpoint/go/midpoint/midpoint_test.go b/pinpoint/go/midpoint/midpoint_test.go
index 131a2e9..d8a472c 100644
--- a/pinpoint/go/midpoint/midpoint_test.go
+++ b/pinpoint/go/midpoint/midpoint_test.go
@@ -110,9 +110,9 @@
gc.On("ReadFileAtRef", testutils.AnyContext, "DEPS", gitHash).Return([]byte(sampleDeps), nil)
c := mockhttpclient.NewURLMock().Client()
- r := New(ctx, c).WithRepo(chromiumSrcGit, gc)
+ r := New(ctx, c).WithRepo(ChromiumSrcGit, gc)
- res, err := r.fetchGitDeps(ctx, &pb.Commit{Repository: chromiumSrcGit, GitHash: gitHash})
+ res, err := r.fetchGitDeps(ctx, &pb.Commit{Repository: ChromiumSrcGit, GitHash: gitHash})
require.NoError(t, err)
// intellij should be missing
assert.Equal(t, 3, len(res))
@@ -189,7 +189,7 @@
gc.On("LogFirstParent", testutils.AnyContext, startGitHash, endGitHash).Return(resp, nil)
c := mockhttpclient.NewURLMock().Client()
- m := New(ctx, c).WithRepo(chromiumSrcGit, gc)
+ m := New(ctx, c).WithRepo(ChromiumSrcGit, gc)
start := NewCombinedCommit(NewChromiumCommit(startGitHash))
end := NewCombinedCommit(NewChromiumCommit(endGitHash))
@@ -280,7 +280,7 @@
wgc.On("LogFirstParent", testutils.AnyContext, wStartGitHash, wEndGitHash).Return(wResp, nil)
c := mockhttpclient.NewURLMock().Client()
- m := New(ctx, c).WithRepo(chromiumSrcGit, gc).WithRepo(webrtcUrl, wgc)
+ m := New(ctx, c).WithRepo(ChromiumSrcGit, gc).WithRepo(webrtcUrl, wgc)
start := NewCombinedCommit(NewChromiumCommit(startGitHash))
end := NewCombinedCommit(NewChromiumCommit(endGitHash))
@@ -499,7 +499,7 @@
})
c := mockhttpclient.NewURLMock().Client()
- m := New(ctx, c).WithRepo(chromiumSrcGit, gc).WithRepo(webrtcUrl, wgc)
+ m := New(ctx, c).WithRepo(ChromiumSrcGit, gc).WithRepo(webrtcUrl, wgc)
res, err := m.FindMidCombinedCommit(ctx, start, end)
assert.NoError(t, err)
// endGitHash is popped off, leaving [4, 3, 2, 1]
@@ -678,7 +678,7 @@
ctx := context.Background()
c := mockhttpclient.NewURLMock().Client()
- m := New(ctx, c).WithRepo(chromiumSrcGit, chromiumClient).WithRepo(webrtcUrl, webrtcClient)
+ m := New(ctx, c).WithRepo(ChromiumSrcGit, chromiumClient).WithRepo(webrtcUrl, webrtcClient)
err := m.fillModifiedDeps(ctx, start, end)
require.NoError(t, err)
@@ -731,7 +731,7 @@
ctx := context.Background()
c := mockhttpclient.NewURLMock().Client()
- m := New(ctx, c).WithRepo(chromiumSrcGit, chromiumClient).WithRepo(webrtcUrl, webrtcClient)
+ m := New(ctx, c).WithRepo(ChromiumSrcGit, chromiumClient).WithRepo(webrtcUrl, webrtcClient)
err := m.fillModifiedDeps(ctx, start, end)
require.NoError(t, err)
diff --git a/pinpoint/go/workflows/internal/BUILD.bazel b/pinpoint/go/workflows/internal/BUILD.bazel
index 5af0a57..b1f7668 100644
--- a/pinpoint/go/workflows/internal/BUILD.bazel
+++ b/pinpoint/go/workflows/internal/BUILD.bazel
@@ -9,6 +9,7 @@
"build_chrome.go",
"commits_runner.go",
"compare.go",
+ "midpoint.go",
"options.go",
"pairwise_runner.go",
"run_benchmark.go",
@@ -47,12 +48,24 @@
"build_chrome_test.go",
"commits_runner_test.go",
"compare_test.go",
+ "midpoint_test.go",
"pairwise_runner_test.go",
"run_benchmark_test.go",
],
+ data = glob(["testdata/**"]),
embed = [":internal"],
+ embedsrcs = [
+ "testdata/LatestChromiumDEPS",
+ "testdata/LatestV8DEPS",
+ "testdata/NMinusOneChromiumDEPS",
+ "testdata/NMinusOneV8DEPS",
+ ],
deps = [
+ "//go/gitiles/mocks",
+ "//go/mockhttpclient",
"//go/swarming",
+ "//go/testutils",
+ "//go/vcsinfo",
"//pinpoint/go/backends",
"//pinpoint/go/compare",
"//pinpoint/go/midpoint",
@@ -63,6 +76,7 @@
"@com_github_stretchr_testify//mock",
"@com_github_stretchr_testify//require",
"@io_temporal_go_sdk//testsuite",
+ "@io_temporal_go_sdk//worker",
"@io_temporal_go_sdk//workflow",
"@org_chromium_go_luci//buildbucket/proto",
"@org_chromium_go_luci//common/api/swarming/swarming/v1:swarming",
diff --git a/pinpoint/go/workflows/internal/bisect.go b/pinpoint/go/workflows/internal/bisect.go
index 1b8fb3b..9428736 100644
--- a/pinpoint/go/workflows/internal/bisect.go
+++ b/pinpoint/go/workflows/internal/bisect.go
@@ -7,8 +7,6 @@
"time"
"github.com/google/uuid"
- "go.skia.org/infra/go/auth"
- "go.skia.org/infra/go/httputils"
"go.skia.org/infra/go/skerr"
"go.skia.org/infra/pinpoint/go/backends"
"go.skia.org/infra/pinpoint/go/compare"
@@ -16,7 +14,6 @@
"go.skia.org/infra/pinpoint/go/workflows"
"go.skia.org/infra/temporal/go/common"
"go.temporal.io/sdk/workflow"
- "golang.org/x/oauth2/google"
pinpoint_proto "go.skia.org/infra/pinpoint/proto/v1"
)
@@ -77,22 +74,6 @@
}
}
-// FindMidCommitActivity is an Activity that finds the middle point of two commits.
-//
-// TODO(b/326352320): Move this into its own file.
-func FindMidCommitActivity(ctx context.Context, lower, higher *midpoint.CombinedCommit) (*midpoint.CombinedCommit, error) {
- httpClientTokenSource, err := google.DefaultTokenSource(ctx, auth.ScopeReadOnly)
- if err != nil {
- return nil, skerr.Wrapf(err, "Problem setting up default token source")
- }
- c := httputils.DefaultClientConfig().WithTokenSource(httpClientTokenSource).Client()
- m, err := midpoint.New(ctx, c).FindMidCombinedCommit(ctx, lower, higher)
- if err != nil {
- return nil, skerr.Wrap(err)
- }
- return m, nil
-}
-
// ReportStatusActivity wraps the call to IssueTracker to report culprits.
func ReportStatusActivity(ctx context.Context, issueID int, culprits []*pinpoint_proto.CombinedCommit) error {
transport, err := backends.NewIssueTrackerTransport(ctx)
@@ -287,7 +268,12 @@
break
}
- if mid.Key() == lower.Build.Commit.Key() {
+ var equal bool
+ if err := workflow.ExecuteActivity(ctx, CheckCombinedCommitEqualActivity, lower.Build.Commit, mid).Get(ctx, &equal); err != nil {
+ logger.Warn("Failed to determine equality between two combined commits")
+ break
+ }
+ if equal {
// TODO(b/329502712): Append additional info to bisectionExecution
// such as p-values, average difference
be.Culprits = append(be.Culprits, (*pinpoint_proto.CombinedCommit)(higher.Build.Commit))
diff --git a/pinpoint/go/workflows/internal/midpoint.go b/pinpoint/go/workflows/internal/midpoint.go
new file mode 100644
index 0000000..f65a89f
--- /dev/null
+++ b/pinpoint/go/workflows/internal/midpoint.go
@@ -0,0 +1,54 @@
+package internal
+
+import (
+ "context"
+
+ "go.skia.org/infra/go/auth"
+ "go.skia.org/infra/go/httputils"
+ "go.skia.org/infra/go/skerr"
+ "go.skia.org/infra/pinpoint/go/midpoint"
+
+ "golang.org/x/oauth2/google"
+)
+
+type MidpointHandlerKey struct{}
+
+var MidpointHandlerContextKey = &MidpointHandlerKey{}
+
+// FindMidCommitActivity is an Activity that finds the middle point of two commits.
+func FindMidCommitActivity(ctx context.Context, lower, higher *midpoint.CombinedCommit) (*midpoint.CombinedCommit, error) {
+ handler, ok := ctx.Value(MidpointHandlerContextKey).(*midpoint.MidpointHandler)
+ if !ok {
+ httpClientTokenSource, err := google.DefaultTokenSource(ctx, auth.ScopeReadOnly)
+ if err != nil {
+ return nil, skerr.Wrapf(err, "problem setting up default token source")
+ }
+ c := httputils.DefaultClientConfig().WithTokenSource(httpClientTokenSource).Client()
+ handler = midpoint.New(ctx, c)
+ }
+
+ m, err := handler.FindMidCombinedCommit(ctx, lower, higher)
+ if err != nil {
+ return nil, skerr.Wrapf(err, "failed to identify midpoint")
+ }
+ return m, nil
+}
+
+// CheckCombinedCommitEqualActivity checks whether two combined commits are equal.
+func CheckCombinedCommitEqualActivity(ctx context.Context, lower, higher *midpoint.CombinedCommit) (bool, error) {
+ handler, ok := ctx.Value(MidpointHandlerContextKey).(*midpoint.MidpointHandler)
+ if !ok {
+ httpClientTokenSource, err := google.DefaultTokenSource(ctx, auth.ScopeReadOnly)
+ if err != nil {
+ return false, skerr.Wrapf(err, "Problem setting up default token source")
+ }
+ c := httputils.DefaultClientConfig().WithTokenSource(httpClientTokenSource).Client()
+ handler = midpoint.New(ctx, c)
+ }
+ equal, err := handler.Equal(ctx, lower, higher)
+ if err != nil {
+ return equal, skerr.Wrapf(err, "failed to determine combined commit equality")
+ }
+
+ return equal, nil
+}
diff --git a/pinpoint/go/workflows/internal/midpoint_test.go b/pinpoint/go/workflows/internal/midpoint_test.go
new file mode 100644
index 0000000..f536d64
--- /dev/null
+++ b/pinpoint/go/workflows/internal/midpoint_test.go
@@ -0,0 +1,376 @@
+package internal
+
+import (
+ "context"
+ "testing"
+
+ "github.com/stretchr/testify/assert"
+ "github.com/stretchr/testify/require"
+ "go.temporal.io/sdk/testsuite"
+ "go.temporal.io/sdk/worker"
+
+ "go.skia.org/infra/go/gitiles/mocks"
+ "go.skia.org/infra/go/mockhttpclient"
+ "go.skia.org/infra/go/testutils"
+ "go.skia.org/infra/go/vcsinfo"
+ "go.skia.org/infra/pinpoint/go/midpoint"
+
+ _ "embed"
+)
+
+//go:embed testdata/LatestChromiumDEPS
+var LatestChromiumDEPS string
+
+//go:embed testdata/LatestV8DEPS
+var LatestV8DEPS string
+
+//go:embed testdata/NMinusOneChromiumDEPS
+var NMinusOneChromiumDEPS string
+
+//go:embed testdata/NMinusOneV8DEPS
+var NMinusOneV8DEPS string
+
+const (
+ LatestChromiumGitHash = "f8e1800"
+ NMinusOneChromiumGitHash = "836476df"
+ NMinusTwoChromiumGitHash = "93dd4db"
+
+ V8Url = "https://chromium.googlesource.com/v8/v8"
+ LatestV8GitHash = "21b24dd"
+ NMinusOneV8GitHash = "ae02432"
+ NMinusTwoV8GitHash = "385416a"
+
+ SkiaUrl = "https://skia.googlesource.com/skia"
+ LatestSkiaGitHash = "14dd552"
+ NMinusOneSkiaGitHash = "0335263"
+ NMinusTwoSkiaGitHash = "3e3f28d"
+)
+
+func createShortCommit(gitHash string) *vcsinfo.ShortCommit {
+ return &vcsinfo.ShortCommit{
+ Hash: gitHash,
+ }
+}
+
+func createResponse(commit ...*vcsinfo.ShortCommit) []*vcsinfo.LongCommit {
+ resp := make([]*vcsinfo.LongCommit, 0)
+ for _, c := range commit {
+ resp = append(resp, &vcsinfo.LongCommit{
+ ShortCommit: c,
+ })
+ }
+ return resp
+}
+
+func runFindMidCommitActivity(t *testing.T, ctx context.Context, lower, higher *midpoint.CombinedCommit) *midpoint.CombinedCommit {
+ testSuite := &testsuite.WorkflowTestSuite{}
+ env := testSuite.NewTestActivityEnvironment().SetWorkerOptions(worker.Options{BackgroundActivityContext: ctx})
+ env.RegisterActivity(FindMidCommitActivity)
+
+ res, err := env.ExecuteActivity(FindMidCommitActivity, lower, higher)
+ require.NoError(t, err)
+
+ var actual *midpoint.CombinedCommit
+ err = res.Get(&actual)
+ require.NoError(t, err)
+ return actual
+}
+
+func runCombinedCommitEqualActivity(t *testing.T, ctx context.Context, first, second *midpoint.CombinedCommit) bool {
+ testSuite := &testsuite.WorkflowTestSuite{}
+ env := testSuite.NewTestActivityEnvironment().SetWorkerOptions(worker.Options{BackgroundActivityContext: ctx})
+ env.RegisterActivity(CheckCombinedCommitEqualActivity)
+
+ res, err := env.ExecuteActivity(CheckCombinedCommitEqualActivity, first, second)
+ require.NoError(t, err)
+
+ var actual bool
+ err = res.Get(&actual)
+ require.NoError(t, err)
+ return actual
+}
+
+// TODO(jeffyoon@) - all tests below can avoid context setting with internal handler behavior mocked
+// if the handler itself is mocked.
+func TestFindMidCommitActivity_NoModifiedDeps_MidpointInMain(t *testing.T) {
+ ctx := context.Background()
+
+ // n+2 vs latest, so midpoint should be n+1
+ lower := midpoint.NewCombinedCommit(midpoint.NewChromiumCommit(NMinusTwoChromiumGitHash))
+ higher := midpoint.NewCombinedCommit(midpoint.NewChromiumCommit(LatestChromiumGitHash))
+
+ chromiumRepo := &mocks.GitilesRepo{}
+
+ // The response always returns inclusive of the latest.
+ logFirstParentResp := createResponse(
+ createShortCommit(LatestChromiumGitHash),
+ createShortCommit(NMinusOneChromiumGitHash),
+ )
+ chromiumRepo.On("LogFirstParent", testutils.AnyContext, NMinusTwoChromiumGitHash, LatestChromiumGitHash).Return(logFirstParentResp, nil)
+
+ c := mockhttpclient.NewURLMock().Client()
+ handler := midpoint.New(ctx, c).WithRepo(midpoint.ChromiumSrcGit, chromiumRepo)
+
+ ctx = context.WithValue(ctx, MidpointHandlerContextKey, handler)
+
+ actual := runFindMidCommitActivity(t, ctx, lower, higher)
+ assert.Equal(t, NMinusOneChromiumGitHash, actual.Main.GitHash)
+}
+
+func TestFindMidCommitActivity_AdjacentMain_MidpointInDep(t *testing.T) {
+ ctx := context.Background()
+
+ // adjacent, so logic assumes a deps roll and parses deps for midpoint
+ lower := midpoint.NewCombinedCommit(midpoint.NewChromiumCommit(NMinusOneChromiumGitHash))
+ higher := midpoint.NewCombinedCommit(midpoint.NewChromiumCommit(LatestChromiumGitHash))
+
+ chromiumRepo := &mocks.GitilesRepo{}
+
+ adjacentChromiumResp := createResponse(
+ createShortCommit(LatestChromiumGitHash),
+ )
+ chromiumRepo.On("LogFirstParent", testutils.AnyContext, NMinusOneChromiumGitHash, LatestChromiumGitHash).Return(adjacentChromiumResp, nil)
+
+ // DEPS roll will be from N-2 to Latest
+ chromiumRepo.On("ReadFileAtRef", testutils.AnyContext, "DEPS", NMinusOneChromiumGitHash).Return([]byte(NMinusOneChromiumDEPS), nil)
+ chromiumRepo.On("ReadFileAtRef", testutils.AnyContext, "DEPS", LatestChromiumGitHash).Return([]byte(LatestChromiumDEPS), nil)
+
+ v8Repo := &mocks.GitilesRepo{}
+
+ // Midpoint between N-2 to Latest would return V8 midpoint of N-1
+ adjacentV8Resp := createResponse(
+ createShortCommit(LatestV8GitHash),
+ createShortCommit(NMinusOneV8GitHash),
+ )
+ v8Repo.On("LogFirstParent", testutils.AnyContext, NMinusTwoV8GitHash, LatestV8GitHash).Return(adjacentV8Resp, nil)
+
+ c := mockhttpclient.NewURLMock().Client()
+ handler := midpoint.New(ctx, c).WithRepo(midpoint.ChromiumSrcGit, chromiumRepo).WithRepo(V8Url, v8Repo)
+
+ ctx = context.WithValue(ctx, MidpointHandlerContextKey, handler)
+
+ actual := runFindMidCommitActivity(t, ctx, lower, higher)
+
+ // Main Git Hash should be off of lower, and the first ModifiedDeps entry
+ // should be V8 at N-1
+ assert.Equal(t, NMinusOneChromiumGitHash, actual.Main.GitHash)
+ assert.Equal(t, 1, len(actual.ModifiedDeps))
+ modifiedDep := actual.GetLatestModifiedDep()
+ assert.Equal(t, NMinusOneV8GitHash, modifiedDep.GitHash)
+}
+
+func TestFindMidCommitActivity_AdjacentMain_NoMoreMidpoint(t *testing.T) {
+ ctx := context.Background()
+
+ // adjacent, so logic assumes a deps roll and parses deps for midpoint
+ lower := midpoint.NewCombinedCommit(midpoint.NewChromiumCommit(NMinusOneChromiumGitHash))
+ higher := midpoint.NewCombinedCommit(midpoint.NewChromiumCommit(LatestChromiumGitHash))
+
+ chromiumRepo := &mocks.GitilesRepo{}
+
+ adjacentChromiumResp := createResponse(
+ createShortCommit(LatestChromiumGitHash),
+ )
+ chromiumRepo.On("LogFirstParent", testutils.AnyContext, NMinusOneChromiumGitHash, LatestChromiumGitHash).Return(adjacentChromiumResp, nil)
+
+ // return the same deps content at both commits
+ chromiumRepo.On("ReadFileAtRef", testutils.AnyContext, "DEPS", NMinusOneChromiumGitHash).Return([]byte(LatestChromiumDEPS), nil)
+ chromiumRepo.On("ReadFileAtRef", testutils.AnyContext, "DEPS", LatestChromiumGitHash).Return([]byte(LatestChromiumDEPS), nil)
+
+ c := mockhttpclient.NewURLMock().Client()
+ handler := midpoint.New(ctx, c).WithRepo(midpoint.ChromiumSrcGit, chromiumRepo)
+
+ ctx = context.WithValue(ctx, MidpointHandlerContextKey, handler)
+
+ actual := runFindMidCommitActivity(t, ctx, lower, higher)
+
+ // When there's no midpoint, the lower commit is returned.
+ assert.Equal(t, lower.Main.GitHash, actual.Main.GitHash)
+ assert.Nil(t, actual.ModifiedDeps)
+}
+
+func TestFindMidCommitActivity_LowerNoModifiedDeps_MidpointInDep(t *testing.T) {
+ ctx := context.Background()
+
+ // same base commit, but different modified deps length, so that lower gets
+ // backfilled.
+ lower := midpoint.NewCombinedCommit(midpoint.NewChromiumCommit(NMinusOneChromiumGitHash))
+ higher := midpoint.NewCombinedCommit(
+ midpoint.NewChromiumCommit(NMinusOneChromiumGitHash),
+ midpoint.NewCommit(V8Url, LatestV8GitHash),
+ )
+
+ // Since lower has no modified deps, it's backfilled starting at the Main commit's git hash.
+ // When fetching DEPS content, return the N-2 V8 hash so that midpoint returned is N-1.
+ chromiumRepo := &mocks.GitilesRepo{}
+ chromiumRepo.On("ReadFileAtRef", testutils.AnyContext, "DEPS", NMinusOneChromiumGitHash).Return([]byte(NMinusOneChromiumDEPS), nil)
+
+ v8Repo := &mocks.GitilesRepo{}
+ v8LogFirstParentResp := createResponse(
+ createShortCommit(LatestV8GitHash),
+ createShortCommit(NMinusOneV8GitHash),
+ )
+ v8Repo.On("LogFirstParent", testutils.AnyContext, NMinusTwoV8GitHash, LatestV8GitHash).Return(v8LogFirstParentResp, nil)
+
+ c := mockhttpclient.NewURLMock().Client()
+ handler := midpoint.New(ctx, c).WithRepo(midpoint.ChromiumSrcGit, chromiumRepo).WithRepo(V8Url, v8Repo)
+
+ ctx = context.WithValue(ctx, MidpointHandlerContextKey, handler)
+
+ actual := runFindMidCommitActivity(t, ctx, lower, higher)
+
+ // Main git hash should remain the same
+ assert.Equal(t, lower.Main.GitHash, actual.Main.GitHash)
+ assert.Equal(t, 1, len(actual.ModifiedDeps))
+
+ actualModifiedDep := actual.GetLatestModifiedDep()
+ assert.Equal(t, NMinusOneV8GitHash, actualModifiedDep.GitHash)
+}
+
+func TestFindMidCommitActivity_AdjancentModifiedDeps_MidpointInDep(t *testing.T) {
+ ctx := context.Background()
+
+ // adjacent modified deps
+ lower := midpoint.NewCombinedCommit(
+ midpoint.NewChromiumCommit(NMinusOneChromiumGitHash),
+ midpoint.NewCommit(V8Url, NMinusOneV8GitHash),
+ )
+ higher := midpoint.NewCombinedCommit(
+ midpoint.NewChromiumCommit(NMinusOneChromiumGitHash),
+ midpoint.NewCommit(V8Url, LatestV8GitHash),
+ )
+
+ // Return adjacent. It'll need to traverse DEPS.
+ v8Repo := &mocks.GitilesRepo{}
+ v8LogFirstParentResp := createResponse(
+ createShortCommit(LatestV8GitHash),
+ )
+ v8Repo.On("LogFirstParent", testutils.AnyContext, NMinusOneV8GitHash, LatestV8GitHash).Return(v8LogFirstParentResp, nil)
+ v8Repo.On("ReadFileAtRef", testutils.AnyContext, "DEPS", NMinusOneV8GitHash).Return([]byte(NMinusOneV8DEPS), nil)
+ v8Repo.On("ReadFileAtRef", testutils.AnyContext, "DEPS", LatestV8GitHash).Return([]byte(LatestV8DEPS), nil)
+
+ skiaRepo := &mocks.GitilesRepo{}
+ skiaLogFirstParentResp := createResponse(
+ createShortCommit(LatestSkiaGitHash),
+ createShortCommit(NMinusOneSkiaGitHash),
+ )
+ // DEPS from V8 would give N-2 to Latest for Skia Git Hash, and the midpoint should be N-1.
+ skiaRepo.On("LogFirstParent", testutils.AnyContext, NMinusTwoSkiaGitHash, LatestSkiaGitHash).Return(skiaLogFirstParentResp, nil)
+
+ c := mockhttpclient.NewURLMock().Client()
+ handler := midpoint.New(ctx, c).WithRepo(V8Url, v8Repo).WithRepo(SkiaUrl, skiaRepo)
+
+ ctx = context.WithValue(ctx, MidpointHandlerContextKey, handler)
+
+ actual := runFindMidCommitActivity(t, ctx, lower, higher)
+
+ assert.Equal(t, 2, len(actual.ModifiedDeps))
+ // first modified dep should be lower of v8
+ actualFirstModifiedDep := actual.ModifiedDeps[0]
+ assert.Equal(t, V8Url, actualFirstModifiedDep.Repository)
+ assert.Equal(t, NMinusOneV8GitHash, actualFirstModifiedDep.GitHash)
+ // next modified dep should be skia
+ latestModifiedDep := actual.GetLatestModifiedDep()
+ assert.Equal(t, SkiaUrl, latestModifiedDep.Repository)
+ assert.Equal(t, NMinusOneSkiaGitHash, latestModifiedDep.GitHash)
+}
+
+func TestFindMidCommitActivity_AdjancentModifiedDeps_NoMoreMidpoint(t *testing.T) {
+ ctx := context.Background()
+
+ // adjacent modified deps
+ lower := midpoint.NewCombinedCommit(
+ midpoint.NewChromiumCommit(NMinusOneChromiumGitHash),
+ midpoint.NewCommit(V8Url, NMinusOneV8GitHash),
+ )
+ higher := midpoint.NewCombinedCommit(
+ midpoint.NewChromiumCommit(NMinusOneChromiumGitHash),
+ midpoint.NewCommit(V8Url, LatestV8GitHash),
+ )
+
+ // Return adjacent. It'll need to traverse DEPS.
+ v8Repo := &mocks.GitilesRepo{}
+ v8LogFirstParentResp := createResponse(
+ createShortCommit(LatestV8GitHash),
+ )
+ v8Repo.On("LogFirstParent", testutils.AnyContext, NMinusOneV8GitHash, LatestV8GitHash).Return(v8LogFirstParentResp, nil)
+ v8Repo.On("ReadFileAtRef", testutils.AnyContext, "DEPS", NMinusOneV8GitHash).Return([]byte(NMinusOneV8DEPS), nil)
+ // return the same DEPS file so that there's nothing more to traverse
+ v8Repo.On("ReadFileAtRef", testutils.AnyContext, "DEPS", LatestV8GitHash).Return([]byte(NMinusOneV8DEPS), nil)
+
+ c := mockhttpclient.NewURLMock().Client()
+ handler := midpoint.New(ctx, c).WithRepo(V8Url, v8Repo)
+
+ ctx = context.WithValue(ctx, MidpointHandlerContextKey, handler)
+
+ actual := runFindMidCommitActivity(t, ctx, lower, higher)
+ // response should be equal to lower
+ assert.Equal(t, 1, len(actual.ModifiedDeps))
+ latestModifiedDep := actual.GetLatestModifiedDep()
+ assert.Equal(t, V8Url, latestModifiedDep.Repository)
+ assert.Equal(t, NMinusOneV8GitHash, latestModifiedDep.GitHash)
+}
+
+func TestCheckCombinedCommitEqual_NoModifiedDeps_Equal(t *testing.T) {
+ ctx := context.Background()
+
+ first := midpoint.NewCombinedCommit(midpoint.NewChromiumCommit(LatestChromiumGitHash))
+ second := midpoint.NewCombinedCommit(midpoint.NewChromiumCommit(LatestChromiumGitHash))
+
+ isEqual := runCombinedCommitEqualActivity(t, ctx, first, second)
+ assert.True(t, isEqual)
+}
+
+func TestCheckCombinedCommitEqual_NoModifiedDeps_NotEqual(t *testing.T) {
+ ctx := context.Background()
+
+ first := midpoint.NewCombinedCommit(midpoint.NewChromiumCommit(NMinusTwoChromiumGitHash))
+ second := midpoint.NewCombinedCommit(midpoint.NewChromiumCommit(LatestChromiumGitHash))
+
+ isEqual := runCombinedCommitEqualActivity(t, ctx, first, second)
+ assert.False(t, isEqual)
+}
+
+func TestCheckCombinedCommitEqual_UnevenModifiedDeps_Equal(t *testing.T) {
+ ctx := context.Background()
+
+ first := midpoint.NewCombinedCommit(midpoint.NewChromiumCommit(NMinusOneChromiumGitHash))
+ second := midpoint.NewCombinedCommit(
+ midpoint.NewChromiumCommit(NMinusOneChromiumGitHash),
+ midpoint.NewCommit(V8Url, NMinusTwoV8GitHash),
+ )
+
+ chromiumRepo := &mocks.GitilesRepo{}
+ // DEPS at NMinusOneChromiumGitHash is NMinusTwoV8GitHash for V8
+ // so should be equal.
+ chromiumRepo.On("ReadFileAtRef", testutils.AnyContext, "DEPS", NMinusOneChromiumGitHash).Return([]byte(NMinusOneChromiumDEPS), nil)
+
+ c := mockhttpclient.NewURLMock().Client()
+ handler := midpoint.New(ctx, c).WithRepo(midpoint.ChromiumSrcGit, chromiumRepo)
+ ctx = context.WithValue(ctx, MidpointHandlerContextKey, handler)
+
+ isEqual := runCombinedCommitEqualActivity(t, ctx, first, second)
+ assert.True(t, isEqual)
+}
+
+func TestCheckCombinedCommitEqual_UnevenModifiedDeps_NotEqual(t *testing.T) {
+ ctx := context.Background()
+
+ first := midpoint.NewCombinedCommit(midpoint.NewChromiumCommit(NMinusOneChromiumGitHash))
+ second := midpoint.NewCombinedCommit(
+ midpoint.NewChromiumCommit(NMinusOneChromiumGitHash),
+ midpoint.NewCommit(V8Url, LatestV8GitHash),
+ )
+
+ chromiumRepo := &mocks.GitilesRepo{}
+ // DEPS at NMinusOneChromiumGitHash is NMinusTwoV8GitHash for V8
+ // so should be different since second has V8 set to LatestV8GitHash.
+ chromiumRepo.On("ReadFileAtRef", testutils.AnyContext, "DEPS", NMinusOneChromiumGitHash).Return([]byte(NMinusOneChromiumDEPS), nil)
+
+ c := mockhttpclient.NewURLMock().Client()
+ handler := midpoint.New(ctx, c).WithRepo(midpoint.ChromiumSrcGit, chromiumRepo)
+ ctx = context.WithValue(ctx, MidpointHandlerContextKey, handler)
+
+ isEqual := runCombinedCommitEqualActivity(t, ctx, first, second)
+ assert.False(t, isEqual)
+}
diff --git a/pinpoint/go/workflows/internal/testdata/LatestChromiumDEPS b/pinpoint/go/workflows/internal/testdata/LatestChromiumDEPS
new file mode 100644
index 0000000..3d4e19e
--- /dev/null
+++ b/pinpoint/go/workflows/internal/testdata/LatestChromiumDEPS
@@ -0,0 +1,5 @@
+# DEPS file with reference to V8 commit 21b24dd, which in midpoint_test is
+# the latest V8 git hash.
+deps = {
+ 'src/v8': 'https://chromium.googlesource.com/v8/v8.git' + '@' + '21b24dd',
+}
\ No newline at end of file
diff --git a/pinpoint/go/workflows/internal/testdata/LatestV8DEPS b/pinpoint/go/workflows/internal/testdata/LatestV8DEPS
new file mode 100644
index 0000000..842d591
--- /dev/null
+++ b/pinpoint/go/workflows/internal/testdata/LatestV8DEPS
@@ -0,0 +1,5 @@
+# DEPS file with reference to Skia commit 14dd552, which in midpoint_test is
+# the latest Skia git hash.
+deps = {
+ 'src/third_party/skia': 'https://skia.googlesource.com/skia.git' + '@' + '14dd552',
+}
\ No newline at end of file
diff --git a/pinpoint/go/workflows/internal/testdata/NMinusOneChromiumDEPS b/pinpoint/go/workflows/internal/testdata/NMinusOneChromiumDEPS
new file mode 100644
index 0000000..e59613a
--- /dev/null
+++ b/pinpoint/go/workflows/internal/testdata/NMinusOneChromiumDEPS
@@ -0,0 +1,5 @@
+# DEPS file with reference to V8 commit 385416a, which in midpoint_test is
+# the n-2 commit hash of V8 so that there's ae02432 remaining in the middle.
+deps = {
+ 'src/v8': 'https://chromium.googlesource.com/v8/v8.git' + '@' + '385416a',
+}
\ No newline at end of file
diff --git a/pinpoint/go/workflows/internal/testdata/NMinusOneV8DEPS b/pinpoint/go/workflows/internal/testdata/NMinusOneV8DEPS
new file mode 100644
index 0000000..2107040
--- /dev/null
+++ b/pinpoint/go/workflows/internal/testdata/NMinusOneV8DEPS
@@ -0,0 +1,5 @@
+# DEPS file with reference to Skia commit 3e3f28d, which in midpoint_test is
+# the n-2 commit hash of Skia so that there's 0335263 remaining in the middle.
+deps = {
+ 'src/third_party/skia': 'https://skia.googlesource.com/skia.git' + '@' + '3e3f28d',
+}
\ No newline at end of file
diff --git a/pinpoint/go/workflows/worker/main.go b/pinpoint/go/workflows/worker/main.go
index 205a1ba..e16bb3d 100644
--- a/pinpoint/go/workflows/worker/main.go
+++ b/pinpoint/go/workflows/worker/main.go
@@ -66,6 +66,7 @@
w.RegisterActivity(internal.CompareActivity)
w.RegisterActivity(internal.FindMidCommitActivity)
+ w.RegisterActivity(internal.CheckCombinedCommitEqualActivity)
w.RegisterActivity(internal.ReportStatusActivity)
w.RegisterWorkflowWithOptions(internal.BisectWorkflow, workflow.RegisterOptions{Name: workflows.Bisect})
