internal/storage/postgres/gc/gc_test.go (3)

388-413: Test name doesn't match test behavior.

The test is named "should continue processing other tenants when one tenant fails" but it actually tests a scenario where both tenants are processed successfully - tenant1 has data and tenant2 has no data. Neither tenant "fails" in this test.

Consider renaming to better reflect the actual test behavior:

-		It("should continue processing other tenants when one tenant fails", func() {
-			// Create two tenants
-			tenant1 := "gc-error-tenant-1"
-			tenant2 := "gc-error-tenant-2"
+		It("should handle tenants with varying data states", func() {
+			// Create two tenants, one with data and one without
+			tenant1 := "gc-data-tenant-1"
+			tenant2 := "gc-nodata-tenant-2"

---

447-484: Test name doesn't match test behavior.

Similar to the earlier test, this is named "should handle multiple tenants with mixed success and failure" but all tenants are processed successfully. Tenant3 simply has no data, which is not a failure scenario.

Consider renaming:

-		It("should handle multiple tenants with mixed success and failure", func() {
+		It("should handle multiple tenants with varying data states", func() {

---

633-634: Consider a stricter assertion.

The assertion transactionCountAfter <= transactionCountBefore is weak because it would pass even if no transactions were deleted. Since this test specifically aims to verify that deleteTransactionsForTenant deletes old transactions, consider using a stricter assertion:

-			// After GC, old transactions should be deleted, so count should be less
-			Expect(transactionCountAfter).Should(BeNumerically("<=", transactionCountBefore))
+			// After GC, old transactions should be deleted, so count should be strictly less
+			Expect(transactionCountAfter).Should(BeNumerically("<", transactionCountBefore))

Alternatively, if there's a possibility that no transactions qualify for deletion in some runs, the current assertion is acceptable but the comment should reflect that.

internal/storage/postgres/tenant_writer_test.go (1)

271-385: Tests don't exercise distinct error paths.

All six error handling tests (lines 272-385) follow the same pattern: close the database, then call DeleteTenant. Since the database is closed, all tests will fail at the same point (transaction begin), meaning the specific error paths mentioned in test names (batch execution error, commit error, query row error, batch close error, final commit error) are not actually being exercised.

These tests provide basic coverage that errors are handled, but consider consolidating into a single test or documenting that these represent coverage for the general error handling path rather than specific error stages.

internal/storage/postgres/tenant_writer.go (1)

78-79: Comment is slightly misleading.

The comment says "Use parameterized query with constant table name for safety" but table names cannot be parameterized in SQL - they're concatenated. The $1 parameter is for the tenantID value. Consider clarifying:

-	// Use parameterized query with constant table name for safety
+	// Table name is a constant; tenant ID is parameterized to prevent SQL injection

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

internal/storage/postgres/tenant_writer_test.go (3)

62-133: CreateTenant edge‑case coverage looks solid; minor timestamp assertion polish possible

The new tests around empty/special/unicode/long names and CreatedAt behavior provide good functional coverage and catch likely regressions. For the timestamp test, you could optionally simplify/readabilize the window check using Gomega’s temporal matchers (e.g. BeTemporally(">=", beforeCreate) / BeTemporally("<=", afterCreate)) instead of manual Add(±1 * time.Second), but the current approach is correct and reasonably robust.

---

150-246: DeleteTenant behavior tests are comprehensive; prefer enum constants over raw error strings

The new tests nicely exercise:

• not‑found behavior (non‑existent ID and empty ID),
• deletion with associated schema data and relation tuples,
• recreation after deletion, and
• double‑delete semantics.

One small consistency issue: the not‑found expectations use hard‑coded strings ("ERROR_CODE_NOT_FOUND"). Elsewhere in this file you assert against base.ErrorCode_... .String(). To keep things type‑safe and resilient to renames, consider switching to:

-Expect(err.Error()).Should(Equal("ERROR_CODE_NOT_FOUND"))
+Expect(err.Error()).Should(Equal(base.ErrorCode_ERROR_CODE_NOT_FOUND.String()))

for the not‑found cases (Lines 155, 237, 245).

---

291-471: Error‑handling tests are valuable; avoid hard‑coded PG version and minor redundancy

These error‑path tests (batch exec/close/commit/query‑row failures) add useful coverage by exercising DeleteTenant against closed DB instances. A couple of small improvements:

1. Avoid hard‑coding "14" in separate DBs

All the separate DB setups use:

separateDB := testinstance.PostgresDB("14")

while BeforeEach honors POSTGRES_VERSION with a default. If CI or local runs change the target PG version, these tests will still pin to 14 and can diverge in behavior. Consider factoring a helper mirroring the existing logic and reusing it:

+func postgresVersion() string {
+    v := os.Getenv("POSTGRES_VERSION")
+    if v == "" {
+        v = "14"
+    }
+    return v
+}

and in the tests:

-separateDB := testinstance.PostgresDB("14")
+separateDB := testinstance.PostgresDB(postgresVersion())

2. Remove redundant setup in the first batch‑exec test

In It("should handle batch execution error during table deletion", ...) you create a tenant via the main tenantWriter before creating and using the separate DB/tenant. That first creation on the primary db doesn’t influence the separate instance and can be safely removed to keep the test focused on the error path in the separate DB.

3. Brittle line‑number references in comments

Comments like “(line 101)” / “(line 111)” in this file will drift as tenant_writer.go evolves. Consider rephrasing them in terms of behavior (“the batch Exec() in the table‑deletion loop”, “the tenant‑deletion Exec path”) rather than specific line numbers.

All of these are cleanups; the current tests are functionally correct and provide good error‑path coverage.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

internal/storage/postgres/tenant_writer_test.go (5)

104-123: Remove redundant distinctness assertions.

Lines 119-122 are redundant. Since lines 109, 113, and 117 already assert that each tenant ID equals its unique expected value ("multi_tenant_1", "multi_tenant_2", "multi_tenant_3"), their distinctness is already proven.

 			Expect(tenant3.Id).Should(Equal("multi_tenant_3"))
-
-			// Verify all tenants are distinct
-			Expect(tenant1.Id).ShouldNot(Equal(tenant2.Id))
-			Expect(tenant2.Id).ShouldNot(Equal(tenant3.Id))
-			Expect(tenant1.Id).ShouldNot(Equal(tenant3.Id))
 		})

---

91-102: Consider using Gomega's BeTemporally matcher for cleaner timestamp assertions.

The current approach works, but Gomega provides a more expressive matcher for time comparisons.

 			Expect(err).ShouldNot(HaveOccurred())
 			Expect(tenant.CreatedAt).ShouldNot(BeNil())
-			Expect(tenant.CreatedAt.AsTime().After(beforeCreate.Add(-1 * time.Second))).Should(BeTrue())
-			Expect(tenant.CreatedAt.AsTime().Before(afterCreate.Add(1 * time.Second))).Should(BeTrue())
+			Expect(tenant.CreatedAt.AsTime()).Should(BeTemporally(">=", beforeCreate.Add(-1*time.Second)))
+			Expect(tenant.CreatedAt.AsTime()).Should(BeTemporally("<=", afterCreate.Add(1*time.Second)))

---

158-178: Consider verifying associated data is actually deleted.

This test confirms that DeleteTenant succeeds when schema data exists, but doesn't verify that the schema data was actually removed. Consider adding a verification step (e.g., attempting to read the schema after deletion) to ensure the cascade delete works correctly.

---

321-351: Consider removing hard-coded line number references in comments.

Comments like "This will test the br.Exec() error path in the loop (line 101)" reference specific line numbers in tenant_writer.go that may become stale as the implementation evolves. Consider describing the error path by its logical purpose instead.

-				// Close the database to trigger batch execution error
-				// This will test the br.Exec() error path in the loop (line 101)
+				// Close the database to trigger batch execution error
+				// This tests the batch execution error path during table deletion loop

---

381-417: Tests "commit error after batch execution" and "query row error" appear to test the same error path.

Both tests close the shared db instance and then call DeleteTenant. Since the database is already closed, both will fail at the same point (transaction begin), not at their intended error paths (commit or query row). These tests are effectively duplicates.

Consider either:

1. Removing one of these redundant tests, or
2. Using different mechanisms (e.g., mocks, fault injection) to trigger distinct error conditions at the specific paths.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

internal/storage/postgres/tenant_writer_test.go (4)

145-151: Prefer using the base error code constant instead of hard‑coded "ERROR_CODE_NOT_FOUND"

These tests currently assert on a raw string literal:

• Line 150: non‑existent tenant
• Line 270: second deletion
• Line 278: empty ID

To keep tests aligned with the proto definition and avoid drift if the string ever changes, you can assert using the enum’s .String() like you do for the unique‑constraint case:

-			Expect(err.Error()).Should(Equal("ERROR_CODE_NOT_FOUND"))
+			Expect(err.Error()).Should(Equal(base.ErrorCode_ERROR_CODE_NOT_FOUND.String()))

Apply the same change to all three occurrences.

Also applies to: 256-271, 273-279

---

183-231: Consider asserting DB cleanup for tuples/all‑data cases, similar to the schema test

For the “associated relation tuples” and “all associated data types” tests you only assert that DeleteTenant returns no error. A regression where some tables aren’t fully cleaned up but still return nil would slip through.

You already directly check the schema table count in the schema test; mirroring that pattern for tuples and other data (using the appropriate table constants) would tighten these tests and validate the full batch‑deletion behavior.

---

324-354: Remove redundant tenant creation against the main tenantWriter

In It("should handle batch execution error during table deletion") you:

• First create a tenant via the main tenantWriter (default DB).
• Then create a separate DB and a separate tenant there.
• Trigger the error path using writerWithClosedDB built from the separate DB.

The initial creation on the main DB isn’t used by this test and can be dropped to reduce noise and make it clearer which database you’re exercising:

-				// Create a tenant first
-				tenantID := "batch-exec-error-tenant"
-				_, err := tenantWriter.CreateTenant(ctx, tenantID, "Batch Exec Error Tenant")
-				Expect(err).ShouldNot(HaveOccurred())
-
-				// Create a separate database instance for this test
-				separateDB := testinstance.PostgresDB("14")
-				separateWriter := NewTenantWriter(separateDB.(*PQDatabase.Postgres))
-
-				// Create tenant in separate DB
-				_, err = separateWriter.CreateTenant(ctx, tenantID, "Batch Exec Error Tenant")
+				// Create a tenant only in the separate database whose deletion path we want to exercise
+				tenantID := "batch-exec-error-tenant"
+				separateDB := testinstance.PostgresDB("14")
+				separateWriter := NewTenantWriter(separateDB.(*PQDatabase.Postgres))
+
+				_, err := separateWriter.CreateTenant(ctx, tenantID, "Batch Exec Error Tenant")
 				Expect(err).ShouldNot(HaveOccurred())

---

287-301: Error‑handling tests are thorough; a couple of minor maintainability nits

The matrix of execution/tx/batch/connection/commit error paths is very well covered. Two small nits you might consider:

1. Avoid line‑number references in comments

   The commit‑error test comment currently mentions a specific source line:

   // This will test the tx.Commit() error path (line 123)

   This will go stale quickly. You can keep the intent without tying it to a line number:

•   		// This will test the tx.Commit() error path (line 123)
  

•   		// This will exercise the tx.Commit() error path in DeleteTenant
  

2. Reduce duplication in DB setup and error assertions (optional)

   • testinstance.PostgresDB("14") is repeated across many tests; if you ever change the default version logic, these can easily drift from the BeforeEach behavior. A small helper (e.g. newTestPostgresDB() that respects POSTGRES_VERSION with a default) would centralize that choice.
   • The Expect(err.Error()).Should(Or(Equal(...))) block is duplicated across all error‑handling tests; a helper like expectTenantWriterError(err) would make it easier to adjust the allowed codes in one place.

Given this is test code, these are purely cleanliness/DRY improvements, not correctness issues.

Also applies to: 308-322, 324-486

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro