Skip to content

test(build): add missing io import in builder_test.go - #679

Merged
bergwolf merged 1 commit into
mainfrom
fix/builder-test-io-import
Oct 10, 2026
Merged

test(build): add missing io import in builder_test.go#679
bergwolf merged 1 commit into
mainfrom
fix/builder-test-io-import

Conversation

@aftersnow

@aftersnow aftersnow commented Oct 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Problem

The second commit of #592 moved the recordingStrategy helper into pkg/backend/build/builder_test.go. The helper uses io.Reader and io.Copy. The file does not import io.

As a result, go vet and golangci-lint fail on main with this error:

pkg/backend/build/builder_test.go:139:119: undefined: io

The Lint and CI workflows for commit 10a428a are red.

Fix

Add "io" to the import block of builder_test.go. No other change.

Verification

go vet ./pkg/backend/build/
go test -race -count=1 ./pkg/backend/build/
ok  	github.com/modelpack/modctl/pkg/backend/build	1.032s

@aftersnow
test(build): add missing io import in builder_test.go
0dfa78b 
The second commit of #592 moved recordingStrategy into builder_test.go.
The helper uses io.Reader and io.Copy, but the file does not import io.
As a result, go vet and golangci-lint fail on main with "undefined: io",
and the Lint and CI workflows for commit 10a428a are red.

Signed-off-by: Zhao Chen <zhaochen.zju@gmail.com>
@bergwolf
bergwolf merged commit 43991c2 into main Oct 10, 2026
5 checks passed
@bergwolf
bergwolf deleted the fix/builder-test-io-import branch October 10, 2026 14:47
aftersnow added a commit that referenced this pull request Oct 10, 2026
@aftersnow
test(build): add missing io import in builder_test.go
c28a7b4 
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>
This was referenced Oct 10, 2026
Merged
Open
aftersnow added a commit that referenced this pull request Oct 10, 2026
@aftersnow
test(build): add missing io import in builder_test.go
c6b5b62 
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>
bergwolf pushed a commit that referenced this pull request Oct 16, 2026
@aftersnow
test: add comprehensive integration tests across modctl (#495)
c52fc1d 
* docs: add integration tests design spec

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>

* docs: update integration tests spec with review feedback

- 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>

* docs: add integration tests implementation plan

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>

* test: add shared mock OCI registry with fault injection

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

* fix: decouple RequestCountByPath from PathFaults in MockRegistry

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

* test: fix review issues in MockRegistry (data race, PATCH status, test coverage)

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

* test: add TrackingReadCloser utility for resource leak detection

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

* test: add integration tests for ExcludePatterns through NewModelfileByWorkspace

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

* test: add ProgressBar concurrency tests, document #493 data race

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

* test: add pull integration tests covering 6 dimensions

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>

* test: fix pull integration test spec compliance (idempotent assertion, 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>

* test: add push integration tests, document ReadCloser leak #491

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>

* test: add CLI integration tests for generate command and config validation

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

* test: add retry-dependent slow tests (//go:build slowtest)

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

* test: add stress tests for file count, nesting depth, and pull cycles (//go:build stress)

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

* test: align known-bug tests with fixed main and address review feedback

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>

* style: move build constraints above the license header

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>

* test(build): add missing io import in builder_test.go

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>

* test: make the slowtest and stress suites pass and fix a deadlocking 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>

---------

Signed-off-by: Zhao Chen <winters.zc@antgroup.com>
Signed-off-by: Zhao Chen <zhaochen.zju@gmail.com>
bergwolf pushed a commit that referenced this pull request Oct 16, 2026
@aftersnow
feat: add disk space pre-check for build and pull (#471)
fb0e12d 
* feat: add disk space pre-check for build and pull operations

Check available disk space before writing to local storage. When space
is insufficient (with 10% safety margin), log a warning but continue
the operation without returning an error.

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

* fix: log filepath.Walk errors during build size estimation

Add debug logging for errors encountered during directory walking in
estimateBuildSize, so that inaccessible paths are visible instead of
being silently ignored.

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

* fix: address review feedback for disk space check

- Add overflow guard for Bavail * Bsize multiplication (cap at MaxInt64)
- Handle negative values in formatBytes (return "0 B")
- Add edge case tests for Check (empty path, root dir, deeply nested)
- Add unit tests for estimateBuildSize (files, dirs, missing, empty)
- Fix copyright year to 2025 in new files

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

* fix(build): expand wildcards in estimateBuildSize

The previous implementation treated every Modelfile entry as a literal
filesystem path, causing wildcard patterns (e.g. models/*.bin) and
absolute paths to be silently skipped. processor/base.go already
handles those patterns via filepath.Glob, so the disk-space pre-check
was under-reporting the required size for common Modelfile inputs,
defeating the warning.

Mirror the processor/base.go expansion rules (literal with absolute-path
support, or filepath.Glob for patterns containing *?[]) so the estimate
matches what the builder actually processes. Also raise the size-estimation
error log level from Debug to Warn so silent under-estimation becomes
visible.

Addresses review feedback on PR #471 (codex, gemini).

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

* feat(diskspace): make platform requirement explicit, use testify

Add //go:build linux || darwin to check.go to make the Unix-only
requirement explicit at compile time, and introduce a no-op stub in
check_other.go so the package (and its callers in pkg/backend) still
compile on Windows — the pre-check is simply skipped there. Native
Windows support is deferred.

Also rewrite check_test.go using stretchr/testify/assert for
consistency with the rest of the project (e.g. pkg/backend/build_test.go).

Addresses review feedback on PR #471 (gemini).

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

* fix(pull): skip blobs already in local storage in the disk space estimate

A repeated pull of the same model (or of a model that shares layers with
one already pulled) writes nothing for the blobs that already exist in
the local storage, because pullIfNotExist skips them. The pre-check still
summed every layer, so it warned about space the pull did not need.

Add estimatePullSize, which stats each blob in the local storage and
only counts the missing ones. In extract-from-remote mode nothing is
stored locally, so all layers count. A stat error counts the blob, which
keeps the estimate an upper bound.

The build estimate stays an upper bound: digests are not known before
hashing, so blobs from an earlier build of the same workspace cannot be
subtracted. The build warning now says so.

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

* test(build): add missing io import in builder_test.go

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>

---------

Signed-off-by: Zhao Chen <winters.zc@antgroup.com>
Signed-off-by: Zhao Chen <zhaochen.zju@gmail.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bergwolf bergwolf bergwolf approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@aftersnow @bergwolf