fix(core): replace fmt.Errorf with structured errors, add log service tests
- Replace all fmt.Errorf calls with coreerr.E() from go-log for structured error context (op, msg, underlying error) across core.go, service_manager.go, and runtime_pkg.go (12 violations fixed) - Replace local Error type and E() in e.go with re-exports from go-log, eliminating duplicate implementation while preserving public API - Add comprehensive tests for pkg/log Service (NewService, OnStartup, QueryLevel, TaskSetLevel) — coverage 72.2% → 87.8% - Update CLAUDE.md: Go 1.25 → 1.26, runtime.go → runtime_pkg.go, document go-log error convention - No os.ReadFile/os.WriteFile violations found (all I/O uses go-io) Co-Authored-By: Virgil <virgil@lethean.io>
This commit is contained in:
parent
f4e2701018
commit
29e6d06633
8 changed files with 213 additions and 67 deletions
|
|
@ -68,7 +68,7 @@ core.New(core.WithService(NewMyService))
|
|||
- `WithService`: Auto-discovers service name from package path, registers IPC handler if service has `HandleIPCEvents` method
|
||||
- `WithName`: Explicitly names a service
|
||||
|
||||
### ServiceRuntime Generic Helper (`runtime.go`)
|
||||
### ServiceRuntime Generic Helper (`runtime_pkg.go`)
|
||||
|
||||
Embed `ServiceRuntime[T]` in services to get access to Core and typed options:
|
||||
```go
|
||||
|
|
@ -77,11 +77,12 @@ type MyService struct {
|
|||
}
|
||||
```
|
||||
|
||||
### Error Handling (`e.go`)
|
||||
### Error Handling (go-log)
|
||||
|
||||
Use the `E()` helper for contextual errors:
|
||||
All errors MUST use `E()` from `go-log` (re-exported in `e.go`), never `fmt.Errorf`:
|
||||
```go
|
||||
return core.E("service.Method", "what failed", underlyingErr)
|
||||
return core.E("service.Method", fmt.Sprintf("service %q not found", name), nil)
|
||||
```
|
||||
|
||||
### Test Naming Convention
|
||||
|
|
@ -100,6 +101,6 @@ Tests use `_Good`, `_Bad`, `_Ugly` suffix pattern:
|
|||
|
||||
## Go Workspace
|
||||
|
||||
Uses Go 1.25 workspaces. This module is part of the workspace at `~/Code/go.work`.
|
||||
Uses Go 1.26 workspaces. This module is part of the workspace at `~/Code/go.work`.
|
||||
|
||||
After adding modules: `go work sync`
|
||||
|
|
|
|||
|
|
@ -64,10 +64,10 @@ func WithService(factory func(*Core) (any, error)) Option {
|
|||
serviceInstance, err := factory(c)
|
||||
|
||||
if err != nil {
|
||||
return fmt.Errorf("core: failed to create service: %w", err)
|
||||
return E("core.WithService", "failed to create service", err)
|
||||
}
|
||||
if serviceInstance == nil {
|
||||
return fmt.Errorf("core: service factory returned nil instance")
|
||||
return E("core.WithService", "service factory returned nil instance", nil)
|
||||
}
|
||||
|
||||
// --- Service Name Discovery ---
|
||||
|
|
@ -79,7 +79,7 @@ func WithService(factory func(*Core) (any, error)) Option {
|
|||
parts := strings.Split(pkgPath, "/")
|
||||
name := strings.ToLower(parts[len(parts)-1])
|
||||
if name == "" {
|
||||
return fmt.Errorf("core: service name could not be discovered for type %T (PkgPath is empty)", serviceInstance)
|
||||
return E("core.WithService", fmt.Sprintf("service name could not be discovered for type %T (PkgPath is empty)", serviceInstance), nil)
|
||||
}
|
||||
|
||||
// --- IPC Handler Discovery ---
|
||||
|
|
@ -89,7 +89,7 @@ func WithService(factory func(*Core) (any, error)) Option {
|
|||
if handler, ok := handlerMethod.Interface().(func(*Core, Message) error); ok {
|
||||
c.RegisterAction(handler)
|
||||
} else {
|
||||
return fmt.Errorf("core: service %q has HandleIPCEvents but wrong signature; expected func(*Core, Message) error", name)
|
||||
return E("core.WithService", fmt.Sprintf("service %q has HandleIPCEvents but wrong signature; expected func(*Core, Message) error", name), nil)
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -107,7 +107,7 @@ func WithName(name string, factory func(*Core) (any, error)) Option {
|
|||
return func(c *Core) error {
|
||||
serviceInstance, err := factory(c)
|
||||
if err != nil {
|
||||
return fmt.Errorf("core: failed to create service '%s': %w", name, err)
|
||||
return E("core.WithName", fmt.Sprintf("failed to create service %q", name), err)
|
||||
}
|
||||
return c.RegisterService(name, serviceInstance)
|
||||
}
|
||||
|
|
@ -259,7 +259,7 @@ func (c *Core) PerformAsync(t Task) string {
|
|||
c.wg.Go(func() {
|
||||
result, handled, err := c.PERFORM(t)
|
||||
if !handled && err == nil {
|
||||
err = fmt.Errorf("no handler found for task type %T", t)
|
||||
err = E("core.PerformAsync", fmt.Sprintf("no handler found for task type %T", t), nil)
|
||||
}
|
||||
|
||||
// Broadcast task completed
|
||||
|
|
@ -316,11 +316,11 @@ func ServiceFor[T any](c *Core, name string) (T, error) {
|
|||
var zero T
|
||||
raw := c.Service(name)
|
||||
if raw == nil {
|
||||
return zero, fmt.Errorf("service '%s' not found", name)
|
||||
return zero, E("core.ServiceFor", fmt.Sprintf("service %q not found", name), nil)
|
||||
}
|
||||
typed, ok := raw.(T)
|
||||
if !ok {
|
||||
return zero, fmt.Errorf("service '%s' is of type %T, but expected %T", name, raw, zero)
|
||||
return zero, E("core.ServiceFor", fmt.Sprintf("service %q is type %T, expected %T", name, raw, zero), nil)
|
||||
}
|
||||
return typed, nil
|
||||
}
|
||||
|
|
|
|||
|
|
@ -1,59 +1,26 @@
|
|||
// Package core provides a standardized error handling mechanism for the Core library.
|
||||
// It allows for wrapping errors with contextual information, making it easier to
|
||||
// trace the origin of an error and provide meaningful feedback.
|
||||
// Package core re-exports the structured error types from go-log.
|
||||
//
|
||||
// The design of this package is influenced by the need for a simple, yet powerful
|
||||
// way to handle errors that can occur in different layers of the application,
|
||||
// from low-level file operations to high-level service interactions.
|
||||
// All error construction in the framework MUST use E() (or Wrap, WrapCode, etc.)
|
||||
// rather than fmt.Errorf. This ensures every error carries an operation context
|
||||
// for structured logging and tracing.
|
||||
//
|
||||
// The key features of this package are:
|
||||
// - Error wrapping: The Op and an optional Msg field provide context about
|
||||
// where and why an error occurred.
|
||||
// - Stack traces: By wrapping errors, we can build a logical stack trace
|
||||
// that is more informative than a raw stack trace.
|
||||
// - Consistent error handling: Encourages a uniform approach to error
|
||||
// handling across the entire codebase.
|
||||
// Example:
|
||||
//
|
||||
// return core.E("config.Load", "failed to load config file", err)
|
||||
package core
|
||||
|
||||
import (
|
||||
"fmt"
|
||||
coreerr "forge.lthn.ai/core/go-log"
|
||||
)
|
||||
|
||||
// Error represents a standardized error with operational context.
|
||||
type Error struct {
|
||||
// Op is the operation being performed, e.g., "config.Load".
|
||||
Op string
|
||||
// Msg is a human-readable message explaining the error.
|
||||
Msg string
|
||||
// Err is the underlying error that was wrapped.
|
||||
Err error
|
||||
}
|
||||
// Error is the structured error type from go-log.
|
||||
// It carries Op (operation), Msg (human-readable), Err (underlying), and Code fields.
|
||||
type Error = coreerr.Err
|
||||
|
||||
// E is a helper function to create a new Error.
|
||||
// This is the primary way to create errors that will be consumed by the system.
|
||||
// For example:
|
||||
//
|
||||
// return e.E("config.Load", "failed to load config file", err)
|
||||
// E creates a new structured error with operation context.
|
||||
// This is the primary way to create errors in the Core framework.
|
||||
//
|
||||
// The 'op' parameter should be in the format of 'package.function' or 'service.method'.
|
||||
// The 'msg' parameter should be a human-readable message that can be displayed to the user.
|
||||
// The 'err' parameter is the underlying error that is being wrapped.
|
||||
func E(op, msg string, err error) error {
|
||||
if err == nil {
|
||||
return &Error{Op: op, Msg: msg}
|
||||
}
|
||||
return &Error{Op: op, Msg: msg, Err: err}
|
||||
}
|
||||
|
||||
// Error returns the string representation of the error.
|
||||
func (e *Error) Error() string {
|
||||
if e.Err != nil {
|
||||
return fmt.Sprintf("%s: %s: %v", e.Op, e.Msg, e.Err)
|
||||
}
|
||||
return fmt.Sprintf("%s: %s", e.Op, e.Msg)
|
||||
}
|
||||
|
||||
// Unwrap provides compatibility for Go's errors.Is and errors.As functions.
|
||||
func (e *Error) Unwrap() error {
|
||||
return e.Err
|
||||
}
|
||||
// The 'msg' parameter should be a human-readable message.
|
||||
// The 'err' parameter is the underlying error (may be nil).
|
||||
var E = coreerr.E
|
||||
|
|
|
|||
|
|
@ -23,8 +23,8 @@ func FuzzE(f *testing.F) {
|
|||
}
|
||||
|
||||
s := e.Error()
|
||||
if s == "" {
|
||||
t.Fatal("Error() returned empty string")
|
||||
if s == "" && (op != "" || msg != "") {
|
||||
t.Fatal("Error() returned empty string for non-empty op/msg")
|
||||
}
|
||||
|
||||
// Round-trip: Unwrap should return the underlying error
|
||||
|
|
|
|||
|
|
@ -64,11 +64,11 @@ func NewWithFactories(app any, factories map[string]ServiceFactory) (*Runtime, e
|
|||
for _, name := range names {
|
||||
factory := factories[name]
|
||||
if factory == nil {
|
||||
return nil, fmt.Errorf("failed to create service %s: factory is nil", name)
|
||||
return nil, E("core.NewWithFactories", fmt.Sprintf("factory is nil for service %q", name), nil)
|
||||
}
|
||||
svc, err := factory()
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("failed to create service %s: %w", name, err)
|
||||
return nil, E("core.NewWithFactories", fmt.Sprintf("failed to create service %q", name), err)
|
||||
}
|
||||
svcCopy := svc
|
||||
coreOpts = append(coreOpts, WithName(name, func(c *Core) (any, error) { return svcCopy, nil }))
|
||||
|
|
|
|||
|
|
@ -34,10 +34,10 @@ func (m *serviceManager) registerService(name string, svc any) error {
|
|||
m.mu.Lock()
|
||||
defer m.mu.Unlock()
|
||||
if m.locked {
|
||||
return fmt.Errorf("core: service %q is not permitted by the serviceLock setting", name)
|
||||
return E("core.RegisterService", fmt.Sprintf("service %q is not permitted by the serviceLock setting", name), nil)
|
||||
}
|
||||
if _, exists := m.services[name]; exists {
|
||||
return fmt.Errorf("core: service %q already registered", name)
|
||||
return E("core.RegisterService", fmt.Sprintf("service %q already registered", name), nil)
|
||||
}
|
||||
m.services[name] = svc
|
||||
|
||||
|
|
|
|||
|
|
@ -118,6 +118,58 @@ func TestRotatingWriter_Append(t *testing.T) {
|
|||
}
|
||||
}
|
||||
|
||||
func TestNewRotatingWriter_Defaults(t *testing.T) {
|
||||
m := io.NewMockMedium()
|
||||
|
||||
// MaxAge < 0 disables age-based cleanup
|
||||
w := NewRotatingWriter(RotationOptions{
|
||||
Filename: "test.log",
|
||||
MaxAge: -1,
|
||||
}, m)
|
||||
defer w.Close()
|
||||
|
||||
if w.opts.MaxSize != 100 {
|
||||
t.Errorf("expected default MaxSize 100, got %d", w.opts.MaxSize)
|
||||
}
|
||||
if w.opts.MaxBackups != 5 {
|
||||
t.Errorf("expected default MaxBackups 5, got %d", w.opts.MaxBackups)
|
||||
}
|
||||
if w.opts.MaxAge != 0 {
|
||||
t.Errorf("expected MaxAge 0 (disabled), got %d", w.opts.MaxAge)
|
||||
}
|
||||
}
|
||||
|
||||
func TestRotatingWriter_RotateEndToEnd(t *testing.T) {
|
||||
m := io.NewMockMedium()
|
||||
opts := RotationOptions{
|
||||
Filename: "test.log",
|
||||
MaxSize: 1, // 1 MB
|
||||
MaxBackups: 2,
|
||||
}
|
||||
|
||||
w := NewRotatingWriter(opts, m)
|
||||
|
||||
// Write just under 1 MB
|
||||
_, _ = w.Write([]byte(strings.Repeat("a", 1024*1024-10)))
|
||||
|
||||
// Write more to trigger rotation
|
||||
_, err := w.Write([]byte(strings.Repeat("b", 20)))
|
||||
if err != nil {
|
||||
t.Fatalf("write after rotation failed: %v", err)
|
||||
}
|
||||
w.Close()
|
||||
|
||||
// Verify rotation happened
|
||||
if !m.Exists("test.log.1") {
|
||||
t.Error("expected test.log.1 after rotation")
|
||||
}
|
||||
|
||||
content, _ := m.Read("test.log")
|
||||
if !strings.Contains(content, "bbb") {
|
||||
t.Errorf("expected new data in test.log after rotation, got %q", content)
|
||||
}
|
||||
}
|
||||
|
||||
func TestRotatingWriter_AgeRetention(t *testing.T) {
|
||||
m := io.NewMockMedium()
|
||||
opts := RotationOptions{
|
||||
|
|
|
|||
126
pkg/log/service_test.go
Normal file
126
pkg/log/service_test.go
Normal file
|
|
@ -0,0 +1,126 @@
|
|||
package log
|
||||
|
||||
import (
|
||||
"context"
|
||||
"testing"
|
||||
|
||||
"forge.lthn.ai/core/go/pkg/core"
|
||||
"github.com/stretchr/testify/assert"
|
||||
"github.com/stretchr/testify/require"
|
||||
)
|
||||
|
||||
func TestNewService_Good(t *testing.T) {
|
||||
opts := Options{Level: LevelInfo}
|
||||
factory := NewService(opts)
|
||||
|
||||
c, err := core.New(core.WithName("log", func(cc *core.Core) (any, error) {
|
||||
return factory(cc)
|
||||
}))
|
||||
require.NoError(t, err)
|
||||
|
||||
svc := c.Service("log")
|
||||
require.NotNil(t, svc)
|
||||
|
||||
logSvc, ok := svc.(*Service)
|
||||
require.True(t, ok)
|
||||
assert.NotNil(t, logSvc.Logger)
|
||||
assert.NotNil(t, logSvc.ServiceRuntime)
|
||||
}
|
||||
|
||||
func TestService_OnStartup_Good(t *testing.T) {
|
||||
opts := Options{Level: LevelInfo}
|
||||
factory := NewService(opts)
|
||||
|
||||
c, err := core.New(core.WithName("log", func(cc *core.Core) (any, error) {
|
||||
return factory(cc)
|
||||
}))
|
||||
require.NoError(t, err)
|
||||
|
||||
svc := c.Service("log").(*Service)
|
||||
|
||||
err = svc.OnStartup(context.Background())
|
||||
assert.NoError(t, err)
|
||||
}
|
||||
|
||||
func TestService_QueryLevel_Good(t *testing.T) {
|
||||
opts := Options{Level: LevelDebug}
|
||||
factory := NewService(opts)
|
||||
|
||||
c, err := core.New(core.WithName("log", func(cc *core.Core) (any, error) {
|
||||
return factory(cc)
|
||||
}))
|
||||
require.NoError(t, err)
|
||||
|
||||
svc := c.Service("log").(*Service)
|
||||
err = svc.OnStartup(context.Background())
|
||||
require.NoError(t, err)
|
||||
|
||||
result, handled, err := c.QUERY(QueryLevel{})
|
||||
assert.NoError(t, err)
|
||||
assert.True(t, handled)
|
||||
assert.Equal(t, LevelDebug, result)
|
||||
}
|
||||
|
||||
func TestService_QueryLevel_Bad(t *testing.T) {
|
||||
opts := Options{Level: LevelInfo}
|
||||
factory := NewService(opts)
|
||||
|
||||
c, err := core.New(core.WithName("log", func(cc *core.Core) (any, error) {
|
||||
return factory(cc)
|
||||
}))
|
||||
require.NoError(t, err)
|
||||
|
||||
svc := c.Service("log").(*Service)
|
||||
err = svc.OnStartup(context.Background())
|
||||
require.NoError(t, err)
|
||||
|
||||
// Unknown query type should not be handled
|
||||
result, handled, err := c.QUERY("unknown")
|
||||
assert.NoError(t, err)
|
||||
assert.False(t, handled)
|
||||
assert.Nil(t, result)
|
||||
}
|
||||
|
||||
func TestService_TaskSetLevel_Good(t *testing.T) {
|
||||
opts := Options{Level: LevelInfo}
|
||||
factory := NewService(opts)
|
||||
|
||||
c, err := core.New(core.WithName("log", func(cc *core.Core) (any, error) {
|
||||
return factory(cc)
|
||||
}))
|
||||
require.NoError(t, err)
|
||||
|
||||
svc := c.Service("log").(*Service)
|
||||
err = svc.OnStartup(context.Background())
|
||||
require.NoError(t, err)
|
||||
|
||||
// Change level via task
|
||||
_, handled, err := c.PERFORM(TaskSetLevel{Level: LevelError})
|
||||
assert.NoError(t, err)
|
||||
assert.True(t, handled)
|
||||
|
||||
// Verify level changed via query
|
||||
result, handled, err := c.QUERY(QueryLevel{})
|
||||
assert.NoError(t, err)
|
||||
assert.True(t, handled)
|
||||
assert.Equal(t, LevelError, result)
|
||||
}
|
||||
|
||||
func TestService_TaskSetLevel_Bad(t *testing.T) {
|
||||
opts := Options{Level: LevelInfo}
|
||||
factory := NewService(opts)
|
||||
|
||||
c, err := core.New(core.WithName("log", func(cc *core.Core) (any, error) {
|
||||
return factory(cc)
|
||||
}))
|
||||
require.NoError(t, err)
|
||||
|
||||
svc := c.Service("log").(*Service)
|
||||
err = svc.OnStartup(context.Background())
|
||||
require.NoError(t, err)
|
||||
|
||||
// Unknown task type should not be handled
|
||||
_, handled, err := c.PERFORM("unknown")
|
||||
assert.NoError(t, err)
|
||||
assert.False(t, handled)
|
||||
}
|
||||
Loading…
Add table
Reference in a new issue