Delete schema fails in mirror tests

[fogelito]

Seems like MirrorTest returning false on $database->exists(FUNCTION)

So $database->delete(FUNCTION);
does not happen

Summary by CodeRabbit

• Tests
  • Improved end-to-end test robustness for shared-tables and tenant-per-document scenarios by centralizing and simplifying cleanup logic, reducing flakiness from race conditions and backend state mismatches.
  • Replaced scattered cleanup steps with a consolidated approach and explicit test database targeting to make CI results more reliable.
  • No functional or UI changes; test-only stability improvements.

[coderabbitai]

Walkthrough

Updated two end-to-end tests: one replaced per-call existence check with a direct delete wrapped in try/catch and added a local type annotation; the other replaced repeated per-schema delete blocks with a loop over a schema list to perform cleanup. No public APIs changed.

Changes

Cohort / File(s)	Summary of Changes
GeneralTests — single-test cleanup & annotation tests/e2e/Adapter/Scopes/GeneralTests.php	Replaced conditional exists-then-delete with a direct delete wrapped in try { ... } catch (\Throwable $e) {} to ignore deletion errors; added a local /** @var Database $database */ annotation and minor formatting tweaks.
MirrorTest — loop-driven schema cleanup tests/e2e/Adapter/MirrorTest.php	Replaced multiple explicit per-schema existence checks and deletes with an array of schema names (utopiaTests, schema1, schema2, sharedTables, sharedTablesTenantPerDocument) and a loop that deletes matching source/destination schemas if present; removed the now-redundant post-database-init exists-delete block.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant T as GeneralTests (test)
  participant DB as Database
  rect rgba(220,235,255,0.4)
    note over T: Pre-test cleanup using direct delete
    T->>DB: delete('sharedTablesTenantPerDocument')
    alt delete succeeds
      DB-->>T: success
    else delete throws
      DB-->>T: Throwable
      note over T: Exception caught and ignored
    end
  end
  note over T: Continue with test setup

Loading

sequenceDiagram
  autonumber
  participant T as MirrorTest (test)
  participant DB as Database
  rect rgba(235,245,220,0.4)
    note over T: Loop-driven cleanup for multiple schemas
    T->>T: schemas = [utopiaTests, schema1, schema2, sharedTables, sharedTablesTenantPerDocument]
    loop for each schema in schemas
      T->>DB: if schema exists in source -> delete
      DB-->>T: success/throw
      T->>DB: if schema exists in destination -> delete
      DB-->>T: success/throw
    end
  end
  note over T: Proceed with DB init/creation

Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested reviewers

• abnegate

Poem

I nibble at schemas, one by one,
Looping through fields until the job is done.
If delete trips, I catch and grin,
A typed carrot tucked within.
Hops and tests — a tidy run. 🥕🐇

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The pull request title “Delete schema fails in mirror tests” clearly and concisely summarizes the main issue addressed by the changes, aligning with the PR objective of fixing the schema deletion problem encountered in MirrorTest. It highlights the specific failure context (mirror tests) without including extraneous details or vague terms. The phrasing is specific enough to inform a reviewer of the primary change and is properly formatted as a single sentence.

✨ Finishing touches

• 📝 Generate Docstrings

🧪 Generate unit tests

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch delete-schema-fails

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

🧪 Early access (Sonnet 4.5): enabled

We are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience.

Note:

• Public repositories are always opted into early access features.
• You can enable or disable early access features from the CodeRabbit UI or by updating the CodeRabbit configuration file.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

tests/e2e/Adapter/Scopes/GeneralTests.php (1)

440-447: Narrow catch to expected exceptions in test cleanup.

Replace the empty catch (\Throwable) with a union of the actual exceptions thrown and document the expected “not found” scenario:

         try {
             /**
-             * MirrorTest returning false $database->exists(__FUNCTION__)
+             * Test cleanup: MirrorTest may misreport `exists()`, so we attempt delete and ignore expected failures.
              */
             $database->delete(__FUNCTION__);
-        } catch (\Throwable $e) {
+        } catch (\PDOException|\Utopia\Database\Exception $e) {
+            // Ignore: database may not exist or cleanup can fail in MirrorTest
         }

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 19a3ab2 and c9e2e15.

📒 Files selected for processing (1)

• tests/e2e/Adapter/Scopes/GeneralTests.php (2 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

tests/e2e/Adapter/Scopes/GeneralTests.php (5)

src/Database/Adapter.php (2)

• getDatabase (160-163)
• delete (515-515)

src/Database/Database.php (2)

• getDatabase (779-782)
• delete (1301-1315)

tests/e2e/Adapter/Base.php (1)

• getDatabase (36-36)

src/Database/Adapter/Pool.php (1)

• delete (148-151)

src/Database/Mirror.php (1)

• delete (192-195)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: Setup & Build Docker Image

🔇 Additional comments (1)

tests/e2e/Adapter/Scopes/GeneralTests.php (1)

427-429: LGTM: Type annotation follows existing pattern.

The PHPDoc type annotation is consistent with other test methods in this file and improves IDE support.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

tests/e2e/Adapter/Scopes/GeneralTests.php (1)

427-428: Type annotation aids code clarity.

The added PHPDoc type annotation makes the database type explicit, which is helpful given the complex setup. This is a minor improvement to code readability.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c9e2e15 and 9cc17c3.

📒 Files selected for processing (2)

• tests/e2e/Adapter/MirrorTest.php (1 hunks)
• tests/e2e/Adapter/Scopes/GeneralTests.php (2 hunks)

🧰 Additional context used

🧬 Code graph analysis (2)

tests/e2e/Adapter/MirrorTest.php (6)

src/Database/Mirror.php (5)

• getSource (56-59)
• exists (182-185)
• setDatabase (105-110)
• delete (192-195)
• getDestination (61-64)

src/Database/Adapter/SQLite.php (2)

• exists (73-103)
• delete (126-129)

src/Database/Adapter/SQL.php (1)

• exists (167-209)

src/Database/Adapter.php (3)

• exists (499-499)
• setDatabase (145-150)
• delete (515-515)

src/Database/Database.php (3)

• exists (1273-1278)
• setDatabase (764-769)
• delete (1301-1315)

src/Database/Adapter/MariaDB.php (1)

• delete (53-64)

tests/e2e/Adapter/Scopes/GeneralTests.php (7)

tests/e2e/Adapter/MirrorTest.php (1)

• getDatabase (36-101)

src/Database/Adapter.php (4)

• getDatabase (160-163)
• exists (499-499)
• delete (515-515)
• setDatabase (145-150)

src/Database/Database.php (4)

• getDatabase (779-782)
• exists (1273-1278)
• delete (1301-1315)
• setDatabase (764-769)

tests/e2e/Adapter/Base.php (1)

• getDatabase (36-36)

src/Database/Adapter/SQL.php (1)

• exists (167-209)

src/Database/Adapter/Pool.php (2)

• exists (138-141)
• delete (148-151)

src/Database/Mirror.php (3)

• exists (182-185)
• delete (192-195)
• setDatabase (105-110)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)

• GitHub Check: Adapter Tests (SharedTables/SQLite)
• GitHub Check: Adapter Tests (Pool)
• GitHub Check: Adapter Tests (Mirror)
• GitHub Check: Adapter Tests (SharedTables/MariaDB)
• GitHub Check: Adapter Tests (SharedTables/MySQL)
• GitHub Check: Adapter Tests (SharedTables/Postgres)
• GitHub Check: Adapter Tests (MySQL)
• GitHub Check: Adapter Tests (Postgres)
• GitHub Check: Adapter Tests (SQLite)
• GitHub Check: Adapter Tests (MariaDB)

🔇 Additional comments (2)

tests/e2e/Adapter/Scopes/GeneralTests.php (1)

440-445: Corrected schema name from function name to literal.

The change from __FUNCTION__ to the literal 'sharedTablesTenantPerDocument' is correct. The test function is named testSharedTablesTenantPerDocument, so __FUNCTION__ would resolve to 'testSharedTablesTenantPerDocument' (with the "test" prefix), but the actual schema is 'sharedTablesTenantPerDocument' (without "test"). This mismatch was likely causing the exists() check to fail, preventing cleanup.

This aligns with the schema list in MirrorTest.php (line 79) which also uses 'sharedTablesTenantPerDocument'.

tests/e2e/Adapter/MirrorTest.php (1)

74-92: Unify delete() invocation pattern across tests. Tests currently use two styles to drop schemas—calling $database->delete('schemaName') and chaining setDatabase('schemaName')->delete(). Consolidate on a single approach (e.g. always use setDatabase()->delete() for clarity) and update all occurrences accordingly.