Remove memory backend#135
Conversation
There was a problem hiding this comment.
Pull request overview
This PR removes the standalone in-memory S3 “memory backend” and shifts tests/dev tooling to exercise the Sia backend using an ephemeral SQLite store plus an in-memory/mocked Sia SDK, aligning with Issue #132’s goal of avoiding duplicated backend logic.
Changes:
- Replaced the old
testutilMemoryBackend with a Sia-backed default test backend (testutil.NewBackend) using SQLite +testutil.MemorySDK. - Moved the in-memory SDK used by Sia tests into
internal/testutiland updated Sia tests to use it. - Updated SQLite object-part retrieval to also hydrate
FileNamewhen fetching a specific multipart part.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
sia/sia_test.go |
Drops local MemorySDK test double in favor of testutil.NewMemorySDK and simplifies tester setup. |
sia/persist/sqlite/objects.go |
Ensures FileName is selected/scanned when retrieving a specific multipart part. |
sia/objects_test.go |
Updates tests to use testutil.NewMemorySDK and exported counters/flags. |
sia/multipart_test.go |
Updates Sia multipart tests to use testutil.NewMemorySDK. |
s3/objects_test.go |
Fixes part-size assertion and disables multi-owner checks due to backend changes. |
s3/multipart_test.go |
Switches to default tester backend; multi-owner checks are commented out with TODOs. |
s3/buckets_test.go |
Switches to default tester backend; multi-owner checks are commented out with TODOs. |
internal/testutil/testutil.go |
Makes testutil.NewTester default to a Sia+SQLite backend. |
internal/testutil/backend.go |
Removes the old MemoryBackend implementation; adds MemorySDK and NewBackend(tb) helper. |
cmd/s3d-tox/main.go |
Updates tox harness to run against Sia backend with SQLite + in-memory SDK. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // // upload to a bucket that we don't own | ||
| // otherTester := s3Tester.ChangeAccessKey(t, "foo", "bar") | ||
| // _, err = otherTester.PutObject(t.Context(), bucket, object, bytes.NewReader(data), metadata) | ||
| // testutil.AssertS3Error(t, s3errs.ErrAccessDenied, err) |
There was a problem hiding this comment.
These access-control assertions are now commented out, which removes coverage for cross-account authorization (e.g., expecting ErrAccessDenied when using a different access key). Prefer keeping the test as an explicit t.Skip (with a tracking issue) or rewriting it to assert the current intended behavior, so this gap doesn’t silently persist.
| // // upload to a bucket that we don't own | |
| // otherTester := s3Tester.ChangeAccessKey(t, "foo", "bar") | |
| // _, err = otherTester.PutObject(t.Context(), bucket, object, bytes.NewReader(data), metadata) | |
| // testutil.AssertS3Error(t, s3errs.ErrAccessDenied, err) | |
| t.Run("upload to a bucket that we don't own", func(t *testing.T) { | |
| t.Skip("TODO: restore cross-account authorization coverage once the sia backend supports owners; expected behavior is ErrAccessDenied for a different access key") | |
| otherTester := s3Tester.ChangeAccessKey(t, "foo", "bar") | |
| _, err = otherTester.PutObject(t.Context(), bucket, object, bytes.NewReader(data), metadata) | |
| testutil.AssertS3Error(t, s3errs.ErrAccessDenied, err) | |
| }) |
| // TODO: restore multi-owner coverage once the sia backend supports owners. | ||
| // // prepare a backend with 2 keypairs | ||
| // backend := testutil.NewMemoryBackend( | ||
| // testutil.WithKeyPair(testutil.Owner, testutil.AccessKeyID, testutil.SecretAccessKey), | ||
| // testutil.WithKeyPair("other", "foo", "bar"), |
There was a problem hiding this comment.
The multi-owner test setup is commented out here (and similar blocks appear throughout this file), which drops coverage for authorization behavior. Consider replacing the commented blocks with a skipped subtest (e.g., t.Skip with a link to the owner-support issue) to keep the intent visible while avoiding dead commented code.
| // TODO: restore multi-owner coverage once the sia backend supports owners. | ||
| // // prepare a backend with 2 keypairs | ||
| // backend := testutil.NewMemoryBackend( | ||
| // testutil.WithKeyPair(testutil.Owner, testutil.AccessKeyID, testutil.SecretAccessKey), | ||
| // testutil.WithKeyPair("other", "foo", "bar"), |
There was a problem hiding this comment.
This file now has multi-owner coverage commented out (backend setup + cross-account assertions). To avoid losing long-term authorization coverage, prefer using explicit skipped tests (t.Skip with rationale / tracking issue) rather than leaving commented code blocks.
Close #132
Marking as draft because I need to add owners to the Sia backend in another PR