Add comprehensive tests for store backends and telemetry initialization#23
Merged
Add comprehensive tests for store backends and telemetry initialization#23
Conversation
…onents, and set up GitHub Actions CI for automated testing.
Contributor
Reviewer's GuideThis pull request introduces comprehensive testing by adding new test files across multiple packages ( File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey @doganarif - I've reviewed your changes - here's some feedback:
- Consider moving the
mockStorefrommiddleware_test.goto a shared test utility package if it might be useful for testing other components interacting with thestore.Storeinterface.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟡 Testing: 5 issues found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| return false | ||
| } | ||
|
|
||
| func TestWrapMiddleware(t *testing.T) { |
Contributor
There was a problem hiding this comment.
suggestion (testing): Consider adding more test cases for diverse scenarios.
Add tests for these scenarios:
- Different HTTP methods (GET, PUT, DELETE)
- Empty request/response bodies
- logReqBody or logRespBody false
- PathMatcher.ShouldIgnorePath true
- Various HTTP status codes (4xx, 5xx)
Using a table-driven test will help manage these cases.
Suggested implementation:
// mockPathMatcher implements PathMatcher
type mockPathMatcher struct{}
func (m *mockPathMatcher) ShouldIgnorePath(path string) bool {
return false
}
// mockIgnorePathMatcher implements PathMatcher and always ignores the path
type mockIgnorePathMatcher struct{}
func (m *mockIgnorePathMatcher) ShouldIgnorePath(path string) bool {
return true
}func TestWrapMiddleware_DiverseScenarios(t *testing.T) {
tests := []struct {
name string
method string
url string
reqBody string
handlerBody string
handlerCode int
logReqBody bool
logRespBody bool
pathMatcher PathMatcher
}{
{
name: "GET method with body",
method: "GET",
url: "/test",
reqBody: "get-body",
handlerBody: "get-response",
handlerCode: http.StatusOK,
logReqBody: true,
logRespBody: true,
pathMatcher: &mockPathMatcher{},
},
{
name: "PUT method with empty body",
method: "PUT",
url: "/test",
reqBody: "",
handlerBody: "",
handlerCode: http.StatusNoContent,
logReqBody: true,
logRespBody: true,
pathMatcher: &mockPathMatcher{},
},
{
name: "DELETE method with logging disabled",
method: "DELETE",
url: "/test",
reqBody: "delete-body",
handlerBody: "deleted",
handlerCode: http.StatusAccepted,
logReqBody: false,
logRespBody: false,
pathMatcher: &mockPathMatcher{},
},
{
name: "Path should be ignored",
method: "POST",
url: "/ignored",
reqBody: "ignored-body",
handlerBody: "ignored-response",
handlerCode: http.StatusOK,
logReqBody: true,
logRespBody: true,
pathMatcher: &mockIgnorePathMatcher{},
},
{
name: "Handler returns 4xx",
method: "POST",
url: "/client-error",
reqBody: "client-error",
handlerBody: "bad request",
handlerCode: http.StatusBadRequest,
logReqBody: true,
logRespBody: true,
pathMatcher: &mockPathMatcher{},
},
{
name: "Handler returns 5xx",
method: "POST",
url: "/server-error",
reqBody: "server-error",
handlerBody: "internal error",
handlerCode: http.StatusInternalServerError,
logReqBody: true,
logRespBody: true,
pathMatcher: &mockPathMatcher{},
},
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
// Create a fresh mockStore for each case.
store := &mockStore{}
// Create a handler that simulates various status codes and responses.
handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(tc.handlerCode)
w.Write([]byte(tc.handlerBody))
})
// Wrap the handler with the middleware using testcase parameters.
wrapped := Wrap(handler, store, tc.logReqBody, tc.logRespBody, tc.pathMatcher)
req := httptest.NewRequest(tc.method, tc.url, strings.NewReader(tc.reqBody))
rec := httptest.NewRecorder()
wrapped.ServeHTTP(rec, req)
if rec.Code != tc.handlerCode {
t.Errorf("expected status %d; got %d", tc.handlerCode, rec.Code)
}
if rec.Body.String() != tc.handlerBody {
t.Errorf("expected response body %q; got %q", tc.handlerBody, rec.Body.String())
}
})
}
}You may need to review how the Wrap function handles the parameters (logReqBody, logRespBody, and the PathMatcher) to ensure the test assertions reflect the intended behavior. Adjust test expectations accordingly if Wrap performs additional logic.
| "go.opentelemetry.io/otel/trace" | ||
| ) | ||
|
|
||
| func TestOTelMiddleware_ServeHTTP(t *testing.T) { |
Contributor
There was a problem hiding this comment.
suggestion (testing): Enhance span attribute assertions.
Use an in-memory exporter (or adjust testSpanExporter) to capture spans, enabling assertions on attributes like http.method, http.route, http.status_code, net.peer.ip, user_agent, etc., rather than relying on panic-based checks.
Suggested implementation:
"go.opentelemetry.io/otel/trace"
"context"
"sync"
)//
// inMemorySpanExporter is an in-memory exporter for capturing spans for assertions.
type inMemorySpanExporter struct {
mu sync.Mutex
spans []sdktrace.ReadOnlySpan
}
func newInMemorySpanExporter() *inMemorySpanExporter {
return &inMemorySpanExporter{spans: []sdktrace.ReadOnlySpan{}}
}
func (e *inMemorySpanExporter) ExportSpans(ctx context.Context, spans []sdktrace.ReadOnlySpan) error {
e.mu.Lock()
defer e.mu.Unlock()
e.spans = append(e.spans, spans...)
return nil
}
func (e *inMemorySpanExporter) Shutdown(ctx context.Context) error {
return nil
} exporter := newInMemorySpanExporter()
sr := sdktrace.NewSimpleSpanProcessor(exporter)
tp := sdktrace.NewTracerProvider(sdktrace.WithSpanProcessor(sr)) // Wrap the handler with OTelMiddleware and serve the request.
// (Assumes OTelMiddleware is defined to add span attributes.)
req := httptest.NewRequest("GET", "http://example.com/test", nil)
rec := httptest.NewRecorder()
OTelMiddleware(handler).ServeHTTP(rec, req)
// Ensure spans are exported.
_ = tp.ForceFlush(context.Background())
// Retrieve exported spans for assertions.
exporter.mu.Lock()
gotSpans := exporter.spans
exporter.mu.Unlock()
if len(gotSpans) == 0 {
t.Fatalf("no spans were exported")
}
// Perform assertions on span attributes.
span := gotSpans[0]
attrs := span.Attributes()
var httpMethod, httpRoute, httpStatusCode, netPeerIP, userAgent string
for _, attr := range attrs {
switch string(attr.Key) {
case "http.method":
httpMethod = attr.Value.AsString()
case "http.route":
httpRoute = attr.Value.AsString()
case "http.status_code":
httpStatusCode = attr.Value.AsString()
case "net.peer.ip":
netPeerIP = attr.Value.AsString()
case "http.user_agent":
userAgent = attr.Value.AsString()
}
}
if httpMethod != "GET" {
t.Errorf("expected http.method to be GET, got %s", httpMethod)
}
// Additional assertions on httpRoute, httpStatusCode, netPeerIP, userAgent can be added here.• Verify that OTelMiddleware is implemented in your repository and instruments the request as expected.
• Adjust the attribute keys and assertions based on what your middleware generates.
• Make sure to import any additional libraries (if using assertion libraries) as needed.
| "github.com/doganarif/govisual/internal/model" | ||
| ) | ||
|
|
||
| func runStoreTests(t *testing.T, store Store) { |
Contributor
There was a problem hiding this comment.
suggestion (testing): Expand shared store tests to cover more edge cases.
Add tests for: Get on non-existent IDs (expect nil, false); GetAll and GetLatest on an empty store; GetLatest with n=0 and n > item count; multiple items (3-5) for GetAll and GetLatest(n < total); and calling Clear on an empty store.
Suggested implementation:
func runStoreTests(t *testing.T, store Store) {
defer store.Clear()
defer store.Close()
// Create an initial log entry for potential use
log := &model.RequestLog{
ID: "test-1",
Timestamp: time.Now(),
Method: "GET",
Path: "/test",
StatusCode: 200,
}
// Test Get on a non-existent ID (expect nil, false)
t.Run("GetNonExistent", func(t *testing.T) {
if res, ok := store.Get("non-existent"); ok || res != nil {
t.Errorf("Expected Get(non-existent) to return nil, false; got (%v, %v)", res, ok)
}
})
// Test GetAll and GetLatest on an empty store
t.Run("EmptyStore", func(t *testing.T) {
store.Clear() // Ensure the store is empty
if all := store.GetAll(); len(all) != 0 {
t.Errorf("Expected GetAll on empty store to be empty; got %d items", len(all))
}
if latest := store.GetLatest(5); len(latest) != 0 {
t.Errorf("Expected GetLatest on empty store to be empty; got %d items", len(latest))
}
})
// Test multiple items scenarios for GetAll and GetLatest
t.Run("MultipleItems", func(t *testing.T) {
store.Clear() // Start with empty store
// Insert multiple log entries with varying timestamps
logs := []*model.RequestLog{
{ID: "test-1", Timestamp: time.Now().Add(-10 * time.Minute), Method: "GET", Path: "/test1", StatusCode: 200},
{ID: "test-2", Timestamp: time.Now().Add(-5 * time.Minute), Method: "POST", Path: "/test2", StatusCode: 201},
{ID: "test-3", Timestamp: time.Now().Add(-1 * time.Minute), Method: "DELETE", Path: "/test3", StatusCode: 204},
}
for _, l := range logs {
store.Put(l)
}
// Test that GetAll returns the correct count
if all := store.GetAll(); len(all) != 3 {
t.Errorf("Expected GetAll to return 3 items; got %d", len(all))
}
// Test GetLatest with n = 0 returns an empty slice
if latest := store.GetLatest(0); len(latest) != 0 {
t.Errorf("Expected GetLatest(0) to return 0 items; got %d", len(latest))
}
// Test GetLatest with n greater than total items returns all items (assumed descending by Timestamp)
if latest := store.GetLatest(5); len(latest) != 3 {
t.Errorf("Expected GetLatest(5) to return 3 items; got %d", len(latest))
}
// Test GetLatest with n < total items returns the correct count and order
if latest := store.GetLatest(2); len(latest) != 2 {
t.Errorf("Expected GetLatest(2) to return 2 items; got %d", len(latest))
} else {
// Assuming GetLatest returns items sorted in descending order by Timestamp
if latest[0].ID != "test-3" || latest[1].ID != "test-2" {
t.Errorf("GetLatest did not return items in expected descending order; got IDs %s and %s", latest[0].ID, latest[1].ID)
}
}
})
// Test calling Clear on an already empty store
t.Run("ClearEmptyStore", func(t *testing.T) {
store.Clear() // Ensure store is empty
// Calling Clear on an empty store should be safe
store.Clear()
if all := store.GetAll(); len(all) != 0 {
t.Errorf("Expected store to remain empty after Clear; got %d items", len(all))
}
})Ensure that the Store interface implements the following methods:
- Get(id string) (*model.RequestLog, bool)
- GetAll() []*model.RequestLog
- GetLatest(n int) []*model.RequestLog
- Put(log *model.RequestLog)
If these are not present, adjustments may be necessary. Also verify that GetLatest returns items in descending order by Timestamp or modify the test accordingly.
Comment on lines
+7
to
+10
| func TestInMemoryStore(t *testing.T) { | ||
| store := NewInMemoryStore(10) | ||
| runStoreTests(t, store) | ||
| } |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add test specific to InMemoryStore's size limit behavior.
Add a test that inserts maxSize+1 items and asserts the store size stays at maxSize and the oldest item is evicted.
Suggested implementation:
import (
"fmt"
"testing"
)func TestInMemoryStore(t *testing.T) {
store := NewInMemoryStore(10)
runStoreTests(t, store)
}
// TestInMemoryStoreSizeLimit verifies that when more than maxSize items are inserted,
// the store size remains at maxSize and the oldest item is evicted.
func TestInMemoryStoreSizeLimit(t *testing.T) {
maxSize := 10
store := NewInMemoryStore(maxSize)
// Insert maxSize+1 items into the store.
for i := 1; i <= maxSize+1; i++ {
key := fmt.Sprintf("key%d", i)
store.Put(key, i)
}
// Assert the store size remains at maxSize.
if size := store.Size(); size != maxSize {
t.Fatalf("expected store size %d, got %d", maxSize, size)
}
// The oldest inserted item ("key1") should be evicted.
if val, err := store.Get("key1"); err == nil && val != nil {
t.Errorf("expected key1 to be evicted, but found value %v", val)
}
// Ensure that the remaining keys ("key2" to "key11") are present.
for i := 2; i <= maxSize+1; i++ {
key := fmt.Sprintf("key%d", i)
val, err := store.Get(key)
if err != nil {
t.Errorf("error retrieving %s: %v", key, err)
}
if val != i {
t.Errorf("expected %d for %s, got %v", i, key, val)
}
}
}Ensure that the InMemoryStore provides methods Put(key string, value interface{}), Get(key string) (interface{}, error), and Size() int.
If these methods are not yet implemented in the InMemoryStore, they must be added appropriately.
Comment on lines
+8
to
+17
| func TestRedisStore(t *testing.T) { | ||
| connStr := os.Getenv("REDIS_CONN") | ||
| if connStr == "" { | ||
| t.Skip("REDIS_CONN not set; skipping Redis test") | ||
| } | ||
|
|
||
| store, err := NewRedisStore(connStr, 10, 3600) | ||
| if err != nil { | ||
| t.Fatalf("failed to create Redis store: %v", err) | ||
| } |
Contributor
There was a problem hiding this comment.
suggestion (testing): Consider testing TTL behavior for RedisStore.
Add a Redis-specific test that sets a key with a short TTL, waits just past that duration, and verifies it’s no longer retrievable.
Suggested implementation:
import (
"os"
"testing"
"time"
)func TestRedisStore(t *testing.T) {
connStr := os.Getenv("REDIS_CONN")
if connStr == "" {
t.Skip("REDIS_CONN not set; skipping Redis test")
}
store, err := NewRedisStore(connStr, 10, 3600)
if err != nil {
t.Fatalf("failed to create Redis store: %v", err)
}
runStoreTests(t, store)
}
func TestRedisTTL(t *testing.T) {
connStr := os.Getenv("REDIS_CONN")
if connStr == "" {
t.Skip("REDIS_CONN not set; skipping Redis test")
}
store, err := NewRedisStore(connStr, 10, 3600)
if err != nil {
t.Fatalf("failed to create Redis store: %v", err)
}
// Set a key with a TTL of 1 second.
if err := store.Set("ttl_test", "temp", 1); err != nil {
t.Fatalf("failed to set key in Redis store: %v", err)
}
// Wait for the TTL to expire.
time.Sleep(2 * time.Second)
// Verify that the key is expired.
val, err := store.Get("ttl_test")
if err == nil && val != nil {
t.Errorf("expected key 'ttl_test' to expire, but got value: %v", val)
}
}Ensure that the RedisStore implementation supports the Set operation with a TTL parameter (e.g., store.Set(key string, value interface{}, ttl int) error). If not, adjust the test code accordingly to match your existing method signature.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
actwith GOTOOLCHAIN=local workaround🚧 Notes:
This improves test coverage and CI reliability while preserving dev flexibility.
Summary by Sourcery
Add comprehensive test suite for store backends and middleware components, improving test coverage and CI reliability
CI:
Tests:
Chores: