chore: add store tests #5397
chore: add store tests #5397
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive test coverage for the store layer with support for testing across multiple database backends (SQLite, MySQL, PostgreSQL). The changes include new test infrastructure using testcontainers, extensive test cases for various store entities, and fixes to database initialization queries to support isolated test databases.
Key changes:
- Introduced test infrastructure with testcontainers for MySQL and PostgreSQL, allowing parallel test execution with isolated databases
- Added comprehensive test coverage for user, memo, attachment, reaction, activity, and settings stores
- Fixed database initialization queries to properly scope table checks to the current database/catalog
- Updated filter schema to clarify timestamp handling and fix attachment memo field type from string to int
Reviewed changes
Copilot reviewed 18 out of 20 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| store/test/main_test.go | New test runner that orchestrates running tests against all three database drivers (SQLite, MySQL, PostgreSQL) |
| store/test/containers.go | Container orchestration logic for spinning up MySQL and PostgreSQL containers with per-test database isolation |
| store/test/store.go | Refactored testing store setup to support multiple drivers and removed database reset logic in favor of fresh databases |
| store/test/filter_helpers_test.go | Test helper utilities including builders for memos and attachments, and test context wrappers |
| store/test/user_test.go | Comprehensive user CRUD tests including role-based filtering, pagination, and field updates |
| store/test/user_setting_test.go | User settings tests covering refresh tokens, personal access tokens, webhooks, and shortcuts |
| store/test/instance_setting_test.go | Instance settings tests for basic, general, memo-related, and storage settings |
| store/test/memo_test.go | Memo CRUD tests including visibility filtering, pagination, pinned status, and payload handling |
| store/test/memo_relation_test.go | Memo relation tests for references and comments between memos |
| store/test/memo_filter_test.go | Extensive filter tests covering content, visibility, tags, timestamps, and logical operators |
| store/test/attachment_test.go | Attachment CRUD tests including updates, pagination, and validation |
| store/test/attachment_filter_test.go | Attachment filter tests for filename, mime type, timestamps, and memo associations |
| store/test/reaction_test.go | Reaction tests for upserting and listing reactions by creator and content |
| store/test/activity_test.go | Activity tests for creating and listing activities by type |
| store/db/mysql/mysql.go | Fixed IsInitialized query to scope to current database using TABLE_SCHEMA filter |
| store/db/postgres/postgres.go | Fixed IsInitialized query to scope to current database using table_catalog filter |
| plugin/filter/schema.go | Updated timestamp field expressions with clarifying comments and fixed attachment memo_id field type |
| web/src/components/LeafletMap.tsx | Minor code formatting changes (removed spaces in empty function bodies) |
| go.mod, go.sum | Added testcontainers dependencies and updated various transitive dependencies |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 21 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if os.Getenv("DRIVER") != "" { | ||
| defer TerminateContainers() | ||
| m.Run() | ||
| return |
There was a problem hiding this comment.
The TestMain function does not call os.Exit(m.Run()) when DRIVER is set. This means the test exit code is not propagated, and tests will always appear to pass even if they fail. This is a critical testing infrastructure bug.
| fmt.Println() | ||
| if len(failed) > 0 { | ||
| fmt.Printf("FAIL: %v\n", failed) | ||
| panic("some drivers failed") |
There was a problem hiding this comment.
The runAllDrivers function uses panic("some drivers failed") which is not an appropriate way to fail tests. This should call os.Exit(1) instead to properly propagate the failure exit code.
| panic("some drivers failed") | |
| os.Exit(1) |
store/test/containers.go
Outdated
| for { | ||
| select { | ||
| case <-ctx.Done(): | ||
| return lastErr |
There was a problem hiding this comment.
The waitForDB function returns the last error when the context is done, but this could be nil if no ping attempt has been made yet. This could lead to confusing error messages. Consider returning a more descriptive error like "timeout waiting for database to be ready".
| return lastErr | |
| if lastErr != nil { | |
| return lastErr | |
| } | |
| if err := ctx.Err(); err != nil { | |
| return fmt.Errorf("timeout waiting for database to be ready: %w", err) | |
| } | |
| return fmt.Errorf("timeout waiting for database to be ready") |
No description provided.