diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index fda75a5ec..42cf15ddc 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -269,6 +269,34 @@ jobs: if: ${{ failure() }} uses: ./.github/actions/upload-testlogs + validator-integration-test: + name: Validator Extension Test + if: ${{ github.event_name != 'pull_request' || github.event.pull_request.draft == false }} + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4.3.1 + with: + # This job executes untrusted PR code (make build/test/lint). Don't + # leave the GITHUB_TOKEN in the workspace git config while it runs. + persist-credentials: false + - uses: ./.github/actions/setup + + - name: Run validator extension tests + env: + # Opens pull requests in the test repository; see the secrets guard + # in workflow-security. + SQ_GITHUB_TOKEN: ${{ secrets.SQ_TEST_REPO_TOKEN }} + SQ_GITHUB_TEST_REPO: ${{ vars.SQ_GITHUB_TEST_REPO }} + # Fail rather than skip when the secret is missing, but only in a + # repository that set up a test repo (forks without one skip), and + # not on fork PRs, which GitHub never gives secrets to. + SQ_GITHUB_TEST_REQUIRED: ${{ vars.SQ_GITHUB_TEST_REPO != '' && (github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == github.repository) }} + run: make integration-test-submitqueue-validator + + - name: Upload Bazel failure logs + if: ${{ failure() }} + uses: ./.github/actions/upload-testlogs + storage-integration-test: name: Storage Extension Test if: ${{ github.event_name != 'pull_request' || github.event.pull_request.draft == false }} @@ -346,10 +374,10 @@ jobs: # .github/. GITHUB_TOKEN (least-privilege, read-only here) is allowlisted. # # SQ_TEST_REPO_TOKEN is the one deliberate exception. The live GitHub - # tests (e2e and merger extension) merge real pull requests in a separate - # test repository, which GITHUB_TOKEN cannot reach. It is a fine-grained - # token limited to that repository, so a PR that exfiltrated it could only - # reach that repository. + # tests (e2e, merger and validator extension) open and merge real pull + # requests in a separate test repository, which GITHUB_TOKEN cannot + # reach. It is a fine-grained token limited to that repository, so a PR + # that exfiltrated it could only reach that repository. - name: Guard — no repository secrets on the untrusted-code path run: | hits="$(grep -rnE '\$\{\{[^}]*secrets\.' \ @@ -387,6 +415,7 @@ jobs: - counter-integration-test - queue-integration-test - merger-integration-test + - validator-integration-test - storage-integration-test - consumer-integration-test - web diff --git a/Makefile b/Makefile index 5f015981d..6699fbfe8 100644 --- a/Makefile +++ b/Makefile @@ -155,7 +155,7 @@ define assert_clean fi endef -.PHONY: build build-all-linux build-runway-linux build-submitqueue-gateway-client build-submitqueue-gateway-linux build-submitqueue-gateway-server build-submitqueue-orchestrator-linux build-stovepipe-linux build-stovepipe-linux-debug check-gazelle check-mocks check-tidy clean clean-proto demo-requests deps docs-build docs-serve e2e-test fmt gazelle integration-test integration-test-submitqueue-consumer integration-test-extensions integration-test-submitqueue-gateway integration-test-submitqueue-orchestrator license-fix lint lint-binary lint-fmt lint-license local-init-runway-queue-schema local-init-stovepipe-schemas local-runway-start local-runway-stop local-submitqueue-stop local-submitqueue-clean local-submitqueue-gateway-start local-submitqueue-gateway-stop local-init-submitqueue-schemas local-submitqueue-logs local-submitqueue-orchestrator-start local-submitqueue-orchestrator-stop local-submitqueue-ps local-submitqueue-restart local-submitqueue-start local-stop local-stovepipe-debug-start local-stovepipe-logs local-stovepipe-start local-stovepipe-stop mocks proto query-deps query-targets run-client-runway run-client-submitqueue-gateway run-client-submitqueue-orchestrator run-client-stovepipe run-queue-admin test test-no-cache test-race tidy tidy-bazel tidy-go web-build web-check web-e2e-test web-image-load web-install web-proto help +.PHONY: build build-all-linux build-runway-linux build-submitqueue-gateway-client build-submitqueue-gateway-linux build-submitqueue-gateway-server build-submitqueue-orchestrator-linux build-stovepipe-linux build-stovepipe-linux-debug check-gazelle check-mocks check-tidy clean clean-proto demo-requests deps docs-build docs-serve e2e-test fmt gazelle integration-test integration-test-submitqueue-consumer integration-test-extensions integration-test-submitqueue-gateway integration-test-submitqueue-orchestrator integration-test-submitqueue-validator license-fix lint lint-binary lint-fmt lint-license local-init-runway-queue-schema local-init-stovepipe-schemas local-runway-start local-runway-stop local-submitqueue-stop local-submitqueue-clean local-submitqueue-gateway-start local-submitqueue-gateway-stop local-init-submitqueue-schemas local-submitqueue-logs local-submitqueue-orchestrator-start local-submitqueue-orchestrator-stop local-submitqueue-ps local-submitqueue-restart local-submitqueue-start local-stop local-stovepipe-debug-start local-stovepipe-logs local-stovepipe-start local-stovepipe-stop mocks proto query-deps query-targets run-client-runway run-client-submitqueue-gateway run-client-submitqueue-orchestrator run-client-stovepipe run-queue-admin test test-no-cache test-race tidy tidy-bazel tidy-go web-build web-check web-e2e-test web-image-load web-install web-proto help build: ## Build all services and examples @@ -320,6 +320,10 @@ integration-test-submitqueue-orchestrator: ## Run Orchestrator integration tests @echo "Running Orchestrator integration tests..." @$(BAZEL) test //test/integration/submitqueue/orchestrator:go_default_test --test_output=streamed +integration-test-submitqueue-validator: ## Run the GitHub validator extension test (needs SQ_GITHUB_TOKEN and SQ_GITHUB_TEST_REPO=owner/repo, else skips) + @echo "Running GitHub validator extension tests..." + @$(BAZEL) test //test/integration/submitqueue/extension/validator/... --test_output=errors + land: ## Land a change or a stack (PR=, PRS=" ", or URI=) @if [ -z "$(PR)$(PRS)$(URI)$(URIS)" ]; then \ echo "Usage: make land PR=https://github.com/owner/repo/pull/7"; \ diff --git a/doc/howto/TESTING.md b/doc/howto/TESTING.md index a5f70cc31..a23136d6d 100644 --- a/doc/howto/TESTING.md +++ b/doc/howto/TESTING.md @@ -94,7 +94,7 @@ make build-all-linux # Build Linux binaries for the local docker- ### Live GitHub Tests -Two suites land real pull requests on github.com: `TestGitHubLandE2E` (in `make e2e-test`) and the Runway GitHub merger extension test (`make integration-test-runway-merger`). They run only when `SQ_GITHUB_TOKEN` and `SQ_GITHUB_TEST_REPO=owner/repo` are set, and skip otherwise, so every other run is unaffected. The token needs write access to the test repository; everything the tests open is named under `sq-it/` and removed afterwards, while what they merge stays on its default branch. +Three suites open real pull requests on github.com: `TestGitHubLandE2E` (in `make e2e-test`), which lands them through the whole stack with the GitHub change provider, validator and merger; the Runway GitHub merger extension test (`make integration-test-runway-merger`); and the GitHub validator extension test (`make integration-test-submitqueue-validator`). They run only when `SQ_GITHUB_TOKEN` and `SQ_GITHUB_TEST_REPO=owner/repo` are set, and skip otherwise, so every other run is unaffected. The token needs write access to the test repository; everything the tests open is named under `sq-it/` and removed afterwards, while what they merge stays on its default branch. ```bash SQ_GITHUB_TOKEN=$(gh auth token) SQ_GITHUB_TEST_REPO=/ make e2e-test @@ -107,7 +107,7 @@ CI takes the repository from the `SQ_GITHUB_TEST_REPO` repository variable and t Any contributor can run these suites against a repository of their own: 1. Create a repository on github.com with a default branch (an initial commit is enough). It must allow squash and rebase merges, and its default branch must not require reviews or status checks, since the tests merge directly. -2. Create a token that can write to it: a fine-grained token limited to that repository with read and write access to contents, pull requests and issues, or simply `gh auth token`. +2. Create a token that can write to it: a fine-grained token limited to that repository with read and write access to contents, pull requests and issues and read access to checks and commit statuses, or simply `gh auth token`. 3. Run locally with `SQ_GITHUB_TOKEN` and `SQ_GITHUB_TEST_REPO` set, as above. 4. For CI in your fork, set the `SQ_GITHUB_TEST_REPO` repository variable and the `SQ_TEST_REPO_TOKEN` repository secret in the fork's Actions settings. Runs in the fork then exercise your test repository. diff --git a/submitqueue/extension/validator/github/README.md b/submitqueue/extension/validator/github/README.md index cca9df8fc..7986aac44 100644 --- a/submitqueue/extension/validator/github/README.md +++ b/submitqueue/extension/validator/github/README.md @@ -42,3 +42,12 @@ The validator adds no credential and no base URL. Its HTTP client's transport ro ## Metrics Under the `github_validator` scope, tagged with the `queue`: `validate.start` and the `validate.finish` latency histogram, whose `result` tag is `error` only for a failure to read GitHub, and `validate.rejections` tagged with the rejection `reason`. + +## Live tests + +Two suites run against a real repository whenever `SQ_GITHUB_TOKEN` and `SQ_GITHUB_TEST_REPO=owner/repo` are set, and skip otherwise (see [`test/testutil/githubtestrepo`](../../../../test/testutil/githubtestrepo)): + +- [`test/integration/submitqueue/extension/validator/github`](../../../../test/integration/submitqueue/extension/validator/github) validates throwaway pull requests on their own (`make integration-test-submitqueue-validator`). +- `TestGitHubLandE2E` in [`test/e2e/submitqueue`](../../../../test/e2e/submitqueue) validates every pull request it lands through the whole stack (`make e2e-test`). + +The test repository requires no reviews or checks, since the tests merge directly, so the suites cover the passing path against the real API, and the extension test also a moved head and a missing pull request; the review and check rules themselves are covered by the unit tests. CI runs both in its e2e and validator extension jobs. diff --git a/test/e2e/submitqueue/github_suite_test.go b/test/e2e/submitqueue/github_suite_test.go index a16ad06be..500253f26 100644 --- a/test/e2e/submitqueue/github_suite_test.go +++ b/test/e2e/submitqueue/github_suite_test.go @@ -17,8 +17,9 @@ // The git suite (git_suite_test.go) proves the merge machinery against a bare // repository and deliberately leaves out the half that is specific to a change // provider. This suite covers that half: the orchestrator reads change metadata -// from GitHub, and Runway lands each change through the GitHub merger, so a -// request reaching `landed` here means GitHub itself merged the pull requests. +// from GitHub and validates each request against the repository's own rules, +// and Runway lands each change through the GitHub merger, so a request reaching +// `landed` here means GitHub itself merged the pull requests. // Builds stay fake — what is under test is the land path, not CI. // // It needs a test repository and a token with write access to it, and skips @@ -177,6 +178,7 @@ func (s *GitHubLandSuite) writeProviderConfig() string { queues: - name: %s changeProvider: {type: github} + validator: {type: github} analyzer: {type: none} `, githubQueue) merge := fmt.Sprintf(`defaults: diff --git a/test/integration/submitqueue/extension/validator/github/BUILD.bazel b/test/integration/submitqueue/extension/validator/github/BUILD.bazel new file mode 100644 index 000000000..07fcb2072 --- /dev/null +++ b/test/integration/submitqueue/extension/validator/github/BUILD.bazel @@ -0,0 +1,30 @@ +load("@rules_go//go:def.bzl", "go_test") + +go_test( + name = "go_default_test", + timeout = "long", + srcs = ["github_test.go"], + # The test repository and its credential reach the test only when set; without it the + # test skips (see test/testutil/githubtestrepo). + env_inherit = [ + "SQ_GITHUB_TEST_REPO", + "SQ_GITHUB_TEST_REQUIRED", + "SQ_GITHUB_TOKEN", + ], + tags = [ + "external", + "integration", + "requires-network", + ], + deps = [ + "//platform/base/change:go_default_library", + "//submitqueue/entity:go_default_library", + "//submitqueue/extension/validator:go_default_library", + "//submitqueue/extension/validator/github:go_default_library", + "//test/testutil/githubtestrepo:go_default_library", + "@com_github_stretchr_testify//assert:go_default_library", + "@com_github_stretchr_testify//require:go_default_library", + "@com_github_uber_go_tally//:go_default_library", + "@org_uber_go_zap//zaptest:go_default_library", + ], +) diff --git a/test/integration/submitqueue/extension/validator/github/github_test.go b/test/integration/submitqueue/extension/validator/github/github_test.go new file mode 100644 index 000000000..3008f4580 --- /dev/null +++ b/test/integration/submitqueue/extension/validator/github/github_test.go @@ -0,0 +1,98 @@ +// Copyright (c) 2026 Uber Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// Package github exercises the GitHub validator against a real repository on +// github.com, the one place its GraphQL query and its reading of the response +// meet the real API rather than a fake. The test repository is the one the +// merger lands pull requests in without approvals or required checks, so a +// fresh pull request there must pass. +// +// It skips unless a test repository is configured; see test/testutil/githubtestrepo. +package github + +import ( + "context" + "fmt" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/uber-go/tally" + "go.uber.org/zap/zaptest" + + "github.com/uber/submitqueue/platform/base/change" + "github.com/uber/submitqueue/submitqueue/entity" + "github.com/uber/submitqueue/submitqueue/extension/validator" + githubvalidator "github.com/uber/submitqueue/submitqueue/extension/validator/github" + "github.com/uber/submitqueue/test/testutil/githubtestrepo" +) + +func newValidator(t *testing.T, repo *githubtestrepo.TestRepo) validator.Validator { + t.Helper() + v, err := githubvalidator.New(githubvalidator.Params{ + Config: validator.Config{QueueName: "sq-it"}, + HTTPClient: repo.HTTPClient(), + Host: githubtestrepo.Host, + Logger: zaptest.NewLogger(t).Sugar(), + MetricsScope: tally.NoopScope, + }) + require.NoError(t, err) + return v +} + +func landRequest(repo *githubtestrepo.TestRepo, uris ...string) entity.Request { + return entity.Request{ID: "sq-it/" + repo.RunID(), Queue: "sq-it", Change: change.Change{URIs: uris}} +} + +func TestGitHubValidator(t *testing.T) { + ctx := context.Background() + + t.Run("open pull request passes", func(t *testing.T) { + repo := githubtestrepo.New(t) + v := newValidator(t, repo) + p := repo.OpenPull("pass", repo.Trunk()) + + assert.NoError(t, v.Validate(ctx, landRequest(repo, repo.URI(p)))) + }) + + t.Run("stack passes", func(t *testing.T) { + repo := githubtestrepo.New(t) + v := newValidator(t, repo) + pulls := repo.OpenStack("stack", 2) + + assert.NoError(t, v.Validate(ctx, landRequest(repo, repo.URIs(pulls...)...))) + }) + + t.Run("pull request whose head moved is rejected", func(t *testing.T) { + repo := githubtestrepo.New(t) + v := newValidator(t, repo) + p := repo.OpenPull("stale", repo.Trunk()) + req := landRequest(repo, repo.URI(p)) + // Passing first pins the failure below on the moved head: the query + // reads this pull request fine, and nothing else changes in between. + require.NoError(t, v.Validate(ctx, req)) + repo.AwaitPullHead(p.Number, repo.CommitFile(p.Branch, repo.FilePath("stale-followup"), "followup")) + + require.Error(t, v.Validate(ctx, req)) + }) + + t.Run("pull request that does not exist fails", func(t *testing.T) { + repo := githubtestrepo.New(t) + v := newValidator(t, repo) + p := repo.OpenPull("missing", repo.Trunk()) + missing := fmt.Sprintf("github://%s/%s/%s/pull/%d/%s", githubtestrepo.Host, repo.Owner(), repo.Repo(), 1<<30, p.Head) + + require.Error(t, v.Validate(ctx, landRequest(repo, missing))) + }) +}