Skip to content

test: add comprehensive integration tests across modctl - #495

Open
aftersnow wants to merge 20 commits into
mainfrom
feat/integration-tests
Open

aftersnow wants to merge 20 commits into
mainfrom
feat/integration-tests

Conversation

@aftersnow

@aftersnow aftersnow commented Apr 8, 2026 •

Copy link
Copy Markdown
Contributor

Summary

New Files (13 files)

File Tests Build Tag
test/helpers/mockregistry.go + _test.go 9 self-tests default
test/helpers/tracking.go — default
pkg/modelfile/modelfile_integration_test.go 2 default
pkg/backend/pull_integration_test.go 11 default
pkg/backend/push_integration_test.go 8 default
internal/pb/pb_integration_test.go 2 default
cmd/modelfile/generate_integration_test.go 3 default
cmd/cli_integration_test.go 3 default
pkg/backend/pull_slowtest_test.go 4 slowtest
pkg/backend/push_slowtest_test.go 1 slowtest
pkg/modelfile/modelfile_stress_test.go 2 stress
pkg/backend/pull_stress_test.go 2 stress

Notes from the review round

  • The TestKnownBug_* reverse-assertion tests were flipped into TestIntegration_* regression tests once main (merged in c38fe8d) fixed bug: Push leaks ReadCloser from PullBlob on both success and error paths #491, bug: disableProgress global variable has data race (no atomic/mutex protection) #493 and bug: retry.Do retries 401/403 auth errors instead of failing immediately #494.
  • MockRegistry reads PATCH and PUT upload bodies before taking its mutex, so network I/O does not serialize unrelated requests.
  • The generated plan and design documents were removed.
  • The slowtest and stress suites are not run by CI. They were broken in the first version of this PR (a global fault hit the manifest fetch, which is outside the retry loop, and the goroutine-leak test counted idle keep-alive connections). Both suites pass now; see the test plan.
  • Finding for a follow-up: every Pull/Push builds its own http.Transport in pkg/backend/remote and never closes its idle keep-alive connections. The CLI exits after one operation, so this only matters for library users that call the backend repeatedly. The stress test disables keep-alives on the mock server to measure real goroutine leaks only.
  • Finding for a follow-up: ProgressBar.Add on an existing name aborts and drops the old bar. Under -race, 10 goroutines replacing 10 bars in a tight loop deadlock mpb while it syncs decorator widths. The concurrent-access test uses unique bar names to avoid that path; production code replaces bars rarely.

Test Plan

  • go test ./... -count=1 — all default tests pass
  • go test -race ./internal/pb/ ./pkg/backend/ ./test/helpers/ — clean; go test -race -count=20 ./internal/pb/ — clean
  • go test ./pkg/backend/ -tags slowtest -timeout 900s — passes (about 150 s)
  • go test ./pkg/modelfile/ ./pkg/backend/ -tags stress -timeout 600s — passes
  • golangci-lint run — 0 issues

aftersnow added 15 commits April 7, 2026 22:00
Define 8 test dimensions (~41 scenarios) covering functional
correctness, network errors, resource leaks, concurrency safety,
stress tests, data integrity, graceful shutdown, and idempotency.
Includes shared mock OCI registry infrastructure design.

Signed-off-by: Zhao Chen <winters.zc@antgroup.com>
- Known bug tests use reverse assertions (CI stays green)
- Retry-dependent network tests moved to //go:build slowtest
- Remove untestable pull reader/tempdir leak scenarios
- Push leak tests cover both success and error paths
- Deduplicate against existing test coverage
- Add GitHub issue references (#491, #492, #493, #494)
- Add auth-error-retry as known design issue

Signed-off-by: Zhao Chen <winters.zc@antgroup.com>
9 tasks covering mock registry infrastructure, resource tracking,
modelfile/pull/push/CLI/pb integration tests, slow tests, and stress
tests. Each task has TDD-style steps with complete code.

Signed-off-by: Zhao Chen <winters.zc@antgroup.com>
Signed-off-by: Zhao Chen <winters.zc@antgroup.com>
Signed-off-by: Zhao Chen <winters.zc@antgroup.com>
…t coverage)

Signed-off-by: Zhao Chen <winters.zc@antgroup.com>
Signed-off-by: Zhao Chen <winters.zc@antgroup.com>
…yWorkspace

Signed-off-by: Zhao Chen <winters.zc@antgroup.com>
Signed-off-by: Zhao Chen <winters.zc@antgroup.com>
Add pkg/backend/pull_integration_test.go with 11 integration tests that
exercise the Pull function using MockRegistry (httptest) and mock Storage:

Functional correctness:
- HappyPath: manifest + 2 blobs, verify PushBlob/PushManifest calls
- BlobAlreadyExists: StatBlob returns true, verify PushBlob skipped
- ConcurrentLayers: 5 blobs in parallel, verify all stored

Network errors:
- ContextTimeout: latency + short deadline, verify context error
- PartialResponse: FailAfterNBytes on blob, verify error
- ManifestOK_BlobFails: 500 on specific blob, verify error

Concurrency safety:
- ConcurrentPartialFailure: 2 of 5 blobs fail, errgroup cancels

Data integrity:
- TruncatedBlob: FailAfterNBytes, digest validation catches mismatch
- CorruptedBlob: wrong bytes under correct digest, caught by validateDigest

Graceful shutdown:
- ContextCancelMidDownload: cancel mid-flight, verify error

Idempotency:
- Idempotent: second pull with all blobs existing skips all writes

Signed-off-by: Zhao Chen <winters.zc@antgroup.com>
…, dedup truncated/partial)

- TestIntegration_Pull_Idempotent: remove unused delta variable and
  misleading comment; clarify that the key invariant is no storage
  writes on second pull (PushBlob/PushManifest not called), not a
  reduction in registry requests.
- TestIntegration_Pull_TruncatedBlob: differentiate from PartialResponse
  by serving same-length wrong-byte content via AddBlobWithDigest instead
  of truncating the stream. This triggers digest-validation failure rather
  than a read error, and asserts the error message contains "digest".
- TestIntegration_Pull_PartialResponse: add RequestCountByPath assertion
  to verify the blob endpoint was contacted at least once.

Signed-off-by: Zhao Chen <winters.zc@antgroup.com>
Add 8 integration tests for the Push workflow covering:
- Functional correctness (happy path, blob-already-exists skip)
- Network errors (manifest push failure via path fault)
- Resource leak documentation (KnownBug reverse assertions for #491)
- Data integrity (byte-level blob verification after push)
- Graceful shutdown (context cancellation mid-upload)
- Idempotency (second push skips existing blobs)

The KnownBug tests use TrackingReadCloser with reverse assertions
(AssertNotClosed) to document that PullBlob ReadClosers are never
closed on either success or error paths. These will fail when #491
is fixed, signaling the assertions should be flipped.

Signed-off-by: Zhao Chen <winters.zc@antgroup.com>
…ation

Signed-off-by: Zhao Chen <winters.zc@antgroup.com>
Signed-off-by: Zhao Chen <winters.zc@antgroup.com>
… (//go:build stress)

Signed-off-by: Zhao Chen <winters.zc@antgroup.com>

@gemini-code-assist gemini-code-assist Bot 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.

Code Review

This pull request introduces a comprehensive integration testing suite for modctl, covering functional correctness, network resilience, concurrency, and stress scenarios. It includes a new mock OCI registry helper and resource tracking utilities. Review feedback identifies a logic error in a 'KnownBug' test that prevents it from failing when fixed, suggests renaming a misleadingly titled test, recommends optimizing the mock registry's performance by reducing mutex contention during I/O, and advises removing a redundant implementation plan file that duplicates the source code.

Comment thread pkg/backend/push_integration_test.go Outdated
Comment thread pkg/backend/pull_integration_test.go Outdated
Comment thread test/helpers/mockregistry.go
Comment thread docs/superpowers/plans/2026-04-07-integration-tests-plan.md Outdated
@aftersnow
aftersnow requested review from bergwolf and imeoer April 9, 2026 04:26
@@ -0,0 +1,2222 @@
# Integration Tests Implementation Plan

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please do not submit your AI's plan.md ;)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Removed in 7dbe732 (both the plan and the design document under docs/superpowers). Sorry for the noise.

Signed-off-by: Zhao Chen <zhaochen.zju@gmail.com>
The branch now includes main, where #491 (ReadCloser leak), #493
(disableProgress data race) and #494 (auth errors retried) are fixed.
The reverse-assertion tests for those bugs fail on the merged tree, as
designed. Flip them into regression tests:

- TestKnownBug_Push_ReadCloserNotClosed_{SuccessPath,ErrorPath} become
  TestIntegration_Push_ReadCloserClosed_{SuccessPath,ErrorPath} and
  assert that Push closes every PullBlob ReadCloser. The error-path test
  no longer guards the assertion with a conditional.
- TestKnownBug_DisableProgress_DataRace becomes
  TestIntegration_DisableProgress_ConcurrentAccess.
- TestKnownBug_Pull_AuthErrorStillRetries becomes
  TestIntegration_Pull_AuthErrorFailsFast and asserts that a 401 fails
  in under 5 seconds.

Review feedback:

- Rename TestIntegration_Pull_TruncatedBlob to
  TestIntegration_Pull_CorruptedContentSameLength; the test serves a
  same-length body with wrong bytes, not a truncated body.
- MockRegistry reads PATCH and PUT upload bodies into a local buffer
  before taking the registry mutex, so network I/O no longer serializes
  unrelated requests.
- Remove the generated plan and design documents under docs/superpowers.

Signed-off-by: Zhao Chen <zhaochen.zju@gmail.com>
gofmt in Go 1.25 places //go:build lines before the leading comment
block. Reformat the tagged test files so the gofmt linter stays clean.

Signed-off-by: Zhao Chen <zhaochen.zju@gmail.com>
Same fix as #679. The recordingStrategy helper merged from main uses
io.Reader and io.Copy without importing io, which breaks go vet and
golangci-lint on this branch.

Signed-off-by: Zhao Chen <zhaochen.zju@gmail.com>
…pb test

The slowtest and stress suites are not run by CI and were broken:

- TestSlow_Pull_RetryOnTransientError, TestSlow_Pull_RateLimited and
  TestSlow_Pull_RetryExhausted used a global fault. The first request
  of a pull is the manifest fetch, which runs outside the retry loop, so
  the pull failed immediately and the retry path was never exercised.
  Scope the faults to the layer blob path with PathFaults.
- TestStress_Pull_RepeatedCycles counted about 5 goroutines per cycle.
  Every Pull builds its own http.Transport and leaves its idle
  keep-alive connections open; both ends of those connections live in
  the test process. Disable keep-alives on the mock server so the test
  measures real leaks only, and poll briefly for teardown.

TestIntegration_DisableProgress_ConcurrentAccess deadlocked under -race:
10 goroutines replacing 10 bars in a tight loop hit an mpb deadlock
between Abort(drop) and the decorator width sync of the render loop.
The race the test guards is on disableProgress, which Add reads before
it touches mpb, so use a unique bar name per call.

Signed-off-by: Zhao Chen <zhaochen.zju@gmail.com>
@aftersnow

Copy link
Copy Markdown
Contributor Author

@bergwolf @imeoer Thanks for the review. Summary of the update:

  • Merged current main (c38fe8d). main has fixed the 3 bugs the reverse-assertion tests documented (bug: Push leaks ReadCloser from PullBlob on both success and error paths #491, bug: disableProgress global variable has data race (no atomic/mutex protection) #493, bug: retry.Do retries 401/403 auth errors instead of failing immediately #494), so those TestKnownBug_* tests now fail as designed. They are flipped into TestIntegration_* regression tests (7dbe732), and the error-path test asserts AssertClosed directly instead of the conditional Gemini flagged.
  • Review items: renamed TestIntegration_Pull_TruncatedBlob to TestIntegration_Pull_CorruptedContentSameLength; MockRegistry reads PATCH/PUT bodies before taking its mutex; the plan and design documents under docs/superpowers are removed.
  • The slowtest and stress suites are not run by CI and were broken in the first version (4774414 fixes them, details in the commit message). Both pass now, and the default suite is race-clean. A pb test that deadlocked mpb under -race is fixed as well.
  • Two findings for follow-ups, not changed here: each Pull/Push builds its own http.Transport and never closes idle keep-alive connections (only matters for library users), and ProgressBar.Add replacing a live bar in a tight loop can deadlock mpb's width sync.
  • Local checks: go test ./..., go test -race ./internal/pb/ ./pkg/backend/ ./test/helpers/, -tags slowtest, -tags stress, and golangci-lint all pass.

PTAL.

This branch has not been deployed

No deployments
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.

bug: Push leaks ReadCloser from PullBlob on both success and error paths

2 participants