Skip to content

test(conformance): add ConformanceOptions to RunIOTests and fix EOF handling - #347

Merged
funkyshu merged 3 commits into
mainfrom
feat/conformance-io-options
Jul 23, 2026
Merged

test(conformance): add ConformanceOptions to RunIOTests and fix EOF handling#347
funkyshu merged 3 commits into
mainfrom
feat/conformance-io-options

Conversation

@funkyshu

Copy link
Copy Markdown
Member

Summary

Prepares the canonical backend/testsuite conformance suite so it can be reused by backends (and a future Testcontainers module, see #294) instead of being duplicated. Two changes:

Added

  • RunIOTests now accepts optional ConformanceOptions (the same options type already used by RunConformanceTests), so a backend can opt out of IO sequences it genuinely cannot support. SkipFTPSpecificTests skips the Write, Seek, Write (partial write) cases — FTP streams writes directly to the server and cannot rewind an in-progress write.
  • The FTP TestIOConformance (vfsintegration) now passes SkipFTPSpecificTests: true; previously it had no way to, so those cases would fail against a real FTP server.

Fixed

  • ExecuteSequence EOF handling. A fixed-size read (R(n)) that returns io.EOF alongside the final bytes is a valid end-of-file condition on some backends. The sequence runner treated any error — including io.EOF — as fatal and aborted. It now recognizes io.EOF as a successful read and continues the sequence.

Notes

  • Reuses the existing ConformanceOptions struct rather than introducing a parallel IOOptions type, keeping one coherent options surface across the suite.
  • The RunIOTests signature change is backward compatible (variadic), so existing callers (mem, os, s3, gs, azure, sftp, dropbox contrib) are unaffected.
  • Skipped cases now use t.Skip (visible as skipped) rather than a silent early return.

Testing

  • go build ./...
  • go vet -tags=vfsintegration ./backend/ftp/... ./backend/testsuite/...
  • go test -tags=vfsintegration -run TestConformance -run TestIOConformance ./backend/mem/... ./backend/os/... — all IO sequences pass, including read-to-EOF cases (mem/os support partial writes, so Write, Seek, Write runs and passes there).
  • golangci-lint run --build-tags=vfsintegration ./backend/testsuite/... ./backend/ftp/... — 0 issues.

Part of a series extracted from #294 (this is "PR B"; the Azure/FTP backend fixes landed in #346).

Made with Cursor

…andling

Adds optional ConformanceOptions to RunIOTests so backends can skip IO
sequences they cannot support; SkipFTPSpecificTests skips the
Write,Seek,Write (partial write) cases. Fixes ExecuteSequence to treat
io.EOF from a fixed-size read as a valid end-of-file (continue) rather
than aborting the sequence. Wires the FTP IO conformance test to skip
partial writes.

Prepares the canonical testsuite for reuse by a testcontainers module
(see #294).

Co-authored-by: Cursor <cursoragent@cursor.com>
@c2fo-cibot c2fo-cibot Bot added the size/M Denotes a PR that changes 30-99 lines label Jul 23, 2026
Addresses review findings on PR B:
- Add untagged tests (backend/testsuite/io_conformance_test.go) that run
  RunIOTests against the mem backend and cover ExecuteSequence's io.EOF
  continue path via a fake that returns io.EOF alongside a full read.
  Previously all callers were vfsintegration-gated, so this logic had no
  coverage in the default test run.
- Replace the fragile description-prefix match for the FTP skip with a
  structured IOTestCase.RequiresSeekWithinWrite field, and lock the
  mapping with a test. Note left re: verifying the exact FTP-unsupported
  sequence set against vsftpd in PR C.

Co-authored-by: Cursor <cursoragent@cursor.com>
@c2fo-cibot c2fo-cibot Bot added size/L Denotes a PR that changes 100-499 lines and removed size/M Denotes a PR that changes 30-99 lines labels Jul 23, 2026
@funkyshu
funkyshu requested a review from Copilot July 23, 2026 02:18

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the backend/testsuite IO conformance suite to be more reusable across backends by (1) allowing IO tests to accept ConformanceOptions (consistent with RunConformanceTests) so backends can explicitly skip unsupported IO sequences, and (2) improving the sequence runner’s handling of io.EOF on fixed-size reads to avoid incorrectly failing valid EOF scenarios.

Changes:

  • Extended RunIOTests to accept optional ConformanceOptions, enabling selective skipping of IO sequences (e.g., FTP partial-write patterns).
  • Added IO conformance unit tests (mem-backed) to cover both the new skip path and the updated EOF behavior.
  • Updated the FTP integration conformance test to pass SkipFTPSpecificTests: true for unsupported seek-within-write sequences.

Reviewed changes

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

File Description
CHANGELOG.md Documents the new IO conformance options support and the EOF-handling fix under Unreleased.
backend/testsuite/io_conformance.go Adds opt-in skipping for seek-within-write sequences and adjusts EOF handling during fixed-size reads.
backend/testsuite/io_conformance_test.go Adds unit tests for the new RunIOTests options behavior and EOF-on-full-read handling.
backend/ftp/conformance_test.go Passes SkipFTPSpecificTests to IO conformance to avoid unsupported FTP sequences.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread backend/testsuite/io_conformance.go
Addresses Copilot review: ExecuteSequence discarded the byte count from
file.Read and swallowed io.EOF unconditionally, which would also mask a
genuine short read (e.g. (0, io.EOF)) as success. Now only forgive EOF
when n == the requested byte count; anything less remains a failure.

Adds a regression test (shortReadEOFFile) proving a short read paired
with io.EOF is reported as an error rather than silently continuing.

Co-authored-by: Cursor <cursoragent@cursor.com>
@funkyshu
funkyshu merged commit 04ebe51 into main Jul 23, 2026
34 checks passed
@funkyshu
funkyshu deleted the feat/conformance-io-options branch July 23, 2026 02:29
funkyshu added a commit that referenced this pull request Jul 25, 2026
…persedes #294) (#348)

* test(integration): add testcontainers-based backend conformance suite

Adds a new test-only `testcontainers/` module that provisions real backend
servers (minio, azurite, fake-gcs-server, atmoz/sftp, fauria/vsftpd) as
disposable Docker containers and runs the shared backend/testsuite conformance
and IO suites against them, plus a dedicated "Integration (testcontainers)" CI
workflow. No credentials or manual setup required.

Also fixes the conformance suite's special-character move assertion to compare
File.Path() instead of File.URI() so sftp/ftp pass the same suite.

Derived from the community contribution in #294 by Nathan Baulch, reworked and
split into #346 (Azure/FTP bug fixes), #347 (conformance ConformanceOptions +
EOF handling), and this module.

Co-authored-by: Nathan Baulch <249604+NathanBaulch@users.noreply.github.com>
Co-authored-by: Cursor <cursoragent@cursor.com>

* fix(testcontainers): pin go directive to 1.25.11 per VFS go-version policy

The module's go.mod declared `go 1.26`, which broke the CI matrix job running
on Go 1.25 (VFS supports the latest Go minus one minor version). `go mod tidy`
had resolved github.com/jlaffaye/ftp to the tagged v0.2.1 (which requires
go 1.26) instead of the go-1.17 pseudo-version the core module pins. Pin
jlaffaye/ftp to that same pseudo-version so the whole module builds on Go 1.25,
and set the go directive to 1.25.11 to match the core and contrib modules.

Co-authored-by: Cursor <cursoragent@cursor.com>

* fix(testcontainers): pin FTP control host to IPv4 loopback for CI

On GitHub-hosted runners ctr.Host() returns "localhost", which resolves to the
IPv6 loopback (::1) first. jlaffaye/ftp reuses the control-connection host for
the passive data connection (discarding the private PASV IP the server
advertises), so it dialed [::1]:21100. The passive ports are published on IPv4
only (0.0.0.0) via HostConfigModifier, so those data connections were refused
("dial tcp [::1]:21100: connect: connection refused").

Pin the control host to 127.0.0.1 (already used for PASV_ADDRESS) so the reused
data-connection host stays on IPv4 and matches the published passive ports.

Co-authored-by: Cursor <cursoragent@cursor.com>

* fix(copilot review): address PR #348 review findings

- integration.yml: gate the Docker Hub login step on both DOCKERHUB_USERNAME
  *and* DOCKERHUB_TOKEN being non-empty, so a half-configured secret set does
  not run the login step and fail the workflow.
- backend/testsuite: reword the two special-character move-assertion messages
  so they accurately describe what was checked. The move-back-to-source
  assertion had inherited a copy-pasted "destination file ... source string"
  message that mislabeled `newSrcSpaces` and the expected suffix; both messages
  now describe the operation ("moved-to-destination" vs "moved-back-to-source")
  and the expected suffix explicitly.

The third review comment (loop-variable capture in the errgroup.SetupSuite
goroutines) is a false positive under Go 1.22+ per-iteration loop scoping;
the testcontainers module pins `go 1.25.11`, so `i` and `reg` are correctly
captured per iteration and no code change is needed.

Co-authored-by: Cursor <cursoragent@cursor.com>

---------

Co-authored-by: Nathan Baulch <249604+NathanBaulch@users.noreply.github.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/L Denotes a PR that changes 100-499 lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants