Skip to content

test: run every integration environment in the parallel batch - #981

Merged
karthikiyer56 merged 6 commits into
feature/full-historyfrom
ci-parallel-envs
Sep 9, 2026
Merged

karthikiyer56 merged 6 commits into
feature/full-historyfrom
ci-parallel-envs

Conversation

@karthikiyer56

@karthikiyer56 karthikiyer56 commented Sep 5, 2026 •

Copy link
Copy Markdown
Contributor

Makes the integration suite use all four CPUs and stop doing setup work no test asked for.

Integration test time

Note

These are not the "Integration tests (P27)" / "(P28)" job durations. They are strictly the go test time for the integration package, the number on this line of the job log:

ok  github.com/stellar/stellar-rpc/cmd/stellar-rpc/internal/rpcv1/integrationtest  329.709s

The job duration is a much bigger number and it is not comparable between runs:

it also includes why that makes the job duration useless here
setup-go, which builds librocksdb and libzstd a cache miss turns 11s into 7m 37s, and a miss happens whenever go.sum changes
make build-libs a Rust cache miss turns 20s into 3m 04s
pulling the Core image, installing captive core, rustup update roughly 45s, varies with the registry
compiling the test binary roughly 18s, and longer on a Go build-cache miss

One run in this PR's own history shows the problem: the job took 20m 51s while the tests took 7m 15s, because the merge changed go.sum and every cache missed. Quoting the go test line keeps the comparison about the tests.

stage P27 P28 run
before any CI work, 2026-09-02 ~20m 40s ~20m 53s https://github.com/stellar/stellar-rpc/actions/runs/33660056116
before this PR ~11m ~11m 30s https://github.com/stellar/stellar-rpc/actions/runs/33914634433
after this PR ~5m 30s ~6m https://github.com/stellar/stellar-rpc/actions/runs/34187304894
saved by this PR ~5m 30s, ~50% ~5m 30s, ~47%

0 failed tests, 0 TRY_AGAIN_LATER retries and 0 -race reports in both jobs.

Per-job links for the run after this PR:

job link
Integration tests (P27) https://github.com/stellar/stellar-rpc/actions/runs/34187304894/job/101938119952
Integration tests (P28) https://github.com/stellar/stellar-rpc/actions/runs/34187304894/job/101938119974

What changed

change what it was what it is now
Six tests ran alone
  • Go runs a non-parallel top-level test on its own, with the other three CPUs idle.
  • Five datastore tests called t.Setenv("STORAGE_EMULATOR_HOST", …). An environment variable belongs to the whole program, so Go forbids t.Setenv in a parallel test. They had to set NoParallel: true.
  • TestMigrate never called t.Parallel(), and it does not finish until its own parallel subtests finish, so it blocked the batch twice over.
  • Together: 6m 46s of the run with three of four CPUs doing nothing.
  • The fake GCS server now starts once for the package instead of once per test, from a new TestMain, which sets the variable before any test is running to disturb.
  • newGCSBucket(t) gives each test its own bucket, named after the test, so no two tests see each other's objects.
  • All five t.Setenv and NoParallel flags deleted.
  • TestMigrate and its subtests call t.Parallel().
  • Running one test on its own still works exactly as before — go test -run filtering is unchanged. TestMain is Go's normal package entry point and m.Run() runs only what the filter selected. Verified: -run 'TestHealth$' runs TestHealth alone, 17.0s.
Every test slept 10s
  • upgradeLimitsWithFile ended with time.Sleep(5 * time.Second) before checking whether Core had applied the upgrade.
  • That function runs twice per environment, so each of the 35 default environments slept 10 seconds.
  • Polls Core's /sorobaninfo every 500ms until the upgrade's value appears, capped at 30s.
  • Costs about 0.5s instead of 5s.
  • No time.Sleep left in infrastructure/test.go.
13 tests upgraded limits they never read
  • Every environment ran a two-round Core settings upgrade, about 19s, to raise Soroban resource ceilings.
  • Those ceilings only matter to a transaction that runs a smart contract.
  • Thirteen tests submit only classic operations, or nothing at all. Checked: zero references to contract upload, invoke, simulate or preflight, in the tests and in the helpers they call.
  • Those 13 pass ApplyLimits: skipLimitsUpgrade() and skip the upgrade.
  • Each dropped from about 39s to about 17s.
  • Verified in CI: all 13 pass.
A branch that could never run
  • upgradeLimits had switch limitFile { case "testnet": … }.
  • By that line limitFile was already testnet.p28.xdr, so the case never matched.
  • Decides the expected value from the unformatted name, before the fmt.Sprintf.
  • Hands it to the poll, so the check is the poll's exit condition.

Waits that had to grow

Four environments now boot at the same time instead of one. Every wait below was sized for the old schedule and expired during verification. Each returns the moment its condition holds, so a fast run pays nothing for the bigger number.

wait was is what it waits for how it was found
fillContainerPorts 2s 30s docker compose port reporting a published port one local failure
waitForCore, both polls 30s 2m 00s the core container answering /info, then reaching sync one local failure
waitForRPC 1m 00s 3m 00s captive core replaying the ledgers core already closed seven local failures in one run; the logs showed core still closing ledgers at the deadline, not stuck
the datastore ledger window in TestGetLedgersFromDatastore 30s 1m 30s the local retention window moving past ledger 40 failed in both CI jobs of https://github.com/stellar/stellar-rpc/actions/runs/34169911746. It only ever passed because the settings upgrade slept 10s first, which let the network close 10 extra ledgers. Removing the sleep sprang the trap

The same wait's failure message was also wrong: require.Eventually formats its message arguments before polling starts, so it printed last health: {LatestLedger:0 …} on every failure. It now uses assert.Eventually plus t.Fatalf and prints the real last response.

Not in this PR

35 of the 45 environments are byte-identical NewTest(t, nil) and each still pays about 37s of setup. Sharing one network between them is a separate change.

One fake GCS server starts in TestMain, so the datastore tests no longer
call t.Setenv and no longer need NoParallel. TestMigrate becomes parallel
too. Harness waits are widened because four environments now boot at once.
Thirteen tests never submit a Soroban transaction, so the raised resource
limits do nothing for them. They now skip the upgrade. The upgrade itself
polls Core's /sorobaninfo instead of sleeping a fixed five seconds twice.

Also fixes a dead branch: the testnet case compared against the formatted
file name, so it could never match.
… setup being slow

The 30s window only ever passed because the settings upgrade slept 10s
first. Waiting for the network to close about 16 more ledgers needs a
window sized for that, not for whatever setup happened to cost.
@karthikiyer56
karthikiyer56 marked this pull request as ready for review September 8, 2026 01:10
Copilot AI balanced review requested due to automatic review settings September 8, 2026 01:10

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Improves integration-test parallelism and reduces unnecessary setup time.

Changes:

  • Shares one isolated fake GCS server across datastore tests.
  • Parallelizes migration tests and skips unnecessary limit upgrades.
  • Replaces fixed sleeps with polling and increases startup timeouts.

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
transaction_test.go Skips limits for classic transaction cases.
migrate_test.go Parallelizes migration environments.
metrics_test.go Skips limits setup.
main_test.go Adds shared GCS setup and limits helper.
infrastructure/test.go Adds polling and longer startup timeouts.
health_test.go Skips limits setup.
get_version_info_test.go Skips limits setup.
get_transactions_test.go Skips limits setup.
get_network_test.go Skips limits setup.
get_ledgers_test.go Shares GCS and improves waiting diagnostics.
get_ledger_entries_test.go Skips limits setup.
cors_test.go Skips limits setup.
builtin_methods_disabled_test.go Skips limits setup.
backfill_test.go Shares isolated GCS buckets and enables parallelism.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread cmd/stellar-rpc/internal/rpcv1/integrationtest/infrastructure/test.go Outdated
@karthikiyer56
karthikiyer56 requested a review from a team September 8, 2026 02:56
- the health wait no longer reads what its condition goroutine writes,
  and the condition no longer logs, so a timeout cannot race or panic
- the /sorobaninfo poll waits for a number the second upgrade file sets
  and enable.xdr does not; 65536 was already present and proved nothing
- the number must stand alone, so 3500000 does not match 35000000
- waitForCheckpoint, waitForCoreAtLedger and the backfill waits get the
  same busy-machine budgets their siblings already got
- TestGetLedgers waits for 15 ledgers, since 5 is exactly the first page
  and leaves nothing for the cursor to return
- the fake GCS server only starts when integration tests are enabled
Copilot AI review requested due to automatic review settings September 8, 2026 04:31

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.

Comment thread cmd/stellar-rpc/internal/rpcv1/integrationtest/backfill_test.go Outdated
Comment thread cmd/stellar-rpc/internal/rpcv1/integrationtest/health_test.go
The wait covers 66 ledgers closing at one per second, so the budget is
already the floor it needs. Nothing had failed on it, and the suite was
green in CI without the extra two minutes.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings September 9, 2026 02:38

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.

@karthikiyer56
karthikiyer56 merged commit c9441dc into feature/full-history Sep 9, 2026
15 checks passed
@karthikiyer56
karthikiyer56 deleted the ci-parallel-envs branch September 9, 2026 03:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants