tests: add comprehensive unit tests for base configuration, decorators, and event handling

Author: intel352

base_config_support_test.go

@@ -0,0 +1,27 @@
+package modular
+
+import (
+    "os"
+    "testing"
+)
+
+func TestBaseConfigSupportEnableDisable(t *testing.T) {
+    // ensure disabled path returns nil feeder
+    BaseConfigSettings.Enabled = false
+    if GetBaseConfigFeeder() != nil { t.Fatalf("expected nil feeder when disabled") }
+
+    SetBaseConfig("configs", "dev")
+    if !IsBaseConfigEnabled() { t.Fatalf("expected enabled after SetBaseConfig") }
+    if GetBaseConfigFeeder() == nil { t.Fatalf("expected feeder when enabled") }
+    if GetBaseConfigComplexFeeder() == nil { t.Fatalf("expected complex feeder when enabled") }
+}
+
+func TestDetectBaseConfigStructureNone(t *testing.T) {
+    // run in temp dir without structure
+    wd, _ := os.Getwd()
+    defer os.Chdir(wd)
+    dir := t.TempDir()
+    os.Chdir(dir)
+    BaseConfigSettings.Enabled = false
+    if DetectBaseConfigStructure() { t.Fatalf("should not detect structure") }
+}

config_decorators_test.go

@@ -0,0 +1,48 @@
+package modular
+
+import "testing"
+
+// simple tenant loader for tests
+type testTenantLoader struct{}
+
+// LoadTenants returns an empty slice of Tenant to satisfy TenantLoader.
+func (l *testTenantLoader) LoadTenants() ([]Tenant, error) { return []Tenant{}, nil }
+
+func TestInstanceAwareConfigDecorator(t *testing.T) {
+	cfg := &minimalConfig{Value: "base"}
+	cp := NewStdConfigProvider(cfg)
+	dec := &instanceAwareConfigDecorator{}
+	wrapped := dec.DecorateConfig(cp)
+	if wrapped.GetConfig().(*minimalConfig).Value != "base" {
+		t.Fatalf("decorated config mismatch")
+	}
+	if dec.Name() != "InstanceAware" {
+		t.Fatalf("unexpected name: %s", dec.Name())
+	}
+}
+
+func TestTenantAwareConfigDecorator(t *testing.T) {
+	cfg := &minimalConfig{Value: "base"}
+	cp := NewStdConfigProvider(cfg)
+	dec := &tenantAwareConfigDecorator{loader: &testTenantLoader{}}
+	wrapped := dec.DecorateConfig(cp)
+	if wrapped.GetConfig().(*minimalConfig).Value != "base" {
+		t.Fatalf("decorated config mismatch")
+	}
+	if dec.Name() != "TenantAware" {
+		t.Fatalf("unexpected name: %s", dec.Name())
+	}
+
+	tenantCfg, err := wrapped.(*tenantAwareConfigProvider).GetTenantConfig("t1")
+	if err != nil || tenantCfg.(*minimalConfig).Value != "base" {
+		t.Fatalf("GetTenantConfig unexpected result: %v", err)
+	}
+
+	// error path (nil loader)
+	decNil := &tenantAwareConfigDecorator{}
+	wrappedNil := decNil.DecorateConfig(cp)
+	_, err = wrappedNil.(*tenantAwareConfigProvider).GetTenantConfig("t1")
+	if err == nil {
+		t.Fatalf("expected error when loader nil")
+	}
+}

debug_module_interfaces_test.go

@@ -0,0 +1,87 @@
+package modular
+
+import (
+	"bytes"
+	"os"
+	"testing"
+)
+
+// localTestDbgModule distinct from any existing test module
+type localTestDbgModule struct{}
+
+func (m *localTestDbgModule) Name() string { return "test" }
+
+// Implement minimal Module interface surface used in tests
+func (m *localTestDbgModule) Init(app Application) error  { return nil }
+func (m *localTestDbgModule) Start(app Application) error { return nil }
+func (m *localTestDbgModule) Stop(app Application) error  { return nil }
+
+// localNoopLogger duplicates minimal logger to avoid ordering issues
+type localNoopLogger struct{}
+
+func (n *localNoopLogger) Debug(string, ...interface{}) {}
+func (n *localNoopLogger) Info(string, ...interface{})  {}
+func (n *localNoopLogger) Warn(string, ...interface{})  {}
+func (n *localNoopLogger) Error(string, ...interface{}) {}
+
+// ensure it satisfies Module
+var _ Module = (*localTestDbgModule)(nil)
+
+func TestDebugModuleInterfaces_New(t *testing.T) {
+	cp := NewStdConfigProvider(&minimalConfig{})
+	logger := &localNoopLogger{}
+	app := NewStdApplication(cp, logger).(*StdApplication)
+	app.RegisterModule(&localTestDbgModule{})
+
+	// capture stdout
+	oldStdout := os.Stdout
+	r, w, _ := os.Pipe()
+	os.Stdout = w
+	DebugModuleInterfaces(app, "test")
+	w.Close()
+	os.Stdout = oldStdout
+	var buf bytes.Buffer
+	_, _ = buf.ReadFrom(r)
+	out := buf.String()
+	if out == "" || !bytes.Contains(buf.Bytes(), []byte("Debugging module")) {
+		t.Fatalf("expected debug output, got none")
+	}
+}
+
+func TestDebugModuleInterfacesNotStdApp_New(t *testing.T) {
+	cp := NewStdConfigProvider(&minimalConfig{})
+	logger := &localNoopLogger{}
+	// Register a module on underlying std app then wrap so decorator is not *StdApplication
+	underlying := NewStdApplication(cp, logger)
+	underlying.RegisterModule(&localTestDbgModule{})
+	base := NewBaseApplicationDecorator(underlying)
+	// capture stdout for early error branch
+	oldStdout := os.Stdout
+	r, w, _ := os.Pipe()
+	os.Stdout = w
+	DebugModuleInterfaces(base, "whatever")
+	w.Close()
+	os.Stdout = oldStdout
+	var buf bytes.Buffer
+	_, _ = buf.ReadFrom(r)
+	if !bytes.Contains(buf.Bytes(), []byte("not a StdApplication")) {
+		t.Fatalf("expected not StdApplication message")
+	}
+}
+
+func TestCompareModuleInstances_New(t *testing.T) {
+	m1 := &localTestDbgModule{}
+	m2 := &localTestDbgModule{}
+	// capture stdout
+	oldStdout := os.Stdout
+	r, w, _ := os.Pipe()
+	os.Stdout = w
+	CompareModuleInstances(m1, m2, "test")
+	w.Close()
+	os.Stdout = oldStdout
+	var buf bytes.Buffer
+	_, _ = buf.ReadFrom(r)
+	if !bytes.Contains(buf.Bytes(), []byte("Comparing module instances")) {
+		t.Fatalf("expected compare output")
+	}
+}

decorator_test.go

@@ -0,0 +1,74 @@
+package modular
+
+import (
+	"context"
+	"testing"
+)
+
+// noopLogger provides a minimal Logger implementation for tests in this package.
+type noopLogger struct{}
+
+func (noopLogger) Info(string, ...any)  {}
+func (noopLogger) Error(string, ...any) {}
+func (noopLogger) Warn(string, ...any)  {}
+func (noopLogger) Debug(string, ...any) {}
+
+// minimalConfig used for simple config provider tests
+type minimalConfig struct{ Value string }
+
+func TestBaseApplicationDecoratorForwarding_New(t *testing.T) { // _New to avoid name clash if similar test exists
+	cfg := &minimalConfig{Value: "ok"}
+	cp := NewStdConfigProvider(cfg)
+	logger := &noopLogger{}
+	app := NewStdApplication(cp, logger)
+
+	dec := NewBaseApplicationDecorator(app)
+
+	if dec.ConfigProvider() != cp {
+		t.Fatalf("expected forwarded ConfigProvider")
+	}
+	// register & retrieve config section forwarding
+	otherCfg := &minimalConfig{Value: "section"}
+	otherCP := NewStdConfigProvider(otherCfg)
+	dec.RegisterConfigSection("other", otherCP)
+	got, err := dec.GetConfigSection("other")
+	if err != nil || got != otherCP {
+		t.Fatalf("expected forwarded config section, err=%v", err)
+	}
+	// service registration / retrieval forwarding
+	type svcType struct{ X int }
+	svc := &svcType{X: 7}
+	if err := dec.RegisterService("svc", svc); err != nil {
+		t.Fatalf("register service: %v", err)
+	}
+	var fetched *svcType
+	if err := dec.GetService("svc", &fetched); err != nil || fetched.X != 7 {
+		t.Fatalf("get service failed: %v", err)
+	}
+
+	// verbose config flag forwarding
+	dec.SetVerboseConfig(true)
+	if !dec.IsVerboseConfig() {
+		t.Fatalf("expected verbose config enabled")
+	}
+
+	// Methods that just forward and return nil should still be invoked to cover lines
+	if err := dec.Init(); err != nil { // empty app
+		t.Fatalf("Init forwarding failed: %v", err)
+	}
+	if err := dec.Start(); err != nil { // no modules
+		t.Fatalf("Start forwarding failed: %v", err)
+	}
+	if err := dec.Stop(); err != nil { // no modules
+		t.Fatalf("Stop forwarding failed: %v", err)
+	}
+
+	// Observer / tenant aware branches when inner does not implement those interfaces
+	obsErr := dec.RegisterObserver(nil)
+	if obsErr == nil { // nil observer & not subject => should error with ErrServiceNotFound
+		t.Fatalf("expected error for RegisterObserver when inner not Subject")
+	}
+	if err := dec.NotifyObservers(context.Background(), NewCloudEvent("x", "y", nil, nil)); err == nil {
+		t.Fatalf("expected error for NotifyObservers when inner not Subject")
+	}
+}

modules/chimux/module.go

@@ -180,21 +180,21 @@ func NewChiMuxModule() modular.Module {
 // controllableMiddleware wraps a Chi middleware with a fast enable/disable flag.
 //
 // Why this exists instead of removing middleware from the chi chain:
-//   * Chi builds a linear slice of middleware; removing items would require
+//   - Chi builds a linear slice of middleware; removing items would require
 //     rebuilding the chain and can race with in‑flight requests referencing the
 //     old handler sequence.
-//   * A single atomic flag read on each request is cheaper and simpler than
+//   - A single atomic flag read on each request is cheaper and simpler than
 //     chain reconstruction + synchronization around route rebuilds. Toggling is
 //     expected to be extremely rare (admin action / config reload) while reads
 //     happen on every request.
-//   * Keeping the wrapper stable avoids subtle ordering drift; the original
+//   - Keeping the wrapper stable avoids subtle ordering drift; the original
 //     registration order is preserved in middlewareOrder for deterministic
 //     reasoning and event emission.
 //
 // Thread-safety & performance:
-//   * enabled is an atomic.Bool so hot-path requests avoid taking a lock.
-//   * Disable simply flips the flag; the wrapper then becomes a no-op pass‑through.
-//   * We intentionally DO NOT attempt an atomic pointer swap to a passthrough
+//   - enabled is an atomic.Bool so hot-path requests avoid taking a lock.
+//   - Disable simply flips the flag; the wrapper then becomes a no-op pass‑through.
+//   - We intentionally DO NOT attempt an atomic pointer swap to a passthrough
 //     function; the single conditional branch keeps clarity and is negligible
 //     compared to typical middleware work (logging, auth, etc.). Premature
 //     micro‑optimizations are avoided until profiling justifies them.
@@ -887,12 +887,15 @@ func (m *ChiMuxModule) disabledRouteMiddleware() func(http.Handler) http.Handler
 			if rctx != nil && len(rctx.RoutePatterns) > 0 {
 				pattern = rctx.RoutePatterns[len(rctx.RoutePatterns)-1]
 			} else {
-				// Fallback to the raw request path. WARNING: For parameterized routes (e.g. /users/{id})
-				// chi records the pattern as /users/{id} but r.URL.Path will be the concrete value
-				// such as /users/123. This means a disabled route registered as /users/{id} will NOT
-				// match here and the route may remain active. Admin tooling disabling dynamic routes
-				// should therefore prefer invoking DisableRoute() with the original pattern captured
-				// at registration time rather than a concrete request path.
+				// Fallback to the raw request path.
+				// WARNING: Parameterized mismatch nuance. For parameterized routes (e.g. /users/{id}) chi
+				// records the pattern as /users/{id} but r.URL.Path is the concrete value /users/123.
+				// If DisableRoute() was called with the pattern /users/{id} we only mark that symbolic
+				// pattern as disabled. When we fall back to r.URL.Path here (because RouteContext is
+				// unavailable or empty), we compare against /users/123 which will not match the stored
+				// disabled entry. Net effect: the route still responds. To reliably disable dynamic
+				// routes, always call DisableRoute() using the original pattern (capture it at
+				// registration time) and avoid relying on raw-path fallbacks in admin tooling.
 				pattern = r.URL.Path
 			}
 			method := r.Method

modules/eventbus/memory.go

@@ -214,9 +214,15 @@ func (m *MemoryEventBus) Publish(ctx context.Context, event Event) error {
 		return nil
 	}
 
-	// Optional rotation for fairness: if RotateSubscriberOrder && len>1 we round-robin the
-	// starting index using pubCounter%len to avoid perpetual head-of-line bias. We copy into
-	// a new slice only when start!=0; clarity > micro-optimization until profiling justifies.
+	// Optional rotation for fairness: if RotateSubscriberOrder && len>1 we round‑robin the
+	// starting index (pubCounter % len) to avoid perpetual head‑of‑line bias when one early
+	// subscriber is slow. We allocate a rotated slice only when start != 0. This trades a
+	// single allocation (for the rotated view) in the less common fairness path for simpler
+	// code; if profiling ever shows this as material we could do an in‑place three‑part
+	// reverse or ring‑buffer view, but we intentionally delay such micro‑optimization.
+	// Decline rationale: The fairness feature is opt‑in; when disabled there is zero overhead.
+	// When enabled, the extra allocation happens only for non‑zero rotation offsets. Empirical
+	// profiling should justify any added complexity before adopting in‑place rotation tricks.
 	if m.config.RotateSubscriberOrder && len(allMatchingSubs) > 1 {
 		pc := atomic.AddUint64(&m.pubCounter, 1) - 1
 		ln := len(allMatchingSubs) // ln >= 2 here due to enclosing condition
@@ -435,6 +441,10 @@ func (m *MemoryEventBus) handleEvents(sub *memorySubscription) {
 		if sub.isCancelled() {
 			return
 		}
+		// Decline rationale (atomic flag suggestion): we keep the small RLock‑protected isCancelled()
+		// helper instead of an atomic.Bool to preserve consistency with other guarded fields and
+		// avoid widening the struct with an additional atomic value. The lock is expected to be
+		// uncontended and the helper is on a non‑hot path relative to user handler execution time.
 		select {
 		case <-m.ctx.Done():
 			return

modules/eventbus/metrics_exporters.go

@@ -21,13 +21,11 @@ package eventbus
 //   go exporter.Run(ctx)
 //   ... later cancel();
 //
-// NOTE: Prometheus and Datadog dependencies are optional. If you want to exclude an exporter
-// from a particular build, prefer Go build tags instead of editing this file manually. Example:
-//   //go:build !prometheus
-//   // +build !prometheus
-// Move the Prometheus collector implementation into a prometheus_collector.go file guarded by
-// a complementary build tag (e.g. //go:build prometheus). This keeps the default experience
-// simple (both available) while allowing consumers to tailor binaries without forking.
+// NOTE: Optional deps. To exclude an exporter, use build tags instead of modifying code.
+// Example split:
+//   prometheus_collector.go      -> //go:build prometheus
+//   prometheus_collector_stub.go -> //go:build !prometheus
+// This keeps mainline source simple while letting consumers tailor binaries without forking.
 
 import (
 	"context"

modules/eventbus/module.go

@@ -149,7 +149,7 @@ type EventBusModule struct {
 	router    *EngineRouter
 	mutex     sync.RWMutex
 	isStarted bool
-	subject   modular.Subject // Observer notification target. Lazily created & guarded by m.mutex to avoid races and to skip allocation when apps never register observers.
+	subject   modular.Subject // Observer notification target. Guarded by m.mutex to avoid a data race with RegisterObservers & emission helpers; not allocated unless observers are actually registered (zero‑cost when observation unused).
 }
 
 // DeliveryStats represents basic delivery outcomes for an engine or aggregate.

modules/eventbus/redis.go

@@ -267,10 +267,9 @@ func (r *RedisEventBus) subscribe(ctx context.Context, topic string, handler Eve
 	r.subscriptions[topic][sub.id] = sub
 	r.topicMutex.Unlock()
 
-	// Start message listener goroutine. We use explicit wg.Add(1)/Done instead of
-	// sync.WaitGroup.Go because the helper is stylistically reserved in this
-	// project for long‑running supervisory loops; per‑subscription workers keep the
-	// conventional pattern for clarity and to highlight lifecycle symmetry.
+	// Start message listener goroutine. We intentionally use explicit wg.Add(1)/Done
+	// instead of sync.WaitGroup.Go to mirror the memory engine style and reserve
+	// the helper for broader supervisory loops; symmetry aids reasoning during reviews.
 	r.wg.Add(1)
 	go r.handleMessages(sub)
 

modules/eventlogger/config.go

@@ -42,7 +42,10 @@ type EventLoggerConfig struct {
 	ShutdownEmitStopped bool `yaml:"shutdownEmitStopped" default:"true" desc:"Emit logger stopped operational event on Stop"`
 
 	// ShutdownDrainTimeout specifies how long Stop() should wait for in-flight events to drain.
-	// Zero or negative duration means unlimited wait (Stop blocks until all events processed).
+	// Zero or negative duration means "wait indefinitely" (Stop blocks until all events processed).
+	// This allows operators to explicitly choose between a bounded shutdown and a fully
+	// lossless drain. A very large positive value is NOT treated specially—only <=0 triggers
+	// the indefinite behavior.
 	ShutdownDrainTimeout time.Duration `yaml:"shutdownDrainTimeout" default:"2s" desc:"Maximum time to wait for draining event queue on Stop. Zero or negative = unlimited wait."`
 }