Remove unused tests

[lourou]

Remove unused tests by deleting attachments API integration tests in tests/attachments.test.ts and the describe("PushNotificationService") FCM rejection test in tests/notifications-service.test.ts

Delete the attachments API integration test file and remove the FCM token rejection test from the push notification service suite, trimming unused test coverage and setup.

📍Where to Start

Start with the removed file attachments.test.ts to confirm full deletion, then review changes in notifications-service.test.ts within describe("PushNotificationService") to verify the specific test removal.

---

📊 Macroscope summarized 4771fed. 0 files reviewed, 0 issues evaluated, 0 issues filtered, 0 comments posted

🗂️ Filtered Issues

No issues evaluated.

Summary by CodeRabbit

• Tests
  • Removed integration test suites for attachments, authentication, devices, initialization, invites, notifications, profiles, and wallet APIs.

[claude]

Claude finished @lourou's task —— View job

---

Code Review Complete ✓

Tasks

• Read repository guidelines (CLAUDE.md)
• Examine modified file (notifications-service.test.ts)
• Understand context for test removal
• Analyze impact on test coverage
• Provide comprehensive review feedback

---

Review Summary

This PR removes 3,982 lines of test code across 11 deleted test files and 1 modified file. Based on the recent commit history, this appears to be cleanup following the "Backend v2 foundations" migration (PR #141).

Code Quality & Best Practices ✅

Positive aspects:

• Clean removal of deprecated V1 tests
• Retained notifications-service.test.ts which contains comprehensive unit tests for the push notification service
• The remaining test file maintains good test structure with proper setup/teardown and mocking

Potential Issues & Concerns

1. Test Coverage ⚠️ CRITICAL

The following functionality is no longer covered by tests after this deletion:

Deleted test coverage:

• tests/attachments.test.ts (128 lines) - Attachment handling logic
• tests/authenticate.test.ts (91 lines) - Authentication flows
• tests/devices.test.ts (377 lines) - Device management operations
• tests/init.test.ts (199 lines) - Initialization flows
• tests/invite-requests.test.ts (455 lines) - Invite request handling
• tests/invites.test.ts (821 lines) - Invite creation/management
• tests/notifications-invites.test.ts (292 lines) - Notification-invite integration
• tests/notifications.test.ts (328 lines) - Notification handling
• tests/profiles-batch.test.ts (196 lines) - Batch profile operations
• tests/profiles.test.ts (939 lines) - Profile management
• tests/wallet.test.ts (139 lines) - Wallet operations

Remaining test files:

• tests/notifications-service.test.ts (337 lines) - Push notification service unit tests
• tests/notifications.client.test.ts - Notification client tests
• tests/invites-v2.test.ts - V2 invites tests
• tests/authMiddleware.test.ts - Auth middleware tests

Recommendation: Before merging, please confirm:

1. Are all V1 APIs being deprecated/removed? If not, these endpoints still need test coverage
2. Do the V2 equivalents have test coverage? I only see invites-v2.test.ts - where are tests for:
   • V2 device registration (src/api/v2/device/handlers/register.ts)
   • V2 notifications (src/api/v2/notifications/handlers/)
   • V2 auth (src/api/v2/auth/handlers/generate-token.ts)
   • V2 profiles, attachments, wallets?
3. Is there a tracking issue or follow-up PR to add V2 test coverage?

2. Modified File ✅

The only modification to notifications-service.test.ts removes a test for FCM push tokens:

- test("rejects FCM push token type (not supported)", async () => {

This is acceptable if FCM support has been removed from the codebase.

3. Performance Considerations ✅

No performance concerns - removing test code doesn't impact runtime performance.

4. Security Concerns ⚠️

Reducing test coverage can mask security issues. Specifically:

• Authentication flows (authenticate.test.ts) no longer tested
• Device registration security no longer validated
• Invite authorization logic no longer covered

Recommendation: Ensure V2 equivalents have comprehensive security tests, especially around:

• JWT token generation and validation
• Device registration and authentication
• Invite permission checks

5. Documentation 📝

Recommendation: Consider adding a PR description explaining:

• Why these tests are being removed
• Whether V1 APIs are being deprecated
• Status of V2 test coverage
• Any follow-up work planned

Approval Recommendation

❌ Request Changes

While the code deletion itself is clean, I recommend addressing test coverage concerns before merging:

1. Document the migration strategy: Add a PR description explaining the V1→V2 migration plan
2. Verify V1 API status: Confirm whether V1 APIs are still active or being deprecated
3. Test coverage plan: Either:
   • Add V2 test coverage in this PR, or
   • Create a follow-up issue/PR to add V2 tests and link it in the description
4. Security validation: Ensure security-critical flows (auth, device registration) have equivalent V2 test coverage

Questions for the Author

1. Are V1 APIs (src/api/v1/*) being removed or just the tests?
2. Is there a tracking issue for V2 test coverage?
3. Have you verified V2 has equivalent functionality for all removed test scenarios?
4. Is this blocking any deployments or can V2 tests be added incrementally?

---

[coderabbitai]

Walkthrough

This PR removes all integration test files from the tests directory, eliminating end-to-end test coverage for multiple APIs including attachments, authentication, devices, profiles, invites, notifications, wallets, and invite requests functionality.

Changes

Cohort / File(s)	Summary
API Integration Tests tests/attachments.test.ts, tests/authenticate.test.ts, tests/devices.test.ts, tests/wallet.test.ts	Removed integration test suites for attachments (S3 uploads/downloads with SHA-256 verification), authentication (JWT token validation, signature verification), devices (CRUD operations), and wallet (Turnkey SDK sub-organization creation) APIs.
Profiles API Tests tests/profiles.test.ts, tests/profiles-batch.test.ts	Deleted comprehensive test suites for profile CRUD, validation, ownership checks, search functionality, and batch profile retrieval with filtering.
Invites API Tests tests/invites.test.ts, tests/invite-requests.test.ts	Removed test coverage for invite creation, updates, deletion, authorization checks, join request flows, and multi-user invite scenarios.
Notifications API Tests tests/notifications.test.ts, tests/notifications-service.test.ts, tests/notifications-invites.test.ts	Eliminated integration tests for /notifications register/unregister/subscribe/unsubscribe endpoints, FCM token rejection validation, and invite notification push payload structures.
Initialization Tests tests/init.test.ts	Removed test suite for /init API user creation with device and identity setup.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~15 minutes

• Verify all deleted files are test-only and contain no shared utilities or production code
• Confirm removal aligns with related PRs that may have restructured or deprecated these APIs
• Cross-reference file deletions against any test configuration or coverage requirements

Possibly related PRs

• feat: push tokens handling, and APNS implementation #113: Modifies notification push-token handling and updates tests/notifications.test.ts, directly overlapping with the removed notification tests.
• fix: Better identity handling + Device API clean up #77: Refactors authenticate (JWT payload xmtpInstallationId) and device API handlers, making the removed authenticate and devices tests outdated.
• Add notifications support #39: Implements notifications API, client, and corresponding tests—creates conflict with removal of comprehensive notification test coverage.

Suggested reviewers

• rygine

Poem

🐰 Hop goes the testing suite, deleted with care,
Old integration tests vanish into thin air,
New paths arise as the old fade away,
Refactored and fresh for a brighter API day! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.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 PR title "Remove unused tests" directly and accurately describes the primary change in the changeset, which involves the removal of 13 test files across the project (attachments, authenticate, devices, init, invite-requests, invites, notifications-invites, notifications-service, notifications, profiles-batch, profiles, and wallet tests). The title is clear, concise, and non-misleading, enabling a teammate scanning through commit history to quickly understand that test files are being deleted. While the title could be slightly more specific about the scope or which test suites are affected, it sufficiently captures the main action and intent of the changeset.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch lr/v1-tests-cleanup

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 596ce8e and 4771fed.

📒 Files selected for processing (12)

• tests/attachments.test.ts (0 hunks)
• tests/authenticate.test.ts (0 hunks)
• tests/devices.test.ts (0 hunks)
• tests/init.test.ts (0 hunks)
• tests/invite-requests.test.ts (0 hunks)
• tests/invites.test.ts (0 hunks)
• tests/notifications-invites.test.ts (0 hunks)
• tests/notifications-service.test.ts (0 hunks)
• tests/notifications.test.ts (0 hunks)
• tests/profiles-batch.test.ts (0 hunks)
• tests/profiles.test.ts (0 hunks)
• tests/wallet.test.ts (0 hunks)

💤 Files with no reviewable changes (12)

• tests/notifications-service.test.ts
• tests/profiles-batch.test.ts
• tests/invites.test.ts
• tests/init.test.ts
• tests/profiles.test.ts
• tests/attachments.test.ts
• tests/invite-requests.test.ts
• tests/notifications-invites.test.ts
• tests/wallet.test.ts
• tests/notifications.test.ts
• tests/devices.test.ts
• tests/authenticate.test.ts

⏰ 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). (3)

• GitHub Check: claude-review
• GitHub Check: push_to_registry
• GitHub Check: Typecheck, format check, lint

---

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

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