You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Playing around with getting more data from tests per function; it could help us diagnose bottlenecks and form our test strategy further based on the findings. Currently we know that 173 functions out of 501 functions in scope are unit tests for the area, and the rest are integration tests; we can separate coverage based on that, which likely yields some numbers for the following:
statement: coverage of unit tests is lower than coverage of integration tests
the amount of coverage for unit tests (discovery)
the amount of coverage for storage integration tests (discovery)
we can hypothetize the statement to be true, as the number of unit tests is 173/501, meaning that integration test cover a larger area, largely focused on middleware as that is our biggest area by code size as a group;
we already can use the presence of StartTest in individual test functions to categorize a test as an integration test as this will create a gateway lifecycle, but that only covers those particular cases. As I am very fond of venn diagrams, i expect a number of tests to be in the integration tests group, and StartTest a subset of that group. The integration tests without StartTest are the remaining code smells, as these should:
be in their own integration scope (e.g. /storage?)
have it's own behaviour/responsibility scope outside of gateway in an internal package
Essentially the solution is the same, but the concerns are mainly around two outcomes:
if it's an existing area, the tests should move there
if it's not, an internal/area package is created to hold the scope
PR Type
Enhancement, Tests
Description
Refactored gRPC streaming checks to use httputil package.
Improved SSE test reliability by removing flaky behavior and adding context timeout.
Added CPU profiling rate setting in test initialization.
Added a baseline integration test TestStartTest.
Added utility functions for streaming checks in httputil.
Simplified linting process using task lint.
Updated docker-compose with project name and additional services.
Included httpbin service in docker services.
Added Taskfile for inspecting test coverage and profiling data.
Changes walkthrough 📝
Relevant files
Enhancement
5 files
handler_success.go
Refactor gRPC streaming check to use httputil package
gateway/handler_success.go
Replaced IsGrpcStreaming with httputil.IsStreamingRequest.
Possible Bug:
The changes in gateway/reverse_proxy.go modify the behavior of how proxy responses are handled, specifically with the removal of conditionals around IsGrpcStreaming. This could affect the functionality of gRPC streaming in the system. It's important to ensure that these changes are thoroughly tested, especially in scenarios involving gRPC streaming.
Refactoring Concern:
The refactoring in gateway/reverse_proxy.go and gateway/handler_success.go to use httputil.IsStreamingRequest instead of IsGrpcStreaming should be carefully reviewed to ensure that it covers all previous use cases. The change in method signature and logic could lead to unexpected behaviors if not properly handled.
Test Coverage:
The modifications and additions in the test files, such as gateway/reverse_proxy_test.go, need to be reviewed to ensure they adequately cover the new logic introduced in the PR. It's crucial that the tests validate the new streaming checks and handle all edge cases.
Restore conditional logic for wrapping HTTP requests based on their type
The ServeHTTP method always passes true as the third parameter to WrappedServeHTTP, which might not be appropriate for all requests, especially if some should not be wrapped based on certain conditions. Consider restoring the condition to check if the request is a gRPC streaming request or similar conditions that were previously in place.
Why: The suggestion addresses a significant issue where the logic for determining whether to wrap HTTP requests was removed. Restoring this logic ensures that requests are handled appropriately based on their type, which is crucial for correct functionality.
9
Possible issue
Maintain the original order of return types in IsUpgrade function
The IsUpgrade function has been refactored to return a boolean and a string, but the order of return values has been changed which might lead to confusion or bugs. It's recommended to maintain the original order of return types for consistency and to avoid potential bugs in the codebase where the function is used.
Why: The suggestion correctly identifies a potential issue with changing the order of return values, which could lead to bugs. Maintaining the original order improves consistency and reduces the risk of errors.
8
Best practice
Replace time.Sleep with a more reliable synchronization mechanism in tests
The use of time.Sleep in tests can lead to flaky tests due to timing issues. Consider using a more reliable synchronization mechanism, such as using channels or wait groups, to ensure that the test behavior is consistent and does not depend on the system's state or load.
-time.Sleep(50 * time.Millisecond)+// Example using a channel to synchronize go routine completion+done := make(chan bool)+go func() {+ // test logic here+ done <- true+}()+<-done // Wait for the go routine to signal completion
Suggestion importance[1-10]: 7
Why: The suggestion improves test reliability by replacing time.Sleep with a more robust synchronization mechanism. This change reduces the likelihood of flaky tests and ensures consistent test behavior.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User description
Playing around with getting more data from tests per function; it could help us diagnose bottlenecks and form our test strategy further based on the findings. Currently we know that 173 functions out of 501 functions in scope are unit tests for the area, and the rest are integration tests; we can separate coverage based on that, which likely yields some numbers for the following:
we can hypothetize the statement to be true, as the number of unit tests is 173/501, meaning that integration test cover a larger area, largely focused on middleware as that is our biggest area by code size as a group;
we already can use the presence of
StartTest
in individual test functions to categorize a test as an integration test as this will create a gateway lifecycle, but that only covers those particular cases. As I am very fond of venn diagrams, i expect a number of tests to be in the integration tests group, and StartTest a subset of that group. The integration tests without StartTest are the remaining code smells, as these should:Essentially the solution is the same, but the concerns are mainly around two outcomes:
PR Type
Enhancement, Tests
Description
httputil
package.TestStartTest
.httputil
.task lint
.Changes walkthrough 📝
5 files
handler_success.go
Refactor gRPC streaming check to use httputil package
gateway/handler_success.go
IsGrpcStreaming
withhttputil.IsStreamingRequest
.reverse_proxy.go
Refactor reverse proxy to use httputil for streaming checks
gateway/reverse_proxy.go
IsGrpcStreaming
withhttputil.IsStreamingRequest
.IsUpgrade
function to usehttputil.IsUpgrade
.testutil.go
Add CPU profiling rate setting in test initialization
gateway/testutil.go
InitTestMain
.streaming.go
Add utility functions for streaming checks in httputil
internal/httputil/streaming.go
Makefile
Simplify linting process using task lint
Makefile
task lint
.3 files
reverse_proxy_test.go
Improve SSE test reliability and add context timeout
gateway/reverse_proxy_test.go
testutil_test.go
Add baseline integration test for StartTest
gateway/testutil_test.go
TestStartTest
.Taskfile.inspect.yml
Add Taskfile for inspecting test coverage and profiling
gateway/Taskfile.inspect.yml
3 files
docker-compose.yml
Update docker-compose with project name and additional services
docker-compose.yml
docker-compose.yml
Include httpbin service in docker services
docker/services/docker-compose.yml
httpbin.yml
Add httpbin service configuration
docker/services/httpbin.yml