diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 803d9cd4..eec670ed 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -68,8 +68,13 @@ jobs: persist-credentials: false fetch-depth: 0 + - name: Configure git identity + run: | + git config --global user.name "CI" + git config --global user.email "ci@test" + # Persist Bazel's disk cache (see .bazelrc: --disk_cache) across runs. The - # integration test drives nested `bazel query` against the repo itself; a + # integration test drives nested `bazel query` against a fixture repo; a # cold cache makes the first GetChangedTargets pay the full analysis cost # and pushes the run against its deadline. Warming it removes that spike # and speeds every run. restore-keys lets a stale cache seed a new one. diff --git a/Makefile b/Makefile index 484256ca..3edf484c 100644 --- a/Makefile +++ b/Makefile @@ -1,4 +1,4 @@ -.PHONY: build cover test test-integration bench lint proto gazelle clean clean-proto run-server run-client-get-graph run-client-changed-targets version help +.PHONY: build cover test test-integration test-integration-tgb test-integration-gob bench lint proto gazelle clean clean-proto run-server run-client-get-graph run-client-changed-targets version help # Bazel wrapper BAZEL = ./tools/bazel @@ -15,11 +15,32 @@ test: @$(BAZEL) test //... @echo "All tests passed!" -# Run integration tests (requires bazel; may take several minutes) -test-integration: - @echo "Running integration tests..." - @$(BAZEL) test //integration:integration_test --test_output=errors --test_env=TANGO_REPO_REMOTE=$$(git rev-parse --show-toplevel) --test_env=HOME=$$HOME - @echo "Integration tests passed!" +FIXTURE_REMOTE ?= https://github.com/xytan0056/bazel-fixture.git +FIXTURE_BASE_SHA ?= 0d79296cfd507440f536ea01e626b294d74d19a8 +FIXTURE_HEAD_SHA ?= b5f81eb872dfcbe4c13131a73367122eb2d92065 +FIXTURE_PR_URL ?= github://github.com/xytan0056/bazel-fixture/pull/2/$(FIXTURE_HEAD_SHA) + +INTEGRATION_ENV = \ + --test_env=HOME=$$HOME \ + --test_env=TANGO_REPO_REMOTE=$(FIXTURE_REMOTE) \ + --test_env=TANGO_BASE_SHA=$(FIXTURE_BASE_SHA) \ + --test_env=TANGO_HEAD_SHA=$(FIXTURE_HEAD_SHA) \ + --test_env=TANGO_PR_URL=$(FIXTURE_PR_URL) + +# Run integration tests with both graph formats +test-integration: test-integration-tgb test-integration-gob + +test-integration-tgb: + @echo "Running integration tests (tgb)..." + @$(BAZEL) test //integration:integration_test --test_output=errors \ + $(INTEGRATION_ENV) --test_env=TANGO_GRAPH_FORMAT=tgb + @echo "Integration tests (tgb) passed!" + +test-integration-gob: + @echo "Running integration tests (gob)..." + @$(BAZEL) test //integration:integration_test --test_output=errors \ + $(INTEGRATION_ENV) --test_env=TANGO_GRAPH_FORMAT=gob + @echo "Integration tests (gob) passed!" # Run GetChangedTargets benchmarks against fixed, checked-in commit pairs. # Measurement only: not part of `make test` / `make test-integration` and not @@ -116,7 +137,9 @@ help: @echo "Build & Test:" @echo " make build - Build all targets" @echo " make test - Run all tests" - @echo " make test-integration - Run integration tests (slow)" + @echo " make test-integration - Run integration tests with both formats (needs network, slow)" + @echo " make test-integration-tgb - Run integration tests with TGB format" + @echo " make test-integration-gob - Run integration tests with gob format" @echo " make bench - Run GetChangedTargets benchs (measurement only, not in CI)" @echo " make lint - Run golangci-lint" @echo " make gazelle - Update BUILD.bazel files" diff --git a/integration/benchmark_test.go b/integration/benchmark_test.go index 7997bdf3..444ff691 100644 --- a/integration/benchmark_test.go +++ b/integration/benchmark_test.go @@ -23,7 +23,7 @@ import ( "go.uber.org/zap/zapcore" ) -// coldCommitPairs are distinct consecutive commit pairs from repo history. +// coldCommitPairs are distinct consecutive commit pairs from tango repo history. // Each pair produces a unique treehash, guaranteeing a cache miss per call. var coldCommitPairs = []struct{ first, second string }{ {"57162624a45965a7e783072c56561f91c5d4084d", "74d1cd55155e5f4f43aa92b4e0146a0c528a0d96"}, @@ -54,7 +54,7 @@ func BenchmarkGetChangedTargets_Cold(b *testing.B) { b.ResetTimer() for i := 0; i < b.N; i++ { pair := coldCommitPairs[i] - getChangedTargets(b, client, remote, pair.first, pair.second) + getChangedTargets(b, client, buildDesc(remote, pair.first), buildDesc(remote, pair.second)) } } @@ -67,13 +67,13 @@ func BenchmarkGetChangedTargets_Cached(b *testing.B) { addr := startServerWithLogger(b, remote, logger) client := newClient(b, addr) - firstSHA := "57162624a45965a7e783072c56561f91c5d4084d" - secondSHA := "74d1cd55155e5f4f43aa92b4e0146a0c528a0d96" + first := buildDesc(remote, "57162624a45965a7e783072c56561f91c5d4084d") + second := buildDesc(remote, "74d1cd55155e5f4f43aa92b4e0146a0c528a0d96") - getChangedTargets(b, client, remote, firstSHA, secondSHA) + getChangedTargets(b, client, first, second) b.ResetTimer() for i := 0; i < b.N; i++ { - getChangedTargets(b, client, remote, firstSHA, secondSHA) + getChangedTargets(b, client, first, second) } } diff --git a/integration/integration_test.go b/integration/integration_test.go index 8fdd0820..2d6891ad 100644 --- a/integration/integration_test.go +++ b/integration/integration_test.go @@ -14,7 +14,8 @@ // Package integration_test implements integration tests for tango. // Its tests spin up the tango server, create a client that connects to it, -// and calls its APIs using the tango GitHub repository itself as the target. +// and calls its APIs against a dedicated Bazel fixture repository with a +// known, stable dependency graph. package integration_test import ( @@ -49,11 +50,36 @@ const ( configTemplateFile = "testdata/tango-config.yaml.tmpl" ) +func requiredEnv(t testing.TB, key string) string { + t.Helper() + v := os.Getenv(key) + require.NotEmpty(t, v, "%s must be set (pass --test_env=%s=... to bazel test)", key, key) + return v +} + func repoRemote(t testing.TB) string { t.Helper() - remote := os.Getenv("TANGO_REPO_REMOTE") - require.NotEmpty(t, remote, "TANGO_REPO_REMOTE must be set (pass --test_env=TANGO_REPO_REMOTE=... to bazel test)") - return remote + return requiredEnv(t, "TANGO_REPO_REMOTE") +} + +func fixtureBaseSHA(t testing.TB) string { + t.Helper() + return requiredEnv(t, "TANGO_BASE_SHA") +} + +func fixtureHeadSHA(t testing.TB) string { + t.Helper() + return requiredEnv(t, "TANGO_HEAD_SHA") +} + +func fixturePRURL(t testing.TB) string { + t.Helper() + return requiredEnv(t, "TANGO_PR_URL") +} + +func graphFormat(t testing.TB) string { + t.Helper() + return requiredEnv(t, "TANGO_GRAPH_FORMAT") } func writeConfig(t testing.TB, dir, remote, clonePath string) string { @@ -71,10 +97,11 @@ func writeConfig(t testing.TB, dir, remote, clonePath string) string { Remote string ClonePath string BazelCommand string + GraphFormat string }{ - Remote: remote, - ClonePath: clonePath, - BazelCommand: filepath.Join(remote, "tools", "bazel"), + Remote: remote, + ClonePath: clonePath, + GraphFormat: graphFormat(t), }) require.NoError(t, err, "failed to render config template") @@ -160,7 +187,6 @@ func newClient(t testing.TB, addr string) pb.TangoYARPCClient { return pb.NewTangoYARPCClient(dispatcher.ClientConfig("tango")) } -// rawGraph holds the full streamed response before any subgraph extraction. type rawGraph struct { targets []*pb.OptimizedTarget metadata *pb.Metadata @@ -190,8 +216,6 @@ func drainTargetGraphStream(t *testing.T, stream pb.TangoServiceGetTargetGraphYA return result } -// subgraph is a helper for constructing a focused subgraph from a raw proto graph. -// It returns a mapping of target names to list of dependency target names. func subgraph(t *testing.T, raw rawGraph, roots ...string) map[string][]string { t.Helper() @@ -250,23 +274,15 @@ type parsedChangedTargets struct { Distances map[string]int32 } -func getChangedTargets(t testing.TB, client pb.TangoYARPCClient, remote, firstSHA, secondSHA string) parsedChangedTargets { +func getChangedTargets(t testing.TB, client pb.TangoYARPCClient, first, second *pb.BuildDescription) parsedChangedTargets { t.Helper() ctx, cancel := context.WithTimeout(context.Background(), requestTimeout) defer cancel() stream, err := client.GetChangedTargets(ctx, &pb.GetChangedTargetsRequest{ - FirstRevision: &pb.BuildDescription{ - Strategy: pb.COMPUTATION_STRATEGY_UNSET, - Remote: remote, - BaseSha: firstSHA, - }, - SecondRevision: &pb.BuildDescription{ - Strategy: pb.COMPUTATION_STRATEGY_UNSET, - Remote: remote, - BaseSha: secondSHA, - }, + FirstRevision: first, + SecondRevision: second, }) require.NoError(t, err, "failed to initiate GetChangedTargets stream") @@ -344,13 +360,26 @@ func mergeMetadata(existing, incoming *pb.Metadata) *pb.Metadata { return existing } -func TestIntegration_GetTargetGraph(t *testing.T) { - remote := repoRemote(t) +func buildDesc(remote, sha string) *pb.BuildDescription { + return &pb.BuildDescription{ + Strategy: pb.COMPUTATION_STRATEGY_UNSET, + Remote: remote, + BaseSha: sha, + } +} - // Pinned SHA for deterministic assertions. The target count and edges are - // fixed for a given treehash — they only change if this SHA is updated. - const pinnedSHA = "74d1cd55155e5f4f43aa92b4e0146a0c528a0d96" +func assertContainsTarget(t testing.TB, names []string, target, category string) { + t.Helper() + for _, n := range names { + if n == target { + return + } + } + t.Errorf("expected %s targets to contain %q, got: %v", category, target, names) +} +func TestIntegration_GetTargetGraph(t *testing.T) { + remote := repoRemote(t) addr := startServer(t, remote) client := newClient(t, addr) @@ -358,11 +387,7 @@ func TestIntegration_GetTargetGraph(t *testing.T) { defer cancel() stream, err := client.GetTargetGraph(ctx, &pb.GetTargetGraphRequest{ - BuildDescription: &pb.BuildDescription{ - Strategy: pb.COMPUTATION_STRATEGY_UNSET, - Remote: remote, - BaseSha: pinnedSHA, - }, + BuildDescription: buildDesc(remote, fixtureBaseSHA(t)), }) require.NoError(t, err, "failed to initiate GetTargetGraph stream") @@ -371,117 +396,124 @@ func TestIntegration_GetTargetGraph(t *testing.T) { require.NotEmpty(t, raw.metadata.GetTargetIdMapping()) require.NotEmpty(t, raw.metadata.GetRuleTypeMapping()) - assert.Equal(t, 3673, len(raw.targets), "expected exact target count for pinned SHA") + targetNames := make([]string, 0, len(raw.metadata.GetTargetIdMapping())) + for _, name := range raw.metadata.GetTargetIdMapping() { + targetNames = append(targetNames, name) + } + sort.Strings(targetNames) + t.Logf("target count: %d, target names:\n%v", len(raw.targets), targetNames) totalEdges := 0 for _, tgt := range raw.targets { totalEdges += len(tgt.DirectDependencies) } - assert.Equal(t, 8105, totalEdges, "expected exact edge count for pinned SHA") - - t.Run("controller sub-graph contains some correct well-known edges", func(t *testing.T) { - controllerGraph := subgraph(t, raw, "//controller:controller") - assert.Contains(t, controllerGraph["//controller:controller"], "//orchestrator:orchestrator") - assert.Contains(t, controllerGraph["//controller:controller"], "//core/storage:storage") - assert.Contains(t, controllerGraph["//controller:controller"], "//tangopb:tangopb") - assert.Contains(t, controllerGraph["//orchestrator:orchestrator"], "//core/storage:storage") - assert.Contains(t, controllerGraph["//orchestrator:orchestrator"], "//core/repomanager:repomanager") - assert.Contains(t, controllerGraph["//orchestrator:orchestrator"], "//graphrunner:graphrunner") + t.Logf("edge count: %d", totalEdges) + + t.Run("service_depends_on_pkg", func(t *testing.T) { + g := subgraph(t, raw, "//service/handlers:handlers") + assert.Contains(t, g["//service/handlers:handlers"], "//pkg/logger:logger") + assert.Contains(t, g["//service/handlers:handlers"], "//service/store:store") + }) + + t.Run("cmd_depends_on_service_and_config", func(t *testing.T) { + g := subgraph(t, raw, "//cmd/server:server_lib") + assert.Contains(t, g["//cmd/server:server_lib"], "//service/api:api") + assert.Contains(t, g["//cmd/server:server_lib"], "//service/config:config") + assert.Contains(t, g["//cmd/server:server_lib"], "//pkg/logger:logger") }) - t.Run("nodes correctly include external dependencies", func(t *testing.T) { - configGraph := subgraph(t, raw, "//config:config") - assert.Equal(t, []string{ - "//config:config.go", - "//config:repository_config.go", - "//config:service_config.go", - "//config:storage_config.go", - "@bazel_tools//tools/allowlists/function_transition_allowlist:function_transition_allowlist", - "@com_github_goccy_go_yaml//:go-yaml", - "@rules_go//:go_context_data", - }, configGraph["//config:config"]) + t.Run("external_dependencies", func(t *testing.T) { + g := subgraph(t, raw, "//pkg/logger:logger") + loggerDeps := g["//pkg/logger:logger"] + hasExternal := false + for _, dep := range loggerDeps { + if len(dep) > 0 && dep[0] == '@' { + hasExternal = true + break + } + } + assert.True(t, hasExternal, "expected at least one external (@) dependency, got: %v", loggerDeps) + }) + + t.Run("proto_targets", func(t *testing.T) { + found := false + for _, name := range targetNames { + if name == "//proto/common:common_proto" || name == "//proto/api:api_proto" || name == "//proto/store:store_proto" { + found = true + break + } + } + assert.True(t, found, "expected at least one proto_library target in the graph") + }) + + t.Run("embedded_config", func(t *testing.T) { + g := subgraph(t, raw, "//service/config:config") + configDeps := g["//service/config:config"] + hasEmbedded := false + for _, dep := range configDeps { + if dep == "//service/config:config.yaml" || dep == "//service/config:defaults.json" { + hasEmbedded = true + break + } + } + assert.True(t, hasEmbedded, "expected embedded config asset, got: %v", configDeps) }) } func TestIntegration_GetChangedTargets(t *testing.T) { remote := repoRemote(t) - addr := startServer(t, remote) client := newClient(t, addr) - t.Run("changed_only", func(t *testing.T) { - // Compare two adjacent commits: - // 5716262 [core/storage] generic reader implementation (#145) - // 74d1cd5 [core/workspace] fix silent drop of invalid scheme (#146) - // The second commit changes core/workspace/request.go and its test. - const firstSHA = "57162624a45965a7e783072c56561f91c5d4084d" - const secondSHA = "74d1cd55155e5f4f43aa92b4e0146a0c528a0d96" - - ct := getChangedTargets(t, client, remote, firstSHA, secondSHA) - - assert.Empty(t, ct.ByType[pb.CHANGE_TYPE_NEW], "expected no new targets") - assert.Empty(t, ct.ByType[pb.CHANGE_TYPE_DELETED], "expected no deleted targets") - assert.ElementsMatch(t, []string{ - "//controller:controller", - "//controller:controller_test", - "//core/repomanager/mock:mock", - "//core/repomanager:repomanager", - "//core/repomanager:repomanager_test", - "//core/workspace/workspacemock:workspacemock", - "//core/workspace:request.go", - "//core/workspace:request_test.go", - "//core/workspace:workspace", - "//core/workspace:workspace_test", - "//example:example", - "//example:example_lib", - "//graphrunner/mock:mock", - "//graphrunner:graphrunner", - "//graphrunner:graphrunner_test", - "//orchestrator/orchestratormock:orchestratormock", - "//orchestrator:orchestrator", - "//orchestrator:orchestrator_test", - }, ct.ByType[pb.CHANGE_TYPE_CHANGED]) - - assert.Equal(t, int32(0), ct.Distances["//core/workspace:request.go"]) - assert.Equal(t, int32(0), ct.Distances["//core/workspace:workspace"]) - assert.Equal(t, int32(1), ct.Distances["//orchestrator:orchestrator"]) - assert.Equal(t, int32(2), ct.Distances["//controller:controller"]) - assert.Equal(t, int32(3), ct.Distances["//example:example"]) + t.Run("sha_comparison", func(t *testing.T) { + ct := getChangedTargets(t, client, buildDesc(remote, fixtureBaseSHA(t)), buildDesc(remote, fixtureHeadSHA(t))) + + t.Logf("NEW: %v", ct.ByType[pb.CHANGE_TYPE_NEW]) + t.Logf("DELETED: %v", ct.ByType[pb.CHANGE_TYPE_DELETED]) + t.Logf("CHANGED: %v", ct.ByType[pb.CHANGE_TYPE_CHANGED]) + t.Logf("Distances: %v", ct.Distances) + + assert.NotEmpty(t, ct.ByType[pb.CHANGE_TYPE_DELETED]) + assert.NotEmpty(t, ct.ByType[pb.CHANGE_TYPE_NEW]) + assert.NotEmpty(t, ct.ByType[pb.CHANGE_TYPE_CHANGED]) + + assertContainsTarget(t, ct.ByType[pb.CHANGE_TYPE_NEW], "//pkg/timeutil:timeutil", "NEW") + assertContainsTarget(t, ct.ByType[pb.CHANGE_TYPE_NEW], "//proto/audit:audit_proto", "NEW") + assertContainsTarget(t, ct.ByType[pb.CHANGE_TYPE_DELETED], "//pkg/mathutil:mathutil", "DELETED") + assertContainsTarget(t, ct.ByType[pb.CHANGE_TYPE_CHANGED], "//service/handlers:handlers", "CHANGED") + assertContainsTarget(t, ct.ByType[pb.CHANGE_TYPE_CHANGED], "//service/api:api", "CHANGED") + assertContainsTarget(t, ct.ByType[pb.CHANGE_TYPE_CHANGED], "//cmd/server:server_lib", "CHANGED") + + assert.Equal(t, int32(0), ct.Distances["//pkg/strutil:strutil"]) + assert.Equal(t, int32(0), ct.Distances["//service/handlers:handlers"]) + assert.Equal(t, int32(1), ct.Distances["//service/api:api"]) + assert.Equal(t, int32(1), ct.Distances["//cmd/server:server_lib"]) + assert.Equal(t, int32(2), ct.Distances["//cmd/server:server"]) }) - t.Run("new_targets", func(t *testing.T) { - // Compare two adjacent commits: - // 046de2c (parent) - // 1f2e3e9 Honor OutputConfig include_hashes/include_tags/include_attributes (#116) - // The second commit adds controller/output_filter.go and output_filter_test.go. - const firstSHA = "046de2c20b5492cd5606d32fd632a38b8b70c8f6" - const secondSHA = "1f2e3e9245b159006cf2103becd51c5c1b6ec868" - - ct := getChangedTargets(t, client, remote, firstSHA, secondSHA) - - assert.Empty(t, ct.ByType[pb.CHANGE_TYPE_DELETED], "expected no deleted targets") - assert.ElementsMatch(t, []string{ - "//controller:output_filter.go", - "//controller:output_filter_test.go", - }, ct.ByType[pb.CHANGE_TYPE_NEW]) - assert.ElementsMatch(t, []string{ - "//controller:BUILD.bazel", - "//controller:controller", - "//controller:controller_test", - "//controller:getchangedtargets.go", - "//controller:getchangedtargets_test.go", - "//controller:gettargetgraph.go", - "//example/client:client", - "//example/client:client.go", - "//example/client:client_lib", - "//example:example", - "//example:example_lib", - }, ct.ByType[pb.CHANGE_TYPE_CHANGED]) - - assert.Equal(t, int32(0), ct.Distances["//controller:output_filter.go"]) - assert.Equal(t, int32(0), ct.Distances["//controller:getchangedtargets.go"]) - assert.Equal(t, int32(0), ct.Distances["//controller:controller"]) - assert.Equal(t, int32(1), ct.Distances["//example:example_lib"]) - assert.Equal(t, int32(2), ct.Distances["//example:example"]) + t.Run("pr_change_request", func(t *testing.T) { + ct := getChangedTargets(t, client, + buildDesc(remote, fixtureBaseSHA(t)), + &pb.BuildDescription{ + Strategy: pb.COMPUTATION_STRATEGY_UNSET, + Remote: remote, + BaseSha: fixtureBaseSHA(t), + Requests: []*pb.Request{{Url: fixturePRURL(t)}}, + }, + ) + + t.Logf("NEW: %v", ct.ByType[pb.CHANGE_TYPE_NEW]) + t.Logf("DELETED: %v", ct.ByType[pb.CHANGE_TYPE_DELETED]) + t.Logf("CHANGED: %v", ct.ByType[pb.CHANGE_TYPE_CHANGED]) + t.Logf("Distances: %v", ct.Distances) + + assert.NotEmpty(t, ct.ByType[pb.CHANGE_TYPE_NEW]) + assert.NotEmpty(t, ct.ByType[pb.CHANGE_TYPE_DELETED]) + assert.NotEmpty(t, ct.ByType[pb.CHANGE_TYPE_CHANGED]) + + assertContainsTarget(t, ct.ByType[pb.CHANGE_TYPE_NEW], "//pkg/timeutil:timeutil", "NEW") + assertContainsTarget(t, ct.ByType[pb.CHANGE_TYPE_NEW], "//proto/audit:audit_proto", "NEW") + assertContainsTarget(t, ct.ByType[pb.CHANGE_TYPE_DELETED], "//pkg/mathutil:mathutil", "DELETED") + assertContainsTarget(t, ct.ByType[pb.CHANGE_TYPE_CHANGED], "//service/handlers:handlers", "CHANGED") }) } diff --git a/integration/testdata/tango-config.yaml.tmpl b/integration/testdata/tango-config.yaml.tmpl index e5f38ae4..20a38f9f 100644 --- a/integration/testdata/tango-config.yaml.tmpl +++ b/integration/testdata/tango-config.yaml.tmpl @@ -16,3 +16,4 @@ repository: service: max_worker_pool_size: 2 workspaces_root_path: {{.ClonePath}} + graph_format: "{{.GraphFormat}}"