Skip to content

Commit 1f2d0b3

Browse files
authored
test(validator): validate real GitHub pull requests in CI e2e and integration tests (#879)
## Why? The GitHub validator's unit tests run against a fake. Only the real API shows whether its GraphQL query and its reading of the response are right: field names, `isRequired` for a given pull request, and errors for a missing pull request. This follows the pattern of the GitHub merger's live tests. ## What? - **`TestGitHubLandE2E`:** `e2e-github-queue` now sets `validator: {type: github}`. Every full-stack land (a single pull request and a stack) passes through the validator against the test repository before it is batched. - **New `test/integration/submitqueue/extension/validator/github`**, which opens throwaway pull requests via `githubtestrepo` and checks that: - an open pull request passes - a stack passes - a pull request whose head moved is rejected - a pull request that doesn't exist fails - **CI:** a new `validator-integration-test` job, added to `required-checks`. - Same gating as the merger job: `SQ_TEST_REPO_TOKEN` and `SQ_GITHUB_TEST_REPO`; it fails rather than skips when the repository opted in but the secret is missing. - The workflow-security note now lists it among the live tests that use that token. - **Make target:** `make integration-test-submitqueue-validator`. - **Docs:** - `doc/howto/TESTING.md` lists the third live suite. - Its token instructions add read access to checks and commit statuses, which the validator reads. - The validator README gains a "Live tests" section. The test repository requires no reviews or checks, because the tests merge directly. So the live suites cover the passing path and real-API behaviour. The review and check rules are covered by the parent PR's unit tests. If `SQ_TEST_REPO_TOKEN` is a fine-grained token and the test repository is private, it may need read access to checks and commit statuses. Without it, GitHub may refuse the check-related fields, the validator would fail, and the e2e land tests would end in `error`. ## Test Plan - Without credentials, both suites skip (verified locally with `bazel test //test/integration/submitqueue/extension/validator/...`). - `make lint check-gazelle check-mocks check-tidy` pass. Run against a real test repository (see `doc/howto/TESTING.md`): ```sh SQ_GITHUB_TOKEN=$(gh auth token) SQ_GITHUB_TEST_REPO=<owner>/<repo> make integration-test-submitqueue-validator SQ_GITHUB_TOKEN=$(gh auth token) SQ_GITHUB_TEST_REPO=<owner>/<repo> make e2e-test ``` ## Issue <!-- Link the issue here. - Use 'Closes #123' if this is the final fix. - Use 'Part of #123' or just '#123' if the feature is still in progress. -->
1 parent ef50dca commit 1f2d0b3

7 files changed

Lines changed: 181 additions & 9 deletions

File tree

‎.github/workflows/ci.yml‎

Lines changed: 33 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -269,6 +269,34 @@ jobs:
269269
if: ${{ failure() }}
270270
uses: ./.github/actions/upload-testlogs
271271

272+
validator-integration-test:
273+
name: Validator Extension Test
274+
if: ${{ github.event_name != 'pull_request' || github.event.pull_request.draft == false }}
275+
runs-on: ubuntu-latest
276+
steps:
277+
- uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4.3.1
278+
with:
279+
# This job executes untrusted PR code (make build/test/lint). Don't
280+
# leave the GITHUB_TOKEN in the workspace git config while it runs.
281+
persist-credentials: false
282+
- uses: ./.github/actions/setup
283+
284+
- name: Run validator extension tests
285+
env:
286+
# Opens pull requests in the test repository; see the secrets guard
287+
# in workflow-security.
288+
SQ_GITHUB_TOKEN: ${{ secrets.SQ_TEST_REPO_TOKEN }}
289+
SQ_GITHUB_TEST_REPO: ${{ vars.SQ_GITHUB_TEST_REPO }}
290+
# Fail rather than skip when the secret is missing, but only in a
291+
# repository that set up a test repo (forks without one skip), and
292+
# not on fork PRs, which GitHub never gives secrets to.
293+
SQ_GITHUB_TEST_REQUIRED: ${{ vars.SQ_GITHUB_TEST_REPO != '' && (github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == github.repository) }}
294+
run: make integration-test-submitqueue-validator
295+
296+
- name: Upload Bazel failure logs
297+
if: ${{ failure() }}
298+
uses: ./.github/actions/upload-testlogs
299+
272300
storage-integration-test:
273301
name: Storage Extension Test
274302
if: ${{ github.event_name != 'pull_request' || github.event.pull_request.draft == false }}
@@ -346,10 +374,10 @@ jobs:
346374
# .github/. GITHUB_TOKEN (least-privilege, read-only here) is allowlisted.
347375
#
348376
# SQ_TEST_REPO_TOKEN is the one deliberate exception. The live GitHub
349-
# tests (e2e and merger extension) merge real pull requests in a separate
350-
# test repository, which GITHUB_TOKEN cannot reach. It is a fine-grained
351-
# token limited to that repository, so a PR that exfiltrated it could only
352-
# reach that repository.
377+
# tests (e2e, merger and validator extension) open and merge real pull
378+
# requests in a separate test repository, which GITHUB_TOKEN cannot
379+
# reach. It is a fine-grained token limited to that repository, so a PR
380+
# that exfiltrated it could only reach that repository.
353381
- name: Guard — no repository secrets on the untrusted-code path
354382
run: |
355383
hits="$(grep -rnE '\$\{\{[^}]*secrets\.' \
@@ -387,6 +415,7 @@ jobs:
387415
- counter-integration-test
388416
- queue-integration-test
389417
- merger-integration-test
418+
- validator-integration-test
390419
- storage-integration-test
391420
- consumer-integration-test
392421
- web

‎Makefile‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -155,7 +155,7 @@ define assert_clean
155155
fi
156156
endef
157157

158-
.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
158+
.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
159159

160160

161161
build: ## Build all services and examples
@@ -320,6 +320,10 @@ integration-test-submitqueue-orchestrator: ## Run Orchestrator integration tests
320320
@echo "Running Orchestrator integration tests..."
321321
@$(BAZEL) test //test/integration/submitqueue/orchestrator:go_default_test --test_output=streamed
322322

323+
integration-test-submitqueue-validator: ## Run the GitHub validator extension test (needs SQ_GITHUB_TOKEN and SQ_GITHUB_TEST_REPO=owner/repo, else skips)
324+
@echo "Running GitHub validator extension tests..."
325+
@$(BAZEL) test //test/integration/submitqueue/extension/validator/... --test_output=errors
326+
323327
land: ## Land a change or a stack (PR=<url>, PRS="<url> <url>", or URI=<change-uri>)
324328
@if [ -z "$(PR)$(PRS)$(URI)$(URIS)" ]; then \
325329
echo "Usage: make land PR=https://github.com/owner/repo/pull/7"; \

‎doc/howto/TESTING.md‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,7 @@ make build-all-linux # Build Linux binaries for the local docker-
9494

9595
### Live GitHub Tests
9696

97-
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.
97+
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.
9898

9999
```bash
100100
SQ_GITHUB_TOKEN=$(gh auth token) SQ_GITHUB_TEST_REPO=<owner>/<repo> make e2e-test
@@ -107,7 +107,7 @@ CI takes the repository from the `SQ_GITHUB_TEST_REPO` repository variable and t
107107
Any contributor can run these suites against a repository of their own:
108108

109109
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.
110-
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`.
110+
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`.
111111
3. Run locally with `SQ_GITHUB_TOKEN` and `SQ_GITHUB_TEST_REPO` set, as above.
112112
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.
113113

‎submitqueue/extension/validator/github/README.md‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,3 +42,12 @@ The validator adds no credential and no base URL. Its HTTP client's transport ro
4242
## Metrics
4343

4444
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`.
45+
46+
## Live tests
47+
48+
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)):
49+
50+
- [`test/integration/submitqueue/extension/validator/github`](../../../../test/integration/submitqueue/extension/validator/github) validates throwaway pull requests on their own (`make integration-test-submitqueue-validator`).
51+
- `TestGitHubLandE2E` in [`test/e2e/submitqueue`](../../../../test/e2e/submitqueue) validates every pull request it lands through the whole stack (`make e2e-test`).
52+
53+
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.

‎test/e2e/submitqueue/github_suite_test.go‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,9 @@
1717
// The git suite (git_suite_test.go) proves the merge machinery against a bare
1818
// repository and deliberately leaves out the half that is specific to a change
1919
// provider. This suite covers that half: the orchestrator reads change metadata
20-
// from GitHub, and Runway lands each change through the GitHub merger, so a
21-
// request reaching `landed` here means GitHub itself merged the pull requests.
20+
// from GitHub and validates each request against the repository's own rules,
21+
// and Runway lands each change through the GitHub merger, so a request reaching
22+
// `landed` here means GitHub itself merged the pull requests.
2223
// Builds stay fake — what is under test is the land path, not CI.
2324
//
2425
// It needs a test repository and a token with write access to it, and skips
@@ -177,6 +178,7 @@ func (s *GitHubLandSuite) writeProviderConfig() string {
177178
queues:
178179
- name: %s
179180
changeProvider: {type: github}
181+
validator: {type: github}
180182
analyzer: {type: none}
181183
`, githubQueue)
182184
merge := fmt.Sprintf(`defaults:
Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
load("@rules_go//go:def.bzl", "go_test")
2+
3+
go_test(
4+
name = "go_default_test",
5+
timeout = "long",
6+
srcs = ["github_test.go"],
7+
# The test repository and its credential reach the test only when set; without it the
8+
# test skips (see test/testutil/githubtestrepo).
9+
env_inherit = [
10+
"SQ_GITHUB_TEST_REPO",
11+
"SQ_GITHUB_TEST_REQUIRED",
12+
"SQ_GITHUB_TOKEN",
13+
],
14+
tags = [
15+
"external",
16+
"integration",
17+
"requires-network",
18+
],
19+
deps = [
20+
"//platform/base/change:go_default_library",
21+
"//submitqueue/entity:go_default_library",
22+
"//submitqueue/extension/validator:go_default_library",
23+
"//submitqueue/extension/validator/github:go_default_library",
24+
"//test/testutil/githubtestrepo:go_default_library",
25+
"@com_github_stretchr_testify//assert:go_default_library",
26+
"@com_github_stretchr_testify//require:go_default_library",
27+
"@com_github_uber_go_tally//:go_default_library",
28+
"@org_uber_go_zap//zaptest:go_default_library",
29+
],
30+
)
Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,98 @@
1+
// Copyright (c) 2026 Uber Technologies, Inc.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
// Package github exercises the GitHub validator against a real repository on
16+
// github.com, the one place its GraphQL query and its reading of the response
17+
// meet the real API rather than a fake. The test repository is the one the
18+
// merger lands pull requests in without approvals or required checks, so a
19+
// fresh pull request there must pass.
20+
//
21+
// It skips unless a test repository is configured; see test/testutil/githubtestrepo.
22+
package github
23+
24+
import (
25+
"context"
26+
"fmt"
27+
"testing"
28+
29+
"github.com/stretchr/testify/assert"
30+
"github.com/stretchr/testify/require"
31+
"github.com/uber-go/tally"
32+
"go.uber.org/zap/zaptest"
33+
34+
"github.com/uber/submitqueue/platform/base/change"
35+
"github.com/uber/submitqueue/submitqueue/entity"
36+
"github.com/uber/submitqueue/submitqueue/extension/validator"
37+
githubvalidator "github.com/uber/submitqueue/submitqueue/extension/validator/github"
38+
"github.com/uber/submitqueue/test/testutil/githubtestrepo"
39+
)
40+
41+
func newValidator(t *testing.T, repo *githubtestrepo.TestRepo) validator.Validator {
42+
t.Helper()
43+
v, err := githubvalidator.New(githubvalidator.Params{
44+
Config: validator.Config{QueueName: "sq-it"},
45+
HTTPClient: repo.HTTPClient(),
46+
Host: githubtestrepo.Host,
47+
Logger: zaptest.NewLogger(t).Sugar(),
48+
MetricsScope: tally.NoopScope,
49+
})
50+
require.NoError(t, err)
51+
return v
52+
}
53+
54+
func landRequest(repo *githubtestrepo.TestRepo, uris ...string) entity.Request {
55+
return entity.Request{ID: "sq-it/" + repo.RunID(), Queue: "sq-it", Change: change.Change{URIs: uris}}
56+
}
57+
58+
func TestGitHubValidator(t *testing.T) {
59+
ctx := context.Background()
60+
61+
t.Run("open pull request passes", func(t *testing.T) {
62+
repo := githubtestrepo.New(t)
63+
v := newValidator(t, repo)
64+
p := repo.OpenPull("pass", repo.Trunk())
65+
66+
assert.NoError(t, v.Validate(ctx, landRequest(repo, repo.URI(p))))
67+
})
68+
69+
t.Run("stack passes", func(t *testing.T) {
70+
repo := githubtestrepo.New(t)
71+
v := newValidator(t, repo)
72+
pulls := repo.OpenStack("stack", 2)
73+
74+
assert.NoError(t, v.Validate(ctx, landRequest(repo, repo.URIs(pulls...)...)))
75+
})
76+
77+
t.Run("pull request whose head moved is rejected", func(t *testing.T) {
78+
repo := githubtestrepo.New(t)
79+
v := newValidator(t, repo)
80+
p := repo.OpenPull("stale", repo.Trunk())
81+
req := landRequest(repo, repo.URI(p))
82+
// Passing first pins the failure below on the moved head: the query
83+
// reads this pull request fine, and nothing else changes in between.
84+
require.NoError(t, v.Validate(ctx, req))
85+
repo.AwaitPullHead(p.Number, repo.CommitFile(p.Branch, repo.FilePath("stale-followup"), "followup"))
86+
87+
require.Error(t, v.Validate(ctx, req))
88+
})
89+
90+
t.Run("pull request that does not exist fails", func(t *testing.T) {
91+
repo := githubtestrepo.New(t)
92+
v := newValidator(t, repo)
93+
p := repo.OpenPull("missing", repo.Trunk())
94+
missing := fmt.Sprintf("github://%s/%s/%s/pull/%d/%s", githubtestrepo.Host, repo.Owner(), repo.Repo(), 1<<30, p.Head)
95+
96+
require.Error(t, v.Validate(ctx, landRequest(repo, missing)))
97+
})
98+
}

0 commit comments

Comments
 (0)