8000 feat: [#441] Add Fluent Json Api to test Json response [#4] by kkumar-gcc · Pull Request #711 · goravel/framework · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

feat: [#441] Add Fluent Json Api to test Json response [#4] #711

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 14 commits into from
Nov 6, 2024

Conversation

kkumar-gcc
Copy link
Member
@kkumar-gcc kkumar-gcc commented Nov 3, 2024

📑 Description

RelatedTo goravel/goravel#441

Summary by CodeRabbit

  • New Features

    • Enhanced JSON assertion capabilities with new methods for checking key existence, value matching, and absence of keys.
    • Introduced a fluent interface for more complex JSON assertions.
    • Added methods for improved handling of HTTP requests, including token management and basic authentication.
  • Bug Fixes

    • Improved error handling and assertion checks in response handling.
  • Tests

    • Added comprehensive test cases for various HTTP response statuses and JSON assertions, increasing test coverage.
    • Introduced unit tests for validating JSON structures and assertion methods.

✅ Checks

  • Added test cases for my code

@kkumar-gcc kkumar-gcc requested a review from a team as a code owner November 3, 2024 14:06
Copy link
Contributor
coderabbitai bot commented Nov 3, 2024

Walkthrough

The pull request introduces significant enhancements to JSON assertion capabilities within the testing framework. The AssertableJSON and TestResponse interfaces have been updated with multiple new methods to facilitate comprehensive assertions on JSON structures. Additionally, a previously existing file, assertable_json_string.go, has been removed as its functionality is now integrated into the new structure. The changes also include updates to testing files to ensure coverage for the new assertion methods.

Changes

File Change Summary
contracts/testing/assertable_json.go Added methods to the AssertableJSON interface: Count, Has, HasAll, HasAny, Missing, MissingAll, Where, WhereNot, Each, First, HasWithScope.
contracts/testing/test_response.go Updated TestResponse interface with new methods: AssertJson, AssertExactJson, AssertJsonMissing, AssertFluentJson.
testing/assertable_json.go Introduced AssertableJson type and methods for JSON assertions, including Json, Count, Has, HasAll, HasAny, Missing, MissingAll, Where, WhereNot, First, HasWithScope, Each, and getDecoded.
testing/assertable_json_string.go Removed AssertableJSONString type and associated methods.
testing/test_response.go Added methods to TestResponseImpl: AssertJson, AssertExactJson, AssertJsonMissing, AssertFluentJson, and modified existing Json method.
testing/test_response_test.go Added new test functions: TestAssertServiceUnavailable, TestAssertSuccessful, TestServerError, TestAssertJson, TestAssertExactJson, TestAssertJsonMissing, TestAssertFluentJson.
contracts/testing/test_request.go Added methods to TestRequest interface: FlushHeaders, WithToken, WithBasicAuth, WithoutToken.
testing/test_request.go Added methods to TestRequest struct: FlushHeaders, WithToken, WithBasicAuth, WithoutToken.

Possibly related PRs

  • feat: [#441] Optimize the HTTP testing process [#2]  #708: The introduction of the AssertableJSON interface in the testing package is related to the main PR as it enhances the testing capabilities for JSON structures, which aligns with the new methods added to the AssertableJSON interface in the main PR.
  • feat: [#441] Optimize the HTTP testing process [#1] #704: The modifications to the TestRequest and TestResponse interfaces in the contracts/testing package are relevant as they expand the functionality for handling HTTP requests and responses, which complements the enhancements made in the main PR regarding JSON assertions and testing.
  • feat: Add error return for the Call method of Artisan #687: The changes to the Call method of the Artisan interface to return an error instead of causing a panic improve error handling, which is a common theme in the main PR's focus on enhancing testing interfaces and methods.
  • feat: [#495] Centralize Error Messages in Framework (PR#2)  #666: The centralization of error messages in the framework improves the overall error handling strategy, which is relevant to the main PR's focus on enhancing the functionality of testing interfaces, including error handling in assertions.
  • fix: dependency error #659: The expansion of the Application interface to include new methods for service management is related as it enhances the overall framework's capabilities, which can indirectly support the testing enhancements made in the main PR.

Suggested reviewers

  • hwbrzzl
  • devhaozi

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between fc746eb and 101413d.

📒 Files selected for processing (1)
  • testing/assertable_json_test.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • testing/assertable_json_test.go

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@kkumar-gcc kkumar-gcc closed this Nov 3, 2024
@kkumar-gcc kkumar-gcc reopened this Nov 3, 2024
Copy link
codecov bot commented Nov 3, 2024

Codecov Report

Attention: Patch coverage is 69.41176% with 52 lines in your changes missing coverage. Please review.

Project coverage is 69.89%. Comparing base (534a84b) to head (101413d).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
testing/assertable_json.go 70.58% 23 Missing and 12 partials ⚠️
testing/test_request.go 0.00% 14 Missing ⚠️
testing/test_response.go 91.89% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #711      +/-   ##
==========================================
+ Coverage   69.70%   69.89%   +0.19%     
==========================================
  Files         197      197              
  Lines       15298    15455     +157     
==========================================
+ Hits        10663    10803     +140     
- Misses       4059     4062       +3     
- Partials      576      590      +14     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ 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: 9b1bc7a Previous: 1cc1f60 Ratio
BenchmarkFile_ReadWrite 316950 ns/op 1800 B/op 21 allocs/op 202099 ns/op 1799 B/op 21 allocs/op 1.57
BenchmarkFile_ReadWrite - ns/op 316950 ns/op 202099 ns/op 1.57

This comment was automatically generated by workflow using github-action-benchmark.

Copy link
Contributor
@hwbrzzl hwbrzzl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty good 👍

Copy link
Contributor
@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (11)
contracts/testing/assertable_json.go (1)

5-12: LGTM! Consider adding documentation for complex assertions.

The new interface methods provide a comprehensive set of JSON assertions following a fluent interface pattern. The additions are well-structured and logically grouped.

Consider adding GoDoc comments for methods with complex behavior, particularly:

  • Count: Clarify if it works with array lengths, nested object counts, etc.
  • HasAny/HasAll: Document behavior with nested paths
  • Where/WhereNot: Specify type comparison behavior
contracts/testing/test_response.go (2)

44-47: Add documentation for the new JSON assertion methods.

The new methods look good and provide valuable JSON assertion capabilities. However, they would benefit from documentation explaining their behavior, especially:

  • Whether AssertJson performs deep or shallow matching
  • The exact equality semantics of AssertExactJson
  • The scope of AssertJsonMissing (keys only or values too)
  • Expected usage pattern for AssertFluentJson

44-47: Consider breaking down the TestResponse interface.

The interface is growing quite large with 40+ methods. Consider:

  1. Breaking it into smaller, focused interfaces (e.g., JSONAssertions, StatusAssertions, HeaderAssertions)
  2. Using composition to maintain the current API while improving maintainability

Example approach:

type JSONAssertions interface {
    Json() (map[string]any, error)
    AssertJson(map[string]any) TestResponse
    AssertExactJson(map[string]any) TestResponse
    AssertJsonMissing(map[string]any) TestResponse
    AssertFluentJson(func(json AssertableJSON)) TestResponse
}

type StatusAssertions interface {
    AssertStatus(status int) TestResponse
    AssertOk() TestResponse
    // ... other status methods
}

type TestResponse interface {
    JSONAssertions
    StatusAssertions
    // ... other interfaces
}
testing/test_response_test.go (3)

184-188: Rename test function to follow naming convention.

The function name TestServerError doesn't follow the "Assert" prefix convention used by other test functions in this file.

-func TestServerError(t *testing.T) {
+func TestAssertServerError(t *testing.T) {

269-275: Enhance test coverage with complex JSON structures.

The current test only verifies simple key-value pairs. Consider adding test cases for:

  • Nested JSON objects
  • Array handling
  • Different data types (boolean, null, etc.)
 func TestAssertJson(t *testing.T) {
 	res := createTestResponse(http.StatusOK)
-	res.Body = io.NopCloser(strings.NewReader(`{"key1": "value1", "key2": 42}`))
+	res.Body = io.NopCloser(strings.NewReader(`{
+		"key1": "value1",
+		"key2": 42,
+		"nested": {
+			"array": [1, 2, 3],
+			"bool": true,
+			"null": null
+		}
+	}`))
 
 	r := NewTestResponse(t, res)
-	r.AssertJson(map[string]any{"key1": "value1"})
+	r.AssertJson(map[string]any{
+		"key1": "value1",
+		"nested": map[string]any{
+			"array": []any{1.0, 2.0, 3.0},
+			"bool": true,
+		},
+	})
 }

293-307: Add test cases for advanced fluent assertions.

The current test demonstrates basic assertions. Consider adding test cases for:

  • Array operations (count, first, last)
  • Type assertions
  • Regular expression matching
  • Error cases
 func TestAssertFluentJson(t *testing.T) {
-	sampleJson := `{"name": "krishan", "age": 22, "email": "krishan@example.com"}`
+	sampleJson := `{
+		"name": "krishan",
+		"age": 22,
+		"email": "krishan@example.com",
+		"tags": ["go", "testing"],
+		"scores": {
+			"math": 95,
+			"science": 88
+		}
+	}`
 	res := createTestResponse(http.StatusOK)
 	res.Body = io.NopCloser(strings.NewReader(sampleJson))
 
 	r := NewTestResponse(t, res)
 
 	r.AssertFluentJson(func(json contractstesting.AssertableJSON) {
-		json.Has("name").Where("name", "krishan")
-		json.Has("age").Where("age", float64(22))
-		json.Has("email").Where("email", "krishan@example.com")
+		// Test nested objects
+		json.Has("scores").Where("scores.math", float64(95))
+
+		// Test array operations
+		json.Has("tags").Count("tags", 2)
+
+		// Test type assertions
+		json.Type("age", "number")
+		json.Type("tags", "array")
+
+		// Test regex matching
+		json.Where("email", "^[a-z]+@example\\.com$")
 	}).AssertFluentJson(func(json contractstesting.AssertableJSON) {
-		json.Missing("non_existent_field")
+		// Test error cases
+		json.Missing("invalid_field")
+		json.Missing("scores.invalid")
 	})
 }
testing/test_response.go (2)

310-322: Consider enhancing error handling for JSON parsing failures

While the implementation is correct, consider making JSON parsing errors more explicit to help developers debug test failures.

 func (r *TestResponseImpl) AssertJson(data map[string]any) contractstesting.TestResponse {
 	content, err := r.getContent()
-	assert.Nil(r.t, err)
+	assert.Nil(r.t, err, "Failed to read response content")
 
 	assertableJson, err := NewAssertableJSON(r.t, content)
-	assert.Nil(r.t, err)
+	assert.Nil(r.t, err, "Failed to parse response as JSON: %v", err)

344-354: Consider enhancing error messages for better debugging

While the implementation is solid, consider making the error messages more specific to help identify issues during test failures.

 func (r *TestResponseImpl) AssertFluentJson(callback func(json contractstesting.AssertableJSON)) contractstesting.TestResponse {
 	content, err := r.getContent()
-	assert.Nil(r.t, err)
+	assert.Nil(r.t, err, "Failed to read response content for fluent JSON assertion")
 
 	assertableJson, err := NewAssertableJSON(r.t, content)
-	assert.Nil(r.t, err)
+	assert.Nil(r.t, err, "Failed to parse response as JSON for fluent assertion: %v", err)
testing/assertable_json.go (3)

14-18: Use consistent capitalization of 'JSON' in type and function names

The type AssertableJsonImpl uses 'Json', while the constructor NewAssertableJSON uses 'JSON'. For consistency and to follow Go naming conventions, consider renaming AssertableJsonImpl to AssertableJSONImpl.


20-32: Handle JSON arrays in NewAssertableJSON or document the limitation

Currently, NewAssertableJSON expects a JSON object and will fail if the input JSON is an array or other type. Consider enhancing it to handle JSON arrays or updating the documentation to specify that only JSON objects are supported.


98-100: Consider removing the getDecoded() method and access r.decoded directly

Since getDecoded() simply returns r.decoded, and r.decoded is an unexported field used only within the struct methods, you could access r.decoded directly to simplify the code.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 534a84b and 47e03c7.

⛔ Files ignored due to path filters (2)
  • mocks/testing/AssertableJSON.go is excluded by !mocks/**
  • mocks/testing/TestResponse.go is excluded by !mocks/**
📒 Files selected for processing (6)
  • contracts/testing/assertable_json.go (1 hunks)
  • contracts/testing/test_response.go (1 hunks)
  • testing/assertable_json.go (1 hunks)
  • testing/assertable_json_string.go (0 hunks)
  • testing/test_response.go (2 hunks)
  • testing/test_response_test.go (4 hunks)
💤 Files with no reviewable changes (1)
  • testing/assertable_json_string.go
🔇 Additional comments (3)
contracts/testing/assertable_json.go (1)

5-12: Verify implementation handles edge cases correctly.

Since these methods will be implemented by AssertableJsonImpl, we should verify the implementation handles various edge cases properly.

Ensure the implementation properly handles:

  • nil values and empty structures
  • deeply nested JSON objects
  • type mismatches in Where/WhereNot
  • non-existent paths in nested structures
✅ Verification successful

Based on the implementation details and test coverage shown in the output, I can now provide a final response regarding the edge case handling:

Implementation properly handles edge cases with robust validation

The implementation in AssertableJsonImpl shows proper edge case handling:

  • Nil/empty checks: Uses maps.Get() and maps.Has() which safely handle nil maps and missing keys
  • Type validation: Performs type assertions with proper error messages via assert package
  • Nested structures: Properly traverses nested JSON via the maps package utilities
  • Error handling: Proper error handling during JSON unmarshaling in NewAssertableJSON()

The implementation is well-tested with extensive test coverage across various test files, validating both success and error scenarios.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Look for the implementation and test files to verify edge case handling

# Find the implementation file
echo "Looking for AssertableJsonImpl implementation..."
rg -l "type AssertableJsonImpl struct"

# Find related test files
echo "Looking for related test files..."
fd -g "*test.go" | xargs rg -l "AssertableJsonImpl"

# Look for specific edge case tests
echo "Checking for edge case tests..."
rg -A 5 "func Test.*JSON.*nil|func Test.*JSON.*Empty|func Test.*JSON.*Nested" 

Length of output: 456


Script:

#!/bin/bash
# Let's examine the implementation and test files more thoroughly

# First, let's look at the actual implementation
echo "=== Implementation details ==="
rg -A 10 "type AssertableJsonImpl struct" testing/assertable_json.go

# Check the implementation of specific methods
echo -e "\n=== Method implementations ==="
for method in "Count" "Has" "HasAll" "HasAny" "Missing" "MissingAll" "Where" "WhereNot"; do
    echo -e "\n--- $method implementation ---"
    rg -A 10 "func.*$method.*AssertableJSON" testing/assertable_json.go
done

# Look for test cases with more flexible pattern
echo -e "\n=== Test cases ==="
rg -A 10 "func Test.*JSON" testing/assertable_json.go

# Check if there are any error handling or validation checks
echo -e "\n=== Error handling and validation ==="
rg "Error|error|nil|validate|check" testing/assertable_json.go

Length of output: 6575


Script:

#!/bin/bash
# Let's check the maps package implementation since it's crucial for edge case handling

# Check maps package implementation
echo "=== Maps package implementation ==="
rg -A 10 "func.*Get.*map\[string\]any" 
rg -A 10 "func.*Has.*map\[string\]any"
rg -A 10 "func.*HasAny.*map\[string\]any"

# Look for test files
echo -e "\n=== Test files content ==="
fd -g "*_test.go" | xargs rg -l "AssertableJSON|maps.Get|maps.Has"

# If we find test files, examine their content
echo -e "\n=== Test cases content ==="
fd -g "*_test.go" | xargs rg -B 2 -A 10 "func Test"

# Check for any panic handling or recovery mechanisms
echo -e "\n=== Panic handling ==="
rg "panic|recover|defer" testing/assertable_json.go

Length of output: 233936

testing/test_response.go (2)

Line range hint 36-42: LGTM: Clean transition to new JSON assertion implementation

The change from NewAssertableJSONString to NewAssertableJSON maintains the same functionality while aligning with the new implementation structure.


324-329: LGTM: Clean implementation of exact JSON matching

The implementation correctly performs deep equality checking of JSON structures with clear error messaging.

Copy link
Contributor
@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (9)
contracts/testing/test_request.go (1)

16-18: LGTM! Comprehensive authentication methods with room for expansion.

The authentication methods provide good coverage for common scenarios. Consider future enhancements for:

  • OAuth2 client credentials flow
  • API key authentication
  • Custom authentication schemes
testing/assertable_json.go (5)

14-18: Add documentation for exported type.

Consider adding GoDoc comments for the exported AssertableJson struct to improve code documentation.

+// AssertableJson provides a fluent interface for making assertions about JSON data
+// in tests. It wraps the testing context and JSON data for convenient assertions.
 type AssertableJson struct {

20-32: Add input validation and documentation for constructor.

The constructor should validate the input JSON string and have proper documentation.

+// NewAssertableJSON creates a new AssertableJSON instance from a JSON string.
+// It decodes the JSON string into a map and returns an error if the decoding fails.
 func NewAssertableJSON(t *testing.T, jsonStr string) (contractstesting.AssertableJSON, error) {
+    if t == nil {
+        return nil, fmt.Errorf("testing.T instance cannot be nil")
+    }
+    if jsonStr == "" {
+        return nil, fmt.Errorf("JSON string cannot be empty")
+    }

80-88: Improve nil handling in Where method.

The Where method should handle nil values gracefully and provide clear error messages.

 func (r *AssertableJson) Where(key string, value any) contractstesting.AssertableJSON {
     r.Has(key)
     actual := maps.Get(r.getDecoded(), key)
+    if actual == nil && value != nil {
+        assert.Fail(r.t, fmt.Sprintf("Property [%s] is null but expected value was [%v].", key, value))
+        return r
+    }
     assert.Equal(r.t, value, actual, fmt.Sprintf("Expected property [%s] to have value [%v], but got [%v].", key, value, actual))
     return r
 }

98-100: Consider making getDecoded private.

The getDecoded method appears to be an internal helper but is exported. Consider making it private since it's only used internally.

-func (r *AssertableJson) getDecoded() map[string]any {
+func (r *AssertableJson) getDecoded() map[string]any {

1-100: Consider adding test coverage metrics.

While the implementation is solid, ensure comprehensive test coverage for this testing utility itself. Consider adding test cases that cover:

  • Edge cases with different JSON types
  • Error scenarios
  • Method chaining
  • Complex nested JSON structures

This is particularly important as this code is part of the testing framework itself.

testing/test_request.go (3)

34-37: Add documentation for FlushHeaders method

Consider adding a doc comment explaining that this is a destructive operation that removes all previously set headers. This helps prevent unexpected behavior.

+// FlushHeaders removes all previously set headers from the request.
+// Use this when you need to start with a clean slate for header configuration.
 func (r *TestRequest) FlushHeaders() contractstesting.TestRequest {

69-75: Add input validation for token parameter

While the implementation is correct, consider adding validation for empty tokens to prevent invalid Authorization headers.

 func (r *TestRequest) WithToken(token string, ttype ...string) contractstesting.TestRequest {
+	if token == "" {
+		return r.WithoutToken()
+	}
 	tt := "Bearer"
 	if len(ttype) > 0 {
 		tt = ttype[0]
 	}

Missing test coverage for authentication-related methods in TestRequest

The authentication-related methods (WithToken, WithBasicAuth, WithoutToken, FlushHeaders) in testing/test_request.go lack test coverage. While there are extensive tests for other response-related functionality, these new methods remain untested.

Required test coverage:

  • FlushHeaders(): Verify headers are cleared
  • WithToken(): Test both default Bearer and custom token types
  • WithBasicAuth(): Verify correct Base64 encoding and header format
  • WithoutToken(): Ensure Authorization header is removed
🔗 Analysis chain

Line range hint 34-84: Verify test coverage for new authentication methods

The new authentication-related methods form a cohesive feature set. Let's verify the test coverage:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for test cases covering the new authentication methods
rg -l "WithToken|WithBasicAuth|WithoutToken|FlushHeaders" --type go | grep "_test.go"

Length of output: 85


Script:

#!/bin/bash
# Let's try a different approach to find test files and their content
# First, find the test file in the same directory
fd "test_request_test.go" testing/

# If not found, let's check the content of test files in the testing directory
fd ".*_test.go" testing/ --exec cat {}

Length of output: 20540

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 47e03c7 and 7219113.

⛔ Files ignored due to path filters (1)
  • mocks/testing/TestRequest.go is excluded by !mocks/**
📒 Files selected for processing (3)
  • contracts/testing/test_request.go (1 hunks)
  • testing/assertable_json.go (1 hunks)
  • testing/test_request.go (3 hunks)
🔇 Additional comments (6)
contracts/testing/test_request.go (2)

9-9: LGTM! Well-designed header management methods.

The header management methods follow a consistent pattern and provide a fluent interface. The addition of FlushHeaders() is particularly useful for test isolation.

Also applies to: 13-15


9-18: Verify integration with JSON assertion capabilities.

Let's ensure that the new header and authentication methods work seamlessly with the JSON assertion enhancements mentioned in the PR.

✅ Verification successful

Authentication methods integrate well with JSON assertions

The verification shows that:

  1. The TestRequest interface methods for authentication (WithToken, WithBasicAuth) are properly implemented and mocked
  2. The JSON assertion capabilities (AssertFluentJson) are well-integrated into the TestResponse interface
  3. Test cases demonstrate the fluent API design allowing chaining of assertions
  4. The AssertableJSON interface provides comprehensive JSON validation methods (Has, Where, Missing, etc.)

The authentication methods and JSON assertions can work together seamlessly in test cases, where responses from authenticated requests can be validated using the fluent JSON assertion API.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for test files that demonstrate the integration of these methods with JSON assertions

# Look for test files that use both the new methods and JSON assertions
rg -l "WithToken|WithBasicAuth|FlushHeaders" | xargs rg "json"

# Check for examples or documentation
fd -g "*.md" | xargs rg -A 5 "WithToken|WithBasicAuth|FlushHeaders"

Length of output: 132


Script:

#!/bin/bash
# Let's try a different approach to verify the integration

# First, let's find all files containing TestRequest interface or implementation
rg -l "TestRequest"

# Then, let's look for test files that might demonstrate usage
fd -e test.go | xargs rg -A 5 "WithToken|WithBasicAuth|FlushHeaders"

# Also check for any JSON-related assertions in the testing package
rg -g "test*.go" "json|JSON" contracts/testing/

Length of output: 10795


Script:

#!/bin/bash
# Let's examine the implementation and usage of AssertFluentJson to understand the integration

# Check the AssertFluentJson implementation
ast-grep --pattern 'AssertFluentJson($$$)'

# Look for actual test cases using both authentication methods and JSON assertions
rg -B 5 -A 5 "AssertFluentJson" testing/

# Check the AssertableJSON interface/type definition
rg -A 5 "type AssertableJSON"

Length of output: 5966

testing/assertable_json.go (2)

1-12: LGTM! Clean package structure and imports.

The package organization follows Go conventions, and the imports are well-structured with appropriate dependencies for testing functionality.


38-43: ⚠️ Potential issue

Add type checking in Count method.

As mentioned in the past review, the Count method needs type checking to prevent panics with unsupported types.

 func (r *AssertableJson) Count(key string, length int) contractstesting.AssertableJSON {
     actual := maps.Get(r.getDecoded(), key)
+    if actual == nil {
+        assert.Fail(r.t, fmt.Sprintf("Property [%s] does not exist.", key))
+        return r
+    }
+    switch v := actual.(type) {
+    case []interface{}, map[string]interface{}, string:
+        assert.Len(r.t, v, length, fmt.Sprintf("Property [%s] does not have the expected size.", key))
+    default:
+        assert.Fail(r.t, fmt.Sprintf("Property [%s] is not a countable type.", key))
+    }
-    assert.Len(r.t, actual, length, fmt.Sprintf("Property [%s] does not have the expected size.", key))
     return r
 }
testing/test_request.go (2)

5-6: LGTM: Required imports added for new functionality

The new imports are appropriate for the added authentication features.


82-84: LGTM: Clean implementation of token removal

The method correctly reuses existing functionality and follows the DRY principle.

@kkumar-gcc kkumar-gcc closed this Nov 4, 2024
@kkumar-gcc kkumar-gcc reopened this Nov 4, 2024
Copy link
Contributor
@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 7

🧹 Outside diff range and nitpick comments (6)
contracts/testing/assertable_json.go (3)

5-15: Add method grouping comments for better documentation.

The interface methods would benefit from grouping comments to improve readability and documentation. Consider adding descriptive comments for each group of related methods.

 type AssertableJSON interface {
 	Json() map[string]any

+	// Count assertions
 	Count(key string, value int) AssertableJSON
 
+	// Existence assertions
 	Has(key string) AssertableJSON
 	HasAll(keys []string) AssertableJSON
 	HasAny(keys []string) AssertableJSON
 	Missing(key string) AssertableJSON
 	MissingAll(keys []string) AssertableJSON
 
+	// Value comparison assertions
 	Where(key string, value any) AssertableJSON
 	WhereNot(key string, value any) AssertableJSON
 
+	// Iteration and scoped assertions
 	Each(key string, callback func(AssertableJSON)) AssertableJSON
 	First(key string, callback func(AssertableJSON)) AssertableJSON
 	HasWithScope(key string, length int, callback func(AssertableJSON)) AssertableJSON

11-12: Consider using generics for type-safe value comparisons.

The Where and WhereNot methods use any type which could lead to runtime errors. Consider using generics to provide compile-time type safety.

-	Where(key string, value any) AssertableJSON
-	WhereNot(key string, value any) AssertableJSON
+	Where[T comparable](key string, value T) AssertableJSON
+	WhereNot[T comparable](key string, value T) AssertableJSON

13-15: Document callback usage patterns.

The callback-based methods (Each, First, HasWithScope) are powerful but might be unclear to users. Consider adding godoc comments explaining their usage patterns and providing examples.

+	// Each iterates over array elements at the given key, applying the callback to each element
+	// Example: Each("users", func(json AssertableJSON) { json.Has("id").Has("name") })
 	Each(key string, callback func(AssertableJSON)) AssertableJSON
 
+	// First applies the callback to the first element of an array at the given key
+	// Example: First("users", func(json AssertableJSON) { json.Has("id").Has("name") })
 	First(key string, callback func(AssertableJSON)) AssertableJSON
 
+	// HasWithScope verifies array length and applies validation callback to each element
+	// Example: HasWithScope("users", 2, func(json AssertableJSON) { json.Has("id") })
 	HasWithScope(key string, length int, callback func(AssertableJSON)) AssertableJSON
testing/assertable_json_test.go (2)

11-22: Consider adding more edge cases to strengthen the test coverage.

While the current test cases cover the basic scenarios well, consider adding tests for:

  • Empty JSON object/array: {} and []
  • Null JSON: null
  • JSON with special characters: {"key\n": "value\u0022"}

1-177: General improvements needed for the test suite.

While the test coverage is good, consider these overall improvements:

  1. Test Organization:

    • Group related test cases using subtests (t.Run)
    • Consider table-driven tests for similar cases
    • Add test constants for commonly used JSON strings
  2. Documentation:

    • Add package documentation
    • Document test purposes and scenarios
    • Add examples in documentation
  3. Error Handling:

    • Standardize error checking across tests
    • Add helper functions for common assertions
    • Implement consistent panic recovery

Example of using subtests:

+// TestAssertableJSON groups all JSON assertion tests
+func TestAssertableJSON(t *testing.T) {
+	tests := []struct {
+		name     string
+		json     string
+		testFunc func(*testing.T, contractstesting.AssertableJSON)
+	}{
+		{
+			name: "count assertions",
+			json: `{"items": [1, 2, 3]}`,
+			testFunc: func(t *testing.T, aj contractstesting.AssertableJSON) {
+				aj.Count("items", 3)
+			},
+		},
+		// Add more test cases
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			assertable, err := NewAssertableJSON(t, tt.json)
+			assert.NoError(t, err)
+			tt.testFunc(t, assertable)
+		})
+	}
+}
testing/assertable_json.go (1)

98-184: Refactor to reduce code duplication and improve error handling.

  1. The array validation logic is duplicated across First, HasWithScope, and Each methods.
  2. The Each method silently continues on errors, which might hide issues.

Consider these improvements:

  1. Extract common array validation:
func (r *AssertableJson) validateArrayField(key string) ([]any, bool) {
    value, exists := r.getDecoded()[key]
    if !assert.True(r.t, exists, fmt.Sprintf("Property [%s] does not exist.", key)) {
        return nil, false
    }

    array, ok := value.([]any)
    if !assert.True(r.t, ok, fmt.Sprintf("Expected key [%s] to hold an array, but got %T", key, value)) {
        return nil, false
    }

    return array, true
}
  1. Add error handling option in Each:
-func (r *AssertableJson) Each(key string, callback func(contractstesting.AssertableJSON)) contractstesting.AssertableJSON {
+func (r *AssertableJson) Each(key string, callback func(contractstesting.AssertableJSON), continueOnError bool) contractstesting.AssertableJSON {
     // ...
     for _, item := range array {
         itemJson, err := json.Marshal(item)
-        if !assert.NoError(r.t, err) {
+        if err != nil {
+            if !continueOnError {
+                assert.NoError(r.t, err)
+                return r
+            }
             continue
         }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 7219113 and fc746eb.

⛔ Files ignored due to path filters (1)
  • mocks/testing/AssertableJSON.go is excluded by !mocks/**
📒 Files selected for processing (3)
  • contracts/testing/assertable_json.go (1 hunks)
  • testing/assertable_json.go (1 hunks)
  • testing/assertable_json_test.go (1 hunks)
🔇 Additional comments (4)
contracts/testing/assertable_json.go (1)

5-15: Well-designed interface with comprehensive assertion capabilities.

The interface provides a robust set of methods for JSON assertions with:

  • Consistent method chaining pattern
  • Clear, single-responsibility methods
  • Flexible validation capabilities
  • Good coverage of common testing scenarios

This enhancement significantly improves the HTTP testing process by providing a more expressive and maintainable way to assert JSON responses.

testing/assertable_json_test.go (1)

1-9: LGTM!

The package structure and imports are clean and appropriate for the testing context.

testing/assertable_json.go (2)

1-12: LGTM! Package structure and imports are well-organized.

The package name and imports are appropriate for the testing utilities being implemented.


186-188: LGTM! Helper method is appropriately implemented.

The getter method is simple and serves its purpose well.

@kkumar-gcc kkumar-gcc changed the title feat: [#441] Optimize the HTTP testing process [#4] feat: [#441] Add Fluent Json Api to test Json response [#4] Nov 5, 2024
@kkumar-gcc kkumar-gcc requested a review from hwbrzzl November 5, 2024 03:46
return r.WithHeader("Authorization", fmt.Sprintf("%s %s", tt, token))
}

func (r *TestRequest) WithBasicAuth(username, password string) contractstesting.TestRequest {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How to use it in Framework?

Copy link
Member Author
@kkumar-gcc kkumar-gcc Nov 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usage of this be added in next PR. Some time api expect authorization. User can use these methods to test

Comment on lines +13 to +15
Each(key string, callback func(AssertableJSON)) AssertableJSON
First(key string, callback func(AssertableJSON)) AssertableJSON
HasWithScope(key string, length int, callback func(AssertableJSON)) AssertableJSON
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sort them and add test cases for them.

Copy link
Member Author
@kkumar-gcc kkumar-gcc Nov 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have already added test cases. Plz find in assertable_json_test

Copy link
Contributor
@hwbrzzl hwbrzzl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great

@kkumar-gcc kkumar-gcc merged commit 94aaaed into master Nov 6, 2024
11 of 12 checks passed
@kkumar-gcc kkumar-gcc deleted the kkumar-gcc/#441-4 branch November 6, 2024 15:42
@coderabbitai coderabbitai bot mentioned this pull request Dec 25, 2024
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants
0