Fix presign tests: use fake credentials instead of minio env vars#453
Conversation
📝 WalkthroughWalkthroughA new private test helper Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Brownian Motion (Brass)Recommendation: Proceed Summary: Marking tests as #[ignore] effectively prevents CI failures. Highlights
Unknowns
Next actions
Reflection questions
|
w4nderlust
force-pushed
the
fix/presign-tests-missing-ignore
branch
from
5b74b1b to
cfefd78
Compare
Brownian Motion (Brass)Recommendation: Proceed Summary: PR effectively addresses test failures due to missing environment variables. Highlights
Next actions
Reflection questions
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
s3/src/bucket.rs (1)
3787-3787: Keep these presign tests in the default suite.These cases only exercise local signing logic; they never make a MinIO request. The file already has the right pattern in
test_presign_url_standard_ports: build a bucket with inline fake credentials. Switching these four tests to a static-credential helper would fix theMINIO_*panic without dropping default CI coverage for presign PUT/POST/GET/DELETE.Example approach
- #[ignore] async fn test_presign_put() { - let bucket = test_minio_bucket(); + let bucket = test_presign_bucket(); // ... } - #[ignore] async fn test_presign_post() { - let bucket = test_minio_bucket(); + let bucket = test_presign_bucket(); // ... } - #[ignore] async fn test_presign_get() { - let bucket = test_minio_bucket(); + let bucket = test_presign_bucket(); // ... } - #[ignore] async fn test_presign_delete() { - let bucket = test_minio_bucket(); + let bucket = test_presign_bucket(); // ... }fn test_presign_bucket() -> Box<Bucket> { Bucket::new( "rust-s3", Region::Custom { region: "us-east-1".to_owned(), endpoint: "http://localhost:9000".to_owned(), }, Credentials::new( Some("test_access_key"), Some("test_secret_key"), None, None, None, ) .unwrap(), ) .unwrap() .with_path_style() }As per coding guidelines,
#[ignore]is meant for integration tests that require credentials: "Mark integration tests with#[ignore]if they require AWS credentials".Also applies to: 3815-3815, 3844-3844, 3861-3861
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@s3/src/bucket.rs` at line 3787, Remove the #[ignore] attribute from the four presign tests and make them construct a Bucket with inline fake/static credentials (following the pattern used in test_presign_url_standard_ports) instead of relying on MINIO_* env credentials; add or reuse a helper like test_presign_bucket() that returns a Bucket built with Credentials::new(Some("test_access_key"), Some("test_secret_key"), None, None, None) and the same custom Region/endpoint, and update the presign PUT/POST/GET/DELETE tests to call that helper so they exercise local signing logic without requiring real AWS creds.s3/src/request/request_trait.rs (1)
707-721: Add a provider-agnostic regression test for this header gate.This looks like the right fix, but default CI still has no local assertion that
GET/HEADomitcontent-lengthandcontent-typewhile body-carrying verbs keep them. Right now this behavior is only exercised through ignored provider-backed coverage, so the R2 signing fix can regress silently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@s3/src/request/request_trait.rs` around lines 707 - 721, Add a provider-agnostic unit test that exercises the header-gating logic around HttpMethod so regressions are caught locally: create a small test in the s3 crate (near request_trait.rs tests) that constructs command instances (or a minimal mock implementing the same command interface) with HttpMethod::Get, HttpMethod::Head and a body-carrying verb, call the code path that builds/inserts headers (the code that calls self.command().http_verb(), content_length(), and content_type()) and assert that for GET/HEAD the CONTENT_LENGTH and CONTENT_TYPE headers are absent and for the body verb they are present with expected values; make the test not rely on any external provider so it runs in CI by default.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@s3/src/bucket.rs`:
- Line 3787: Remove the #[ignore] attribute from the four presign tests and make
them construct a Bucket with inline fake/static credentials (following the
pattern used in test_presign_url_standard_ports) instead of relying on MINIO_*
env credentials; add or reuse a helper like test_presign_bucket() that returns a
Bucket built with Credentials::new(Some("test_access_key"),
Some("test_secret_key"), None, None, None) and the same custom Region/endpoint,
and update the presign PUT/POST/GET/DELETE tests to call that helper so they
exercise local signing logic without requiring real AWS creds.
In `@s3/src/request/request_trait.rs`:
- Around line 707-721: Add a provider-agnostic unit test that exercises the
header-gating logic around HttpMethod so regressions are caught locally: create
a small test in the s3 crate (near request_trait.rs tests) that constructs
command instances (or a minimal mock implementing the same command interface)
with HttpMethod::Get, HttpMethod::Head and a body-carrying verb, call the code
path that builds/inserts headers (the code that calls
self.command().http_verb(), content_length(), and content_type()) and assert
that for GET/HEAD the CONTENT_LENGTH and CONTENT_TYPE headers are absent and for
the body verb they are present with expected values; make the test not rely on
any external provider so it runs in CI by default.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dee4b81f-0905-4250-888c-8ce68662766e
📒 Files selected for processing (2)
s3/src/bucket.rss3/src/request/request_trait.rs
test_presign_get, test_presign_put, test_presign_post, and test_presign_delete were using test_minio_bucket() which reads MINIO_ACCESS_KEY_ID from the environment and panics without it, breaking `make ci` on every clean checkout. These tests only exercise local signing logic and never hit the network, so they don't actually need a running minio instance. Switched them to a new test_presign_bucket() helper with hardcoded fake credentials (same pattern as test_presign_url_standard_ports), so they run in the default test suite and provide CI coverage.
w4nderlust
force-pushed
the
fix/presign-tests-missing-ignore
branch
from
cfefd78 to
181664d
Compare
Brownian Motion (Brass)Recommendation: Proceed Summary: PR effectively addresses test failures by using fake credentials. Highlights
Next actions
Reflection questions
|
w4nderlust
changed the title
|
Thanks for fixing these presign tests. This removes the accidental dependency on Minio environment credentials and makes the presign test coverage deterministic with fake credentials, which is exactly what these unit-style signing tests need. Local verification before merge:
This also unblocks the current GitHub CI baseline for follow-up PRs that are failing on missing presign test credentials. Merging now. |
2 participants
Problem
make ci(andcargo test) fails on a clean checkout because 4 presign tests panic trying to readMINIO_ACCESS_KEY_IDfrom the environment:test_presign_gettest_presign_puttest_presign_posttest_presign_deleteFix
These tests only exercise local signing logic and never hit the network, so they don't actually need a running minio instance. Switched them from
test_minio_bucket()(which reads env vars) to a newtest_presign_bucket()helper with hardcoded fake credentials, following the same pattern already used bytest_presign_url_standard_ports.This keeps the tests in the default (non-ignored) suite so they provide CI coverage for presign PUT/POST/GET/DELETE.
Verification
make cipasses clean after this change.This change is
Summary by CodeRabbit
Note: Internal testing update only; no user-facing changes.