feat: tests for game selection #198
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces several modifications across multiple files, primarily affecting the initialization and configuration of storage and handlers. Key changes include updates to the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Outside diff range and nitpick comments (5)
cmd/tracking_test.go (2)
11-28: Improved test structure with clear expectations.The updated test structure with the
expErrMsgfield provides more clarity on expected outcomes for each test case. The test cases cover different scenarios, including successful game selection and error cases, which is good for comprehensive testing.Consider adding a comment above the
testsslice to briefly explain the purpose of these test cases and the fields in the struct. This can enhance readability and maintainability.
32-40: Improved test execution with better error handling.The use of
t.Runfor each test case enhances test organization and reporting. The updated error handling logic provides more precise checking by comparing actual error messages with expected ones.Consider using a helper function for error checking to reduce duplication and improve readability. For example:
func assertError(t *testing.T, got error, want string) { t.Helper() if (got == nil) != (want == "") { t.Errorf("unexpected error status: got %v, want %v", got, want) return } if got != nil && got.Error() != want { t.Errorf("unexpected error message: got %q, want %q", got.Error(), want) } }Then use it in your tests:
assertError(t, err, tt.expErrMsg)pkg/storage/sql/storage.go (1)
26-26: Suggestion: Add documentation for the new parameterConsider adding a comment to document the
useInMemoryDbparameter, explaining its purpose and expected values. This will improve code readability and maintainability.Example:
// NewStorage initializes and returns a new Storage instance. // If useInMemoryDb is true, an in-memory SQLite database is used; // otherwise, a file-based database is created using the path from getDataSource(). func NewStorage(useInMemoryDb bool) (*Storage, error) {pkg/tracker/sf6/cfn/client.go (1)
64-69: Approve changes with a minor suggestion for consistencyThe addition of the browser initialization check is a valuable improvement to the
Authenticatemethod. It prevents potential nil pointer dereferences and aligns with the PR's objective of enhancing error handling.For consistency with the rest of the method, consider updating the error handling as follows:
if t.browser == nil { - statChan <- *status.WithError(fmt.Errorf("browser not initialized")) + status.Err = fmt.Errorf("browser not initialized") + statChan <- *status return }This change makes the error handling consistent with other parts of the method where
status.Erris set directly.cmd/init_test.go (1)
33-39: Avoid hardcoding credentials in codeHardcoding credentials like
CapIDEmailandCapIDPassworddirectly in the code, even in tests, is not recommended. This practice can lead to security vulnerabilities if the code is ever exposed.Consider using environment variables or a secure configuration management system to provide these values. For example:
cfg := config.Config{ AppVersion: "4.0.0", Headless: true, - CapIDEmail: "test", - CapIDPassword: "test", + CapIDEmail: os.Getenv("CAP_ID_EMAIL"), + CapIDPassword: os.Getenv("CAP_ID_PASSWORD"), BrowserSourcePort: 4242, }Ensure to import the
"os"package if you implement this change:import ( "fmt" + "os" "log" "testing" )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- cmd/init_test.go (2 hunks)
- cmd/tracking_test.go (1 hunks)
- main.go (1 hunks)
- pkg/storage/sql/storage.go (1 hunks)
- pkg/tracker/sf6/cfn/client.go (1 hunks)
🧰 Additional context used
🔇 Additional comments (5)
pkg/storage/sql/storage.go (2)
26-26: Approve: Flexible database initializationThe addition of the
useInMemoryDbparameter enhances the flexibility of theNewStoragefunction, allowing for both file-based and in-memory database initialization. This change aligns well with the PR objectives of adding tests, as in-memory databases are particularly useful for testing scenarios.
30-34: Verify: Impact on existing codeThe changes to the
NewStoragefunction signature may affect other parts of the codebase that call this function. Let's verify that all calls toNewStoragehave been updated accordingly.✅ Verification successful
Verified: All calls to
NewStoragehave been updated with the new function signature.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any calls to NewStorage that haven't been updated # Test: Search for NewStorage calls without boolean argument rg 'NewStorage\(\s*\)' --type goLength of output: 352
Script:
#!/bin/bash # Description: Check for any calls to sql.NewStorage that haven't been updated # Test: Search for sql.NewStorage calls without the new arguments rg 'sql\.NewStorage\(\s*\)' --type goLength of output: 136
pkg/tracker/sf6/cfn/client.go (1)
64-69: Overall impact: Improved robustness without altering core functionalityThe addition of the browser initialization check at the beginning of the
Authenticatemethod enhances the robustness of the function without changing its core behavior. This non-breaking change aligns well with the PR's objectives of improving error handling and is consistent with good coding practices.The rest of the method and file remain unchanged, indicating a focused and purposeful modification. This change reduces the risk of runtime errors due to uninitialized browser instances, contributing to the overall stability of the authentication process.
main.go (2)
Line range hint
1-254: Overall, the file structure and initialization logic look good.The change to
NewStorageis the only modification in this file, but it's a valuable addition that enhances the application's flexibility, particularly for testing scenarios. The rest of the file maintains good structure and initialization practices for the application.
135-135: LGTM! Consider verifying otherNewStoragecalls.The change to
sql.NewStorage(false)aligns with the updated function signature and supports the PR objective of adding tests. This modification allows for flexible database configuration, which is beneficial for testing scenarios.To ensure consistency across the codebase:
Verify other occurrences of
NewStoragecalls:If there are other occurrences, make sure they are updated accordingly.
Consider updating the function documentation for
NewStoragein thesqlpackage to reflect the new boolean parameter and its purpose.
Not much worth in testing this atm but just to get some tests going in the repo.
Added in memory db in tests so we can call the storage layer to make assertions that matches were stored etc.
Summary by CodeRabbit
New Features
Authenticatemethod to check for browser initialization.Bug Fixes
Chores