mirrors/seaweedfs
1
0
Fork 0
mirror of https://github.com/seaweedfs/seaweedfs.git synced 2026-05-14 05:41:29 +00:00
Code Issues Packages Projects Releases Wiki Activity

test(s3): address CodeRabbit nitpicks on range coverage matrix

Browse Source 
Three small follow-ups on the range-read coverage matrix from the
previous commit, per CodeRabbit nitpicks on PR #9228:

1. Promote the body-length check from `assert.Equal` to `require.Equal`
   so a truncation regression -- the canonical #8908 failure mode --
   aborts the subtest immediately. Previously the assertion logged a
   length mismatch and then `assertDataEqual` ran on differently-sized
   slices, producing a noisy byte-diff on top of the actual symptom.
   The redundant trailing `t.Fatalf` block becomes dead and is removed.

2. Broaden the SSE-KMS probe-skip heuristic. The probe previously
   produced the friendly "KMS provider not configured" message only
   for 5xx responses; KMS-misconfig surfaces also include 501
   NotImplemented, 4xx KMS.NotConfigured, and error messages
   containing "KMS.NotConfigured" / "NotImplemented" /
   "not configured". The behaviour change is purely cosmetic (the
   caller t.Skip's on any non-empty reason either way) but the new
   diagnostic is more useful in CI logs.

3. Add `t.Parallel()` at the mode and size-class levels of the matrix.
   Each (mode, size) writes an independent object key under the shared
   bucket, with no cross-talk, so parallel execution is safe. Local
   wall time on the full matrix dropped from ~2.0s to ~1.1s (~45%);
   the savings scale with chunk count and CI machine concurrency.

Verified locally against `weed mini` with s3-config-template.json:
  - go test ./weed/s3api/ -count=1                   PASS
  - TestSSERangeReadIntegration -v                   112 PASS, 0 SKIP
  - TestSSEMultipartUploadIntegration etc.           PASS
This commit is contained in:
Chris Lu
2026-04-26 15:18:08 -07:00
parent f083fa9fed
commit 058cbf2711
1 changed files with 38 additions and 18 deletions
Download Patch File Download Diff File Expand all files Collapse all files

56
test/s3/sse/s3_sse_range_coverage_test.go
View File

@@ -6,6 +6,7 @@ import (
"errors"
"fmt"
"io"
"strings"
"testing"
"github.com/aws/aws-sdk-go-v2/aws"
@@ -93,9 +94,16 @@ func TestSSERangeReadIntegration(t *testing.T) {
},
}
// Each (mode, size) pair uploads an independent object key under the
// shared bucket and exercises range reads against it. The four modes
// have no shared state (each one carries its own SSE-C key, KMS keyID,
// or none); within a mode each size class also writes a unique key.
// That makes both levels safe to t.Parallel(), which substantially cuts
// CI wall time on the matrix (~45MB of data per mode).
for _, mode := range modes {
mode := mode
t.Run(mode.name(), func(t *testing.T) {
t.Parallel()
if reason := mode.probe(t, ctx, client, bucketName); reason != "" {
t.Skipf("%s unsupported in this test environment: %s", mode.name(), reason)
}
@@ -103,6 +111,7 @@ func TestSSERangeReadIntegration(t *testing.T) {
for _, sz := range sizes {
sz := sz
t.Run(sz.name, func(t *testing.T) {
t.Parallel()
objectKey := fmt.Sprintf("%s/%s", mode.name(), sz.name)
var data []byte
if len(sz.multipartParts) > 0 {
@@ -351,13 +360,28 @@ func (m *rangeModeSSEKMS) probe(t *testing.T, ctx context.Context, client *s3.Cl
SSEKMSKeyId: aws.String(m.keyID),
})
if err != nil {
// Treat any 5xx / "InternalError" / connection-level failure as
// "no KMS configured here". The test environments where SSE-KMS is
// expected to work (Makefile target `test-with-kms`) configure a
// provider; default `weed mini` does not.
// Treat any error from the probe as "no KMS configured here": this
// surface includes 5xx ("InternalError" / generic provider failure),
// 501 NotImplemented, 4xx KMS.NotConfigured, and any error message
// that mentions KMS not being configured. The test environments
// where SSE-KMS is expected to work (the Makefile's
// `start-seaweedfs-ci` target via s3-config-template.json, or
// `test-with-kms` via OpenBao) configure a provider; ad-hoc local
// `weed mini` setups may not. Either way, the caller t.Skips the
// SSE-KMS subtree -- the friendlier "not configured" framing is
// just for diagnostics in CI logs.
var apiErr *smithyhttp.ResponseError
if errors.As(err, &apiErr) && apiErr.HTTPStatusCode() >= 500 {
return fmt.Sprintf("KMS provider not configured (PutObject returned %d)", apiErr.HTTPStatusCode())
if errors.As(err, &apiErr) {
code := apiErr.HTTPStatusCode()
if code >= 500 || code == 501 || code == 400 {
return fmt.Sprintf("KMS provider not configured (PutObject returned %d)", code)
}
}
errMsg := err.Error()
if strings.Contains(errMsg, "KMS.NotConfigured") ||
strings.Contains(errMsg, "NotImplemented") ||
strings.Contains(errMsg, "not configured") {
return fmt.Sprintf("KMS provider not configured: %v", err)
}
return fmt.Sprintf("KMS PutObject probe failed: %v", err)
}
@@ -519,8 +543,14 @@ func verifyRangeRead(t *testing.T, ctx context.Context, client *s3.Client, mode
expected := source[rc.start : rc.end+1]
expectedLen := rc.end - rc.start + 1
assert.Equal(t, len(expected), len(got),
"body length mismatch for %s range=%s (source size=%d)", key, rc.rangeHeader, totalLen)
// Body-length check is `require` rather than `assert` because the bug
// fixed in #8908 surfaces as a fully-readable body that is shorter than
// requested -- a "truncation regression". Comparing different-length
// slices with assertDataEqual below would just produce a noisy byte-diff
// on top of the underlying truncation; bailing here keeps the failure
// log focused on the symptom that actually matters.
require.Equal(t, len(expected), len(got),
"body length mismatch for %s range=%s (source size=%d) — likely truncation regression", key, rc.rangeHeader, totalLen)
assert.Equal(t, expectedLen, aws.ToInt64(resp.ContentLength),
"Content-Length header mismatch for %s range=%s", key, rc.rangeHeader)
@@ -534,14 +564,4 @@ func verifyRangeRead(t *testing.T, ctx context.Context, client *s3.Client, mode
assertDataEqual(t, expected, got, "Range body mismatch for %s range=%s", key, rc.rangeHeader)
mode.verifyGet(t, resp)
// Sanity: never accidentally truncate. The bug fixed in #8908 shows up
// as a fully-readable body that's shorter than expected -- so an
// explicit "did we get fewer bytes than asked?" check, even though the
// body-equal assertion above subsumes it, makes regressions far easier
// to recognize in CI logs than a long byte-diff.
if int64(len(got)) != expectedLen {
t.Fatalf("range %s of %s returned %d bytes, expected %d (truncation regression)",
rc.rangeHeader, key, len(got), expectedLen)
}
}