Fix brittle tests, move delete batching into client package, run s3 tests against real bucket

attachment/backend_s3.go

@@ -8,8 +8,6 @@ import (
 	"heckel.io/ntfy/v2/s3"
 )
 
-const deleteBatchSize = 1000
-
 type s3Backend struct {
 	client *s3.Client
 }
@@ -45,17 +43,7 @@ func (b *s3Backend) List() ([]object, error) {
 }
 
 func (b *s3Backend) Delete(ids ...string) error {
-	// S3 DeleteObjects supports up to 1000 keys per call
-	for i := 0; i < len(ids); i += deleteBatchSize {
-		end := i + deleteBatchSize
-		if end > len(ids) {
-			end = len(ids)
-		}
-		if err := b.client.DeleteObjects(context.Background(), ids[i:end]); err != nil {
-			return err
-		}
-	}
-	return nil
+	return b.client.DeleteObjects(context.Background(), ids)
 }
 
 func (b *s3Backend) DeleteIncomplete(cutoff time.Time) error {

attachment/store_s3_test.go

@@ -14,20 +14,20 @@ import (
 )
 
 func TestS3Store_WriteWithPrefix(t *testing.T) {
-	s3URL := os.Getenv("NTFY_TEST_ATTACHMENT_S3_URL")
+	s3URL := os.Getenv("NTFY_TEST_S3_URL")
 	if s3URL == "" {
-		t.Skip("NTFY_TEST_ATTACHMENT_S3_URL not set")
+		t.Skip("NTFY_TEST_S3_URL not set")
 	}
 	cfg, err := s3.ParseURL(s3URL)
 	require.Nil(t, err)
 	cfg.Prefix = "test-prefix"
 	client := s3.New(cfg)
-	deleteAllObjects(client)
+	deleteAllObjects(t, client)
 	backend := newS3Backend(client)
 	cache, err := newStore(backend, 10*1024, nil)
 	require.Nil(t, err)
 	t.Cleanup(func() {
-		deleteAllObjects(client)
+		deleteAllObjects(t, client)
 		cache.Close()
 	})
 
@@ -47,34 +47,46 @@ func TestS3Store_WriteWithPrefix(t *testing.T) {
 
 func newTestRealS3Store(t *testing.T, totalSizeLimit int64) (*Store, *modTimeOverrideBackend) {
 	t.Helper()
-	s3URL := os.Getenv("NTFY_TEST_ATTACHMENT_S3_URL")
+	s3URL := os.Getenv("NTFY_TEST_S3_URL")
 	if s3URL == "" {
-		t.Skip("NTFY_TEST_ATTACHMENT_S3_URL not set")
+		t.Skip("NTFY_TEST_S3_URL not set")
 	}
 	cfg, err := s3.ParseURL(s3URL)
 	require.Nil(t, err)
+	if cfg.Prefix != "" {
+		cfg.Prefix = cfg.Prefix + "/testpkg-attachment"
+	} else {
+		cfg.Prefix = "testpkg-attachment"
+	}
 	client := s3.New(cfg)
 	inner := newS3Backend(client)
 	wrapper := &modTimeOverrideBackend{backend: inner, modTimes: make(map[string]time.Time)}
-	deleteAllObjects(client)
+	deleteAllObjects(t, client)
 	store, err := newStore(wrapper, totalSizeLimit, nil)
 	require.Nil(t, err)
 	t.Cleanup(func() {
-		deleteAllObjects(client)
+		deleteAllObjects(t, client)
 		store.Close()
 	})
 	return store, wrapper
 }
 
-func deleteAllObjects(client *s3.Client) {
-	objects, _ := client.ListObjectsV2(context.Background())
-	keys := make([]string, 0, len(objects))
-	for _, obj := range objects {
-		keys = append(keys, obj.Key)
-	}
-	if len(keys) > 0 {
-		client.DeleteObjects(context.Background(), keys) //nolint:errcheck
+func deleteAllObjects(t *testing.T, client *s3.Client) {
+	t.Helper()
+	for i := 0; i < 20; i++ {
+		objects, err := client.ListObjectsV2(context.Background())
+		require.Nil(t, err)
+		if len(objects) == 0 {
+			return
+		}
+		keys := make([]string, len(objects))
+		for j, obj := range objects {
+			keys[j] = obj.Key
+		}
+		require.Nil(t, client.DeleteObjects(context.Background(), keys))
+		time.Sleep(200 * time.Millisecond)
 	}
+	t.Fatal("timed out waiting for bucket to be empty")
 }
 
 // modTimeOverrideBackend wraps a backend and allows overriding LastModified times returned by List().

attachment/store_test.go

@@ -332,7 +332,7 @@ func TestStore_Sync_SkipsRecentFiles(t *testing.T) {
 // callback that makes a specific object's timestamp old enough for orphan cleanup (> 1 hour).
 // For the file backend, this uses os.Chtimes; for the S3 backend, it overrides the object's
 // LastModified time via a modTimeOverrideBackend wrapper. Objects start with recent timestamps
-// by default. The S3 subtest is skipped if NTFY_TEST_ATTACHMENT_S3_URL is not set.
+// by default. The S3 subtest is skipped if NTFY_TEST_S3_URL is not set.
 func forEachBackend(t *testing.T, totalSizeLimit int64, f func(t *testing.T, s *Store, makeOld func(string))) {
 	t.Run("file", func(t *testing.T) {
 		dir, s := newTestFileStore(t, totalSizeLimit)