feat: [#441] Add session to http testing [#5] #720
Conversation
WalkthroughThe pull request introduces several modifications across multiple files, primarily enhancing the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
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 (
|
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.
| Benchmark suite | Current: 9d5e7cc | Previous: ab4c85c | Ratio |
|---|---|---|---|
Benchmark_Fatal |
2e-7 ns/op 0 B/op 0 allocs/op |
1e-7 ns/op 0 B/op 0 allocs/op |
2 |
Benchmark_Fatal - ns/op |
2e-7 ns/op |
1e-7 ns/op |
2 |
This comment was automatically generated by workflow using github-action-benchmark.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #720 +/- ##
==========================================
+ Coverage 69.78% 69.83% +0.05%
==========================================
Files 205 205
Lines 16334 16396 +62
==========================================
+ Hits 11398 11450 +52
- Misses 4295 4303 +8
- Partials 641 643 +2 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (7)
contracts/testing/assertable_json.go (2)
5-6: LGTM! Well-designed callback-based assertion methods.The new
FirstandEachmethods follow a fluent interface pattern and provide powerful nested assertion capabilities. The callback approach allows for flexible and expressive testing of nested JSON structures.Consider documenting common usage patterns and examples in the package documentation to help users understand these powerful features effectively.
Line range hint
1-17: Well-structured interface with consistent design patterns.The
AssertableJSONinterface maintains a cohesive design with its assertion methods. The new additions enhance JSON testing capabilities while following established patterns:
- Fluent interface design for method chaining
- Consistent callback patterns for nested assertions
- Clear method grouping and organization
Consider adding interface-level documentation comments (godoc) to:
- Explain the overall purpose and usage of
AssertableJSON- Provide examples of common assertion chains
- Document the relationship with HTTP testing and session management
testing/service_provider.go (1)
7-7: LGTM! Consider future refactoring of global variables.The new session-related additions follow the existing patterns consistently. While global variables are generally discouraged in Go, this implementation maintains consistency with the current architecture.
Consider exploring dependency injection patterns in future iterations to reduce global state and improve testability.
Also applies to: 16-16
errors/list.go (1)
16-16: Move the error constant to the session errors group.For better organization and maintainability, consider moving
SessionFacadeNotSetto be grouped with other session-related errors (around line 140 where otherSession*errors are defined).- SessionFacadeNotSet = New("session facade is not initialized") AuthEmptySecret = New("authentication secret is missing or required") AuthInvalidClaims = New("authentication token contains invalid claims") // ... other errors ... SessionDriverAlreadyExists = New("session driver [%s] already exists") SessionDriverExtensionFailed = New("session failed to extend session [%s] driver [%v]") SessionDriverIsNotSet = New("session driver is not set") + SessionFacadeNotSet = New("session facade is not initialized") SessionDriverNotSupported = New("session driver [%s] not supported") SessionNotFound = New("session [%s] not found")testing/test_request.go (3)
23-23: Use consistent naming: renamesessionIdtosessionIDIn Go, acronyms should be consistently capitalized. Consider renaming
sessionIdtosessionIDto follow Go naming conventions.Apply this diff:
- sessionId string + sessionID string
96-100: RenameWithSessionIdtoWithSessionIDfor consistent namingTo adhere to Go naming conventions, consider renaming the method
WithSessionIdtoWithSessionID, and update references tosessionId.Apply this diff:
-func (r *TestRequest) WithSessionId(id string, attributes map[string]any) contractstesting.TestRequest { - r.sessionId = id +func (r *TestRequest) WithSessionID(id string, attributes map[string]any) contractstesting.TestRequest { + r.sessionID = id return r.WithSession(attributes) }
174-174: Update variable usage to match renamedsessionIDFollowing the renaming of
sessionIdtosessionID, update its usage in thesetSessionmethod.Apply this diff:
- session, err := sessionFacade.BuildSession(driver, r.sessionId) + session, err := sessionFacade.BuildSession(driver, r.sessionID)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
mocks/testing/TestRequest.gois excluded by!mocks/**
📒 Files selected for processing (5)
contracts/testing/assertable_json.go(1 hunks)contracts/testing/test_request.go(1 hunks)errors/list.go(1 hunks)testing/service_provider.go(3 hunks)testing/test_request.go(4 hunks)
🔇 Additional comments (5)
contracts/testing/assertable_json.go (2)
11-11: LGTM! Improved method organization.
The repositioning of the Json() method maintains a logical grouping of related assertion methods while preserving the interface's functionality.
10-10: LGTM! Comprehensive scope-based assertion method.
The HasWithScope method effectively combines length validation with nested assertions, providing a robust way to verify both the structure and content of JSON arrays.
Let's verify the consistency of this pattern across the codebase:
✅ Verification successful
The search results show that HasWithScope is consistently implemented across the codebase. Let me verify the implementation details to ensure it's working as expected.
Let me try a different approach to check the implementation.
Implementation verified and working as expected
The HasWithScope method is properly implemented with:
- Correct array existence and type checking
- Length validation support
- Proper scoping for nested assertions
- Consistent test coverage and mocking support
The implementation aligns well with the interface definition and is used correctly in tests.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for similar scope-based validation patterns in tests
rg -g '*.go' -g '!vendor/*' 'HasWithScope|WithScope' -A 3
Length of output: 4447
Script:
#!/bin/bash
# Check the implementation of HasWithScope
ast-grep --pattern 'func $_($_ *AssertableJson) HasWithScope($_, $_, $_) $_ {
$$$
}'
Length of output: 88
Script:
#!/bin/bash
# Check the implementation details of HasWithScope
rg -g '*.go' "func.*HasWithScope" -A 10
Length of output: 4162
contracts/testing/test_request.go (1)
19-20: LGTM! Well-designed session management interface.
The new methods WithSession and WithSessionId are well-designed additions that enhance the HTTP testing capabilities with session management features. The method signatures are clear, follow Go conventions, and provide flexible options for session handling.
Let's verify the complete session management implementation:
✅ Verification successful
Based on the comprehensive verification results, I can now generate the final response:
Session management implementation is complete and well-structured
The session management implementation is properly integrated:
- TestRequest struct correctly includes session-related fields (
sessionId,sessionAttributes,withSession) - Session initialization and handling is implemented in the
callmethod throughsetSession() - Session facade and manager are properly initialized in the service provider
- Error handling for session-related operations is in place
- Mock implementations are available for testing
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the complete session management implementation across the codebase
# Test 1: Verify the session facade implementation
echo "Checking for session facade implementation..."
rg "type Manager interface" -A 10
# Test 2: Verify the error handling for session-related errors
echo "Checking for session-related error definitions..."
rg "SessionFacadeNotSet"
# Test 3: Verify the service provider implementation
echo "Checking for session initialization in service provider..."
rg "sessionFacade.*Manager"
# Test 4: Look for test coverage of new session methods
echo "Checking for test coverage of session methods..."
rg "func Test.*Session" -g "**/*_test.go"
Length of output: 1796
Script:
#!/bin/bash
# Let's verify the implementation of TestRequest interface methods
# Check for the implementation of WithSession and WithSessionId
echo "Checking for implementation of new session methods..."
rg "func.*WithSession" -A 5
# Check for actual usage of these methods in tests
echo "Checking for usage of new session methods in tests..."
rg "WithSession|WithSessionId" -g "**/*_test.go" -A 2
# Check for session-related test helpers
echo "Checking for session test helpers..."
rg "func.*Session.*Helper" -g "**/*_test.go"
# Check for session driver implementations
echo "Looking for session driver implementations..."
ast-grep --pattern 'type $_ struct {
$$$
}
func ($_ $_) BuildSession($$$) {
$$$
}'
Length of output: 4999
Script:
#!/bin/bash
# Let's verify the test request implementation details and session handling
# Check the complete TestRequest struct definition
echo "Checking TestRequest struct definition..."
rg "type TestRequest struct" -A 10
# Check how session is handled in the request execution
echo "Checking session handling in request execution..."
rg "func \(.*TestRequest\) (Get|Post|Put|Patch|Delete)" -A 15
# Look for session initialization in the test setup
echo "Checking test setup with sessions..."
rg "func.*Setup" -g "**/*_test.go" -A 10
# Check for session-related fields and their initialization
echo "Checking session-related fields..."
rg "session.*=|withSession.*=" -B 2
Length of output: 55789
testing/service_provider.go (1)
38-41: LGTM! Verify error definition exists.
The session initialization and error handling follow the established patterns consistently.
Let's verify the error definition exists:
✅ Verification successful
Error definition verified and properly defined
The SessionFacadeNotSet error is correctly defined in errors/list.go with an appropriate error message: "session facade is not initialized"
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if SessionFacadeNotSet error is properly defined
# Expected: Find definition of SessionFacadeNotSet error
# Search for SessionFacadeNotSet definition
rg "SessionFacadeNotSet.*=.*Error{" errors/
Length of output: 169
Script:
#!/bin/bash
# Let's try a simpler search pattern for SessionFacadeNotSet
rg "SessionFacadeNotSet" errors/
# Also search with ast-grep to catch variable declarations
ast-grep --pattern 'SessionFacadeNotSet = $_'
Length of output: 160
testing/test_request.go (1)
91-91: Check for potential data race on sessionAttributes
If TestRequest instances are used concurrently, modifying sessionAttributes without synchronization could lead to race conditions. Consider documenting that TestRequest is not safe for concurrent use or implement proper synchronization.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
testing/test_request.go (1)
19-24: Initialize the sessionId field in the constructorThe
sessionIdfield is added but not initialized in the constructor. Consider initializing it to ensure consistent behavior.type TestRequest struct { t *testing.T ctx context.Context defaultHeaders map[string]string defaultCookies map[string]string sessionId string sessionAttributes map[string]any }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
mocks/testing/TestRequest.gois excluded by!mocks/**
📒 Files selected for processing (2)
contracts/testing/test_request.go(1 hunks)testing/test_request.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/testing/test_request.go
🔇 Additional comments (4)
testing/test_request.go (4)
29-33: LGTM! Constructor properly initializes all fields
The initialization of maps is done correctly using make().
89-92: LGTM! Session attributes management is implemented correctly
The method follows the builder pattern and safely merges attributes using collect.Merge.
123-127: LGTM! Proper error handling for session setup
The session setup is performed at the right point in the request lifecycle with appropriate error handling.
Line range hint 89-185: Add unit tests for session management
The new session management functionality needs test coverage to ensure reliability. Please add tests for:
WithSessionmethod- Session setup with valid/invalid facades
- Error cases in
setSession - Session lifecycle (build, save, release)
This reinforces the earlier review comment about adding unit tests.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
testing/test_request.go(4 hunks)
🔇 Additional comments (4)
testing/test_request.go (4)
19-23: LGTM! Clean struct extension
The addition of sessionAttributes field is well-structured and follows Go conventions.
28-32: LGTM! Proper initialization
The constructor correctly initializes all fields, including the new sessionAttributes map.
88-91: LGTM! Clean implementation of session attributes merger
The method follows the builder pattern consistently and uses thread-safe map operations.
122-126: LGTM! Proper error handling for session setup
The session setup is correctly integrated into the request flow with appropriate error handling.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
contracts/testing/test_response.go (2)
13-13: 9E88 Consider error handling documentation for Session methodThe new
Session()method returns a tuple with an error, which is good practice. However, to ensure consistent usage:Consider adding a comment documenting the specific error conditions that can be returned, similar to how Laravel documents its session-related errors. This would help users handle session-related errors appropriately.
Example documentation format:
// Session returns the session attributes and an error if: // - The session facade is not properly initialized // - The session store is unavailable
Line range hint
5-65: LGTM: Well-structured interface evolutionThe interface modifications demonstrate good design:
- Clear separation between inspection methods (Content, Cookie, Headers, etc.) and assertion methods
- Consistent return types and error handling
- Logical grouping of related functionality
The new session and HTTP inspection methods enhance the testing capabilities while maintaining the interface's clean structure.
Consider documenting the recommended order of method calls, especially for session-related operations, to guide users in writing effective tests.
testing/test_response.go (2)
46-56: Consider adding nil check for Cookie methodThe methods are well-implemented, but the
Cookiemethod could benefit from explicit nil handling when the cookie is not found.Consider this improvement:
func (r *TestResponseImpl) Cookie(name string) *http.Cookie { - return r.getCookie(name) + cookie := r.getCookie(name) + if cookie == nil { + return nil + } + return cookie }
63-65: Consider dependency injection for session facadeUsing a global
sessionFacadevariable makes the code harder to test and maintain. Consider injecting the session facade through the constructor.Example improvement:
type TestResponseImpl struct { t *testing.T mu sync.Mutex response *http.Response content string sessionAttributes map[string]any sessionFacade contracts.Session // Add this field } func NewTestResponse(t *testing.T, response *http.Response, sessionFacade contracts.Session) contractstesting.TestResponse { return &TestResponseImpl{ t: t, response: response, sessionFacade: sessionFacade, } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
mocks/testing/TestResponse.gois excluded by!mocks/**
📒 Files selected for processing (2)
contracts/testing/test_response.go(1 hunks)testing/test_response.go(2 hunks)
🔇 Additional comments (4)
contracts/testing/test_response.go (3)
3-3: LGTM: Required import for new HTTP methods
The addition of "net/http" import is necessary to support the new HTTP-related methods.
7-9: LGTM: Enhanced HTTP response inspection capabilities
The new methods Cookie, Cookies, and Headers provide essential functionality for inspecting HTTP response metadata, aligning with the PR's objective to optimize HTTP testing.
These additions follow standard Go HTTP testing patterns and complement the existing assertion methods.
10-11: Verify impact of method reordering
While the reordering of IsServerError and IsSuccessful methods doesn't affect functionality, it might impact existing documentation or generated docs.
✅ Verification successful
Method reordering has no impact
The reordering of IsServerError and IsSuccessful methods in the interface is safe. The evidence shows:
- No documentation references these methods
- The methods are only used in test-related code
- The mock implementation and actual implementation are independent of method order
- The interface methods have no doc comments that could be affected
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the method reordering impacts any documentation
# Look for any documentation files referencing these methods
rg -l "IsServerError|IsSuccessful" --type md
Length of output: 46
Script:
#!/bin/bash
# Let's check if these methods are referenced in any Go files or test files
# This will help us understand their usage and potential impact
rg -l "IsServerError|IsSuccessful" --type go
# Let's also check the actual implementation to understand if there are any comments
# or documentation associated with these methods
ast-grep --pattern 'func $_($$$) bool {
$$$
}'
Length of output: 4510
Script:
#!/bin/bash
# Let's examine the actual interface and implementation to see if there are any doc comments
# or if the order matters in the interface definition
rg -A 5 "IsServerError|IsSuccessful" contracts/testing/test_response.go
# Also check the mock implementation to understand usage
rg -A 5 "IsServerError|IsSuccessful" mocks/testing/TestResponse.go
# And check the actual test implementation
rg -A 5 "IsServerError|IsSuccessful" testing/test_response.go
Length of output: 3485
testing/test_response.go (1)
21-25: LGTM: Well-structured struct definition
The addition of sessionAttributes field is well-placed and properly typed for session management. The struct maintains thread-safety with the mutex field.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
testing/test_request_test.go (3)
27-92: Consider using error type assertions instead of string comparison.While the error testing is comprehensive, comparing error strings directly is fragile and can break if error messages change. Consider using error types or sentinel errors for more robust testing.
Example refactor:
type testCase struct { name string mockBehavior func(mockDriver *mockssession.Driver, mockSession *mockssession.Session) - expectedError string + expectedError error sessionAttributes map[string]any } // In test cases: - expectedError: "driver retrieval error", + expectedError: errors.New("driver retrieval error"), // Or better, use sentinel errors // In assertions: - s.EqualError(err, tc.expectedError) + s.ErrorIs(err, tc.expectedError)
94-125: Consider adding edge cases to session attributes test.While the happy path is well-tested, consider adding test cases for:
- Empty session attributes map
- Nil values in attributes
- Special characters in keys
- Large values
127-136: Add documentation to clarify test purpose.While the test correctly verifies behavior without a session, it would be helpful to add comments explaining the expected behavior and why we verify that certain methods aren't called.
Add documentation:
+// TestSetSessionUsingWithoutSession verifies that when no session attributes +// are provided, the session manager's methods are not called, ensuring we don't +// create unnecessary sessions. func (s *TestRequestSuite) TestSetSessionUsingWithoutSession() {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
testing/test_request_test.go(1 hunks)
🔇 Additional comments (2)
testing/test_request_test.go (2)
1-10: LGTM! Clean package structure and imports.
The imports are well-organized with proper grouping of standard library and external packages.
1-136: Verify test coverage for session management.
The test implementation is thorough and well-structured. Let's verify the test coverage for the session management functionality.
✅ Verification successful
Let me analyze the test coverage results and generate a final response.
Test coverage for session functionality is comprehensive
The test suite demonstrates thorough coverage of session-related functionality:
- The test file
testing/test_request_test.gocontains comprehensive test cases for session error handling, including driver errors, build session errors, and save errors - Mock implementations in
mocks/session/cover all session interface methods with proper test expectations - Session middleware tests are present in
session/middleware/start_session_test.go - Core session functionality is tested in
session/session_test.goandsession/manager_test.go - Integration with HTTP context is tested in
http/middleware/throttle_test.go
The test implementation follows best practices:
- Uses table-driven tests for error cases
- Properly mocks dependencies
- Verifies expectations on mock objects
- Tests both success and failure scenarios
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage for session-related code
# Find all files that implement session functionality
echo "Files implementing session functionality:"
rg -l "func.*Session" --type go
# Check for any untested session-related functions
echo "\nPotentially untested session functions:"
for file in $(rg -l "func.*Session" --type go); do
if [[ $file != *_test.go ]]; then
functions=$(rg "^func.*Session" "$file" -N --no-filename)
for func in $functions; do
test_file="${file%.*}_test.go"
if ! rg -q "Test.*${func#func }" "$test_file" 2>/dev/null; then
echo "$file: $func"
fi
done
fi
done
Length of output: 338520
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
testing/test_response_test.go (1)
322-350: Refactor repeated setup code to improve maintainability.The setup code for mocking
sessionManager,driver, andsessionis duplicated across multiple tests. Consider extracting this common setup into a helper function to reduce code duplication and enhance readability.Here's an example of how you might refactor the setup:
func setupSessionMocks(t *testing.T) (*mockssession.Manager, *mockssession.Driver, *mockssession.Session) { mockSessionManager := mockssession.NewManager(t) mockDriver := mockssession.NewDriver(t) mockSession := mockssession.NewSession(t) return mockSessionManager, mockDriver, mockSession }Then, in your tests:
func TestSession_Success(t *testing.T) { + mockSessionManager, mockDriver, mockSession := setupSessionMocks(t) sessionFacade = mockSessionManagerAlso applies to: 352-363, 364-376
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
testing/test_request_test.go(1 hunks)testing/test_response_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- testing/test_request_test.go
🔇 Additional comments (2)
testing/test_response_test.go (2)
11-12: Import require package for assertions is appropriate.
The addition of "github.com/stretchr/testify/require" provides useful assertion methods for the tests.
14-15: Import errors and mock sessions packages correctly.
The imports of "github.com/goravel/framework/errors" and mockssession "github.com/goravel/framework/mocks/session" are necessary for error handling and session mocking.
📑 Description
Closes goravel/goravel#441
Summary by CodeRabbit
New Features
First,Each, andHasWithScope.TestRequestinterface with session management capabilities through theWithSessionmethod.SessionFacadeNotSet.TestRequeststruct to manage session data with new fields and methods for session handling.TestResponsefor accessing cookies, headers, and session data.Bug Fixes
✅ Checks