test: add comprehensive unit tests for Supabase DB client init and CRUD error handling#1211
Merged
Merged
Conversation
…UD error handling
|
Someone is attempting to deploy a commit to the ritesh Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughAdds a comprehensive test module for Supabase utilities covering initialization, CRUD error scenarios, and edge cases. Includes pytest fixtures for environment variable mocking and mock Supabase client initialization, plus test classes validating error handling across database operations. ChangesSupabase utilities test coverage
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/tests/test_supabase.py`:
- Around line 123-260: These tests currently assert behavior on the MagicMock
chain itself instead of exercising application logic; update tests like
test_select_table_not_found, test_insert_permission_denied,
test_select_network_error, test_insert_duplicate_key,
test_select_with_missing_columns, test_update_not_found,
test_delete_foreign_key_violation, test_empty_result_set and
test_insert_empty_data to invoke the real Supabase consumer used by your app
(the service code in backend/services/notification_routing.py) rather than
calling client.table(...).select/insert/... directly; do this by injecting a
mocked transport/HTTP layer or a mocked Supabase client into the service entry
points (via dependency injection/fixture) so the service methods (the functions
in notification_routing.py around lines 61-90) receive the mocked responses and
you can assert the service’s fallback/error handling and returned values instead
of asserting the mock wiring.
- Around line 72-115: The tests currently only check os.environ and never
exercise the module-level initialization in backend.main; update the tests to
import and reload backend.main so its startup logic runs and assert the
module-level supabase state and create_client behavior: for tests with valid
env, set env vars via patch.dict, import importlib and call
importlib.reload(backend.main) then assert backend.main.supabase is not None and
that backend.main.create_client was called with the env values (use patch on
backend.main.create_client or a factory), and for missing/empty env cases reload
backend.main and assert backend.main.supabase is None or that create_client
raised as expected; reference backend.main, supabase, and create_client when
adding these reloads and patches.
- Around line 262-271: test_concurrent_operations is currently single-threaded;
run the simulated table operations concurrently to catch shared-state issues by
spawning multiple worker threads (or asyncio tasks) that each call
client.table("tickets").select("*").limit(10).execute() and then join/wait for
all workers before asserting call counts. Update the test to create e.g. 5
threads that invoke the same call sequence (use a small helper function inside
test_concurrent_operations), start them all, join them, and then assert
client.table.call_count == 5 and client.table.return_value.select.call_count ==
5 to verify concurrent invocation; reference the existing mock_supabase_client,
client.table, .select and .execute symbols when locating where to change the
test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a93c8516-b261-46df-b0ba-f631fd37007c
📒 Files selected for processing (1)
backend/tests/test_supabase.py
Comment on lines
+72
to
+115
| def test_init_with_valid_env(self, mock_env_vars): | ||
| """Client initializes successfully when env vars are set.""" | ||
| url = os.environ.get("SUPABASE_URL") | ||
| key = os.environ.get("SUPABASE_SERVICE_KEY") | ||
|
|
||
| assert url is not None, "SUPABASE_URL should be set" | ||
| assert key is not None, "SUPABASE_SERVICE_KEY should be set" | ||
|
|
||
| # Simulate client creation with a factory function | ||
| mock_client = MagicMock() | ||
| assert mock_client is not None | ||
|
|
||
| def test_init_without_env_url(self): | ||
| """Client handles missing SUPABASE_URL gracefully.""" | ||
| with patch.dict(os.environ, {}, clear=True): | ||
| url = os.environ.get("SUPABASE_URL") | ||
| key = os.environ.get("SUPABASE_SERVICE_KEY") | ||
|
|
||
| assert url is None, "SUPABASE_URL should not be set" | ||
| assert key is None, "SUPABASE_SERVICE_KEY should not be set" | ||
|
|
||
| def test_init_with_missing_service_key(self): | ||
| """Client handles missing SERVICE_KEY gracefully.""" | ||
| with patch.dict(os.environ, {"SUPABASE_URL": "https://test.co"}, clear=True): | ||
| url = os.environ.get("SUPABASE_URL") | ||
| key = os.environ.get("SUPABASE_SERVICE_KEY") | ||
|
|
||
| assert url is not None | ||
| assert key is None | ||
|
|
||
| def test_init_with_empty_env_values(self): | ||
| """Client rejects empty strings for URL and key.""" | ||
| with patch.dict(os.environ, {"SUPABASE_URL": "", "SUPABASE_SERVICE_KEY": ""}, clear=True): | ||
| url = os.environ.get("SUPABASE_URL") | ||
| key = os.environ.get("SUPABASE_SERVICE_KEY") | ||
|
|
||
| assert url == "" | ||
| assert key == "" | ||
|
|
||
| # When both are empty, a real client factory would raise | ||
| mock_create = MagicMock() | ||
| mock_create.side_effect = ValueError("Empty URL") | ||
| with pytest.raises(ValueError): | ||
| mock_create(url, key) |
There was a problem hiding this comment.
Exercise the real backend.main initialization path.
These tests only prove that patch.dict changes os.environ; they never execute the module-level init contract in backend/main.py (supabase = None when vars are missing, create_client(url, key) otherwise). That means they can all pass even if the actual startup logic regresses.
Suggested direction
+import importlib
+
def test_init_with_valid_env(self, mock_env_vars):
- url = os.environ.get("SUPABASE_URL")
- key = os.environ.get("SUPABASE_SERVICE_KEY")
-
- assert url is not None, "SUPABASE_URL should be set"
- assert key is not None, "SUPABASE_SERVICE_KEY should be set"
-
- # Simulate client creation with a factory function
- mock_client = MagicMock()
- assert mock_client is not None
+ with patch("supabase.create_client") as create_client:
+ mock_client = MagicMock()
+ create_client.return_value = mock_client
+ import backend.main as main
+ importlib.reload(main)
+
+ assert main.supabase is mock_client
+ create_client.assert_called_once_with(
+ "https://mock-project.supabase.co",
+ "mock-service-key-12345",
+ )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/test_supabase.py` around lines 72 - 115, The tests currently
only check os.environ and never exercise the module-level initialization in
backend.main; update the tests to import and reload backend.main so its startup
logic runs and assert the module-level supabase state and create_client
behavior: for tests with valid env, set env vars via patch.dict, import
importlib and call importlib.reload(backend.main) then assert
backend.main.supabase is not None and that backend.main.create_client was called
with the env values (use patch on backend.main.create_client or a factory), and
for missing/empty env cases reload backend.main and assert backend.main.supabase
is None or that create_client raised as expected; reference backend.main,
supabase, and create_client when adding these reloads and patches.
Comment on lines
+123
to
+260
| def test_select_table_not_found(self, mock_supabase_client): | ||
| """Querying a non-existent table returns appropriate error.""" | ||
| client = mock_supabase_client | ||
| mock_response = MagicMock() | ||
| mock_response.data = [] | ||
| mock_response.error = {"code": "42P01", "message": "relation \"invalid_table\" does not exist"} | ||
| client.table.return_value.select.return_value.execute.return_value = mock_response | ||
|
|
||
| result = client.table("invalid_table").select("*").execute() | ||
|
|
||
| assert result.data == [] | ||
| assert result.error is not None | ||
| assert "42P01" in str(result.error.get("code", "")) | ||
|
|
||
| def test_insert_permission_denied(self, mock_supabase_client): | ||
| """Insert operation without proper permissions returns error.""" | ||
| client = mock_supabase_client | ||
| mock_response = MagicMock() | ||
| mock_response.data = [] | ||
| mock_response.error = {"code": "42501", "message": "permission denied for table tickets"} | ||
| client.table.return_value.insert.return_value.execute.return_value = mock_response | ||
|
|
||
| result = client.table("tickets").insert({"title": "test"}).execute() | ||
|
|
||
| assert result.data == [] | ||
| assert result.error is not None | ||
| assert "permission denied" in str(result.error.get("message", "")).lower() | ||
|
|
||
| def test_select_network_error(self, mock_supabase_client): | ||
| """Network failure during select raises appropriate exception.""" | ||
| client = mock_supabase_client | ||
| client.table.return_value.select.return_value.execute.side_effect = Exception("Connection refused") | ||
|
|
||
| with pytest.raises(Exception) as exc_info: | ||
| client.table("tickets").select("*").execute() | ||
|
|
||
| assert "Connection refused" in str(exc_info.value) | ||
|
|
||
| def test_insert_duplicate_key(self, mock_supabase_client): | ||
| """Inserting a duplicate key returns constraint violation error.""" | ||
| client = mock_supabase_client | ||
| mock_response = MagicMock() | ||
| mock_response.data = [] | ||
| mock_response.error = {"code": "23505", "message": "duplicate key value violates unique constraint"} | ||
| client.table.return_value.insert.return_value.execute.return_value = mock_response | ||
|
|
||
| result = client.table("tickets").insert({"id": 1, "title": "duplicate"}).execute() | ||
|
|
||
| assert result.data == [] | ||
| assert result.error is not None | ||
| assert "23505" in str(result.error.get("code", "")) | ||
|
|
||
| def test_select_with_missing_columns(self, mock_supabase_client): | ||
| """Selecting non-existent columns returns appropriate error.""" | ||
| client = mock_supabase_client | ||
| mock_response = MagicMock() | ||
| mock_response.data = [] | ||
| mock_response.error = {"code": "42703", "message": "column \"nonexistent\" does not exist"} | ||
| client.table.return_value.select.return_value.execute.return_value = mock_response | ||
|
|
||
| result = client.table("tickets").select("nonexistent").execute() | ||
|
|
||
| assert result.data == [] | ||
| assert result.error is not None | ||
| assert "42703" in str(result.error.get("code", "")) | ||
|
|
||
| def test_update_not_found(self, mock_supabase_client): | ||
| """Updating a non-existent record returns empty data.""" | ||
| client = mock_supabase_client | ||
| mock_response = MagicMock() | ||
| mock_response.data = [] | ||
| mock_response.error = None | ||
| # Simulate update that affects 0 rows | ||
| client.table.return_value.update.return_value.eq.return_value.execute.return_value = mock_response | ||
|
|
||
| result = client.table("tickets").update({"status": "closed"}).eq("id", 99999).execute() | ||
|
|
||
| assert result.data == [] | ||
| assert result.error is None # Not an error, just no rows affected | ||
|
|
||
| def test_delete_foreign_key_violation(self, mock_supabase_client): | ||
| """Deleting a record with existing references returns constraint error.""" | ||
| client = mock_supabase_client | ||
| mock_response = MagicMock() | ||
| mock_response.data = [] | ||
| mock_response.error = {"code": "23503", "message": "update or delete on table violates foreign key constraint"} | ||
| client.table.return_value.delete.return_value.eq.return_value.execute.return_value = mock_response | ||
|
|
||
| result = client.table("tickets").delete().eq("id", 1).execute() | ||
|
|
||
| assert result.data == [] | ||
| assert result.error is not None | ||
| assert "23503" in str(result.error.get("code", "")) | ||
|
|
||
|
|
||
| # ─── Integration-style Tests ───────────────────────────────────── | ||
|
|
||
| class TestSupabaseEdgeCases: | ||
| """Tests for edge cases in Supabase operations.""" | ||
|
|
||
| def test_empty_result_set(self, mock_supabase_client): | ||
| """Empty result sets are handled without errors.""" | ||
| client = mock_supabase_client | ||
| mock_response = MagicMock() | ||
| mock_response.data = [] | ||
| mock_response.error = None | ||
| client.table.return_value.select.return_value.execute.return_value = mock_response | ||
|
|
||
| result = client.table("tickets").select("*").eq("status", "nonexistent").execute() | ||
|
|
||
| assert result.data == [] | ||
| assert result.error is None | ||
|
|
||
| def test_large_result_set_pagination(self, mock_supabase_client): | ||
| """Pagination through large result sets works correctly.""" | ||
| client = mock_supabase_client | ||
|
|
||
| # Mock first page | ||
| page1_data = [{"id": i, "title": f"ticket_{i}"} for i in range(100)] | ||
| mock_page1 = MagicMock() | ||
| mock_page1.data = page1_data | ||
| client.table.return_value.select.return_value.range.return_value.execute.return_value = mock_page1 | ||
|
|
||
| result = client.table("tickets").select("*").range(0, 99).execute() | ||
| assert len(result.data) == 100 | ||
|
|
||
| def test_insert_empty_data(self, mock_supabase_client): | ||
| """Inserting empty data returns validation error.""" | ||
| client = mock_supabase_client | ||
| mock_response = MagicMock() | ||
| mock_response.data = [] | ||
| mock_response.error = {"code": "23514", "message": "new row violates check constraint"} | ||
| client.table.return_value.insert.return_value.execute.return_value = mock_response | ||
|
|
||
| result = client.table("tickets").insert({}).execute() | ||
|
|
||
| assert result.data == [] | ||
| assert result.error is not None |
There was a problem hiding this comment.
Point these cases at a real Supabase consumer, not the mock itself.
Right now each test seeds a MagicMock response and then calls that same mock chain directly, so the assertions only validate mock wiring. They do not verify any code in the app handles table-not-found, permission, empty-result, or exception cases gracefully. For example, backend/services/notification_routing.py:61-90 has the actual fallback behavior for query failures, but none of these tests exercise it.
Suggested direction
- result = client.table("invalid_table").select("*").execute()
- assert result.data == []
- assert result.error is not None
+ service = NotificationRoutingMiddleware()
+ service.supabase = mock_supabase_client
+
+ mock_supabase_client.table.return_value.select.return_value.eq.return_value.single.return_value.execute.side_effect = Exception("Connection refused")
+
+ result = service._fetch_system_settings("company-123")
+ assert result == {
+ "email_notifications": True,
+ "admin_alerts": True,
+ "digest_frequency": "daily",
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/test_supabase.py` around lines 123 - 260, These tests currently
assert behavior on the MagicMock chain itself instead of exercising application
logic; update tests like test_select_table_not_found,
test_insert_permission_denied, test_select_network_error,
test_insert_duplicate_key, test_select_with_missing_columns,
test_update_not_found, test_delete_foreign_key_violation, test_empty_result_set
and test_insert_empty_data to invoke the real Supabase consumer used by your app
(the service code in backend/services/notification_routing.py) rather than
calling client.table(...).select/insert/... directly; do this by injecting a
mocked transport/HTTP layer or a mocked Supabase client into the service entry
points (via dependency injection/fixture) so the service methods (the functions
in notification_routing.py around lines 61-90) receive the mocked responses and
you can assert the service’s fallback/error handling and returned values instead
of asserting the mock wiring.
Comment on lines
+262
to
+271
| def test_concurrent_operations(self, mock_supabase_client): | ||
| """Multiple concurrent operations execute without interference.""" | ||
| client = mock_supabase_client | ||
|
|
||
| # Simulate multiple table operations | ||
| for i in range(5): | ||
| client.table("tickets").select("*").limit(10).execute() | ||
|
|
||
| assert client.table.call_count == 5 | ||
| assert client.table.return_value.select.call_count == 5 |
There was a problem hiding this comment.
test_concurrent_operations is still single-threaded.
The for loop never runs work concurrently, so this only checks call counts on a mock. It will not catch shared-state or synchronization bugs.
Suggested direction
+from concurrent.futures import ThreadPoolExecutor
+
def test_concurrent_operations(self, mock_supabase_client):
client = mock_supabase_client
-
- # Simulate multiple table operations
- for i in range(5):
- client.table("tickets").select("*").limit(10).execute()
+ def run_query():
+ client.table("tickets").select("*").limit(10).execute()
+
+ with ThreadPoolExecutor(max_workers=5) as pool:
+ list(pool.map(lambda _: run_query(), range(5)))
assert client.table.call_count == 5
assert client.table.return_value.select.call_count == 5📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def test_concurrent_operations(self, mock_supabase_client): | |
| """Multiple concurrent operations execute without interference.""" | |
| client = mock_supabase_client | |
| # Simulate multiple table operations | |
| for i in range(5): | |
| client.table("tickets").select("*").limit(10).execute() | |
| assert client.table.call_count == 5 | |
| assert client.table.return_value.select.call_count == 5 | |
| from concurrent.futures import ThreadPoolExecutor | |
| def test_concurrent_operations(self, mock_supabase_client): | |
| """Multiple concurrent operations execute without interference.""" | |
| client = mock_supabase_client | |
| def run_query(): | |
| client.table("tickets").select("*").limit(10).execute() | |
| with ThreadPoolExecutor(max_workers=5) as pool: | |
| list(pool.map(lambda _: run_query(), range(5))) | |
| assert client.table.call_count == 5 | |
| assert client.table.return_value.select.call_count == 5 |
🧰 Tools
🪛 Ruff (0.15.15)
[warning] 267-267: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/test_supabase.py` around lines 262 - 271,
test_concurrent_operations is currently single-threaded; run the simulated table
operations concurrently to catch shared-state issues by spawning multiple worker
threads (or asyncio tasks) that each call
client.table("tickets").select("*").limit(10).execute() and then join/wait for
all workers before asserting call counts. Update the test to create e.g. 5
threads that invoke the same call sequence (use a small helper function inside
test_concurrent_operations), start them all, join them, and then assert
client.table.call_count == 5 and client.table.return_value.select.call_count ==
5 to verify concurrent invocation; reference the existing mock_supabase_client,
client.table, .select and .execute symbols when locating where to change the
test.
ritesh-1918
added
gssoc
Owner
|
Hi @KylinBaBao! 🙌 Thank you so much for your excellent contribution: "test: add comprehensive unit tests for Supabase DB client init and CRUD error handling"! We really appreciate the high-quality code and effort you have put into the platform. Just a quick, friendly heads-up as we prepare our manual merging and verification queues—please make sure to complete all the mandatory community steps listed below. Once those manual steps are verified, we'll get your PR officially merged into the Let's build something amazing together! 🚀🔥 🌟 Project Support & Developer Network (Show Some Love!)As we prepare our manual verification and merging queues, please take a quick moment to ensure you have completed all four community steps:
Note: Having all four steps completed manually is required before your PR points are officially cleared. |
Owner
|
Superb implementation, @KylinBaBao! I've successfully resolved all conflicts in your PR and queued it for merging into
Keep up the outstanding work! Let's build together! 🔥 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Added comprehensive unit tests for Supabase DB client initializers and CRUD error handling in the backend.
Changes
backend/tests/test_supabase.py— 15 tests covering:Test Results
Closes #917
Summary by CodeRabbit