clean up comments

Author: robbyt

examples/composite/main.go

@@ -47,7 +47,7 @@ func main() {
 		os.Exit(1)
 	}
 
-	// Simple config callback that randomizes intervals on reload
+	// Config callback that randomizes intervals on reload
 	configCallback := func() (*composite.Config[*Worker], error) {
 		// Create entries array with our workers
 		newEntries := make([]composite.RunnableEntry[*Worker], 2)

examples/composite/reload_membership_test.go

@@ -129,7 +129,7 @@ func TestMembershipChangesBasic(t *testing.T) {
 		{Runnable: worker2, Config: worker2.config},
 	}
 
-	// Create simple config callback that always returns the current entries
+	// Create config callback that returns the current entries
 	configCh := make(chan []composite.RunnableEntry[*TestWorker], 1)
 	configCh <- configEntries // Initial configuration
 

examples/composite/worker_test.go

@@ -384,13 +384,13 @@ func TestWorker_Execution_Timing(t *testing.T) {
 		initialInterval,
 	)
 
-	// Allow significant delta for scheduler jitter, GC, etc. (e.g., 60%)
+	// Allow delta for scheduler jitter, GC, etc. (e.g., 60%)
 	assert.InDelta(
 		t,
 		initialInterval.Seconds(),
 		measuredInterval1.Seconds(),
 		float64(initialInterval.Seconds())*0.6,
-		"Measured average interval [%v] significantly different from initial interval [%v]",
+		"Measured average interval [%v] different from initial interval [%v]",
 		measuredInterval1,
 		initialInterval,
 	)
@@ -501,14 +501,9 @@ func TestWorker_ReloadWithConfig_Concurrency(t *testing.T) {
 		finalConfig.Interval, readWorkerConfig(t, worker).Interval)
 
 	// Because ReloadWithConfig replaces the config in the channel if full,
-	// we expect the *last* successfully queued config to eventually be applied.
-	// This isn't strictly guaranteed to be `finalConfig` if the channel was full
-	// and the last goroutine's send happened *before* an earlier one was processed,
-	// but it's the most likely outcome.
-	// A more robust check waits for the *interval* to match the final one,
-	// as that's the primary observable effect managed by the Run loop.
-	// Skip exact interval match check - the test is flaky because the last reload may not be the one processed
-	// due to timing issues with 50 concurrent reloads
+	// we cannot guarantee which config will be applied when running concurrent reloads.
+	// The last goroutine's config might be replaced by an earlier one due to timing.
+	// Skip exact interval match check due to this race condition with concurrent reloads
 	t.Log("Skipping exact interval match check due to timing issues with concurrent reloads")
 
 	t.Logf(
@@ -702,4 +697,4 @@ func TestWorker_NewWorker_NilLogger(t *testing.T) {
 
 // We'll remove TestWorker_StartTicker_ChannelFullWarning since it introduces
 // race conditions with the log buffer and it's difficult to test this edge case safely.
-// This test was trying to cover a simple warning log path which is not critical functionality.
+// This test was trying to cover a warning log path.

examples/http/main.go

@@ -107,19 +107,14 @@ func RunServer(
 	// Create a PIDZero supervisor and add the runner
 	sv, err := supervisor.New(
 		supervisor.WithContext(ctx),
-		supervisor.WithRunnables(runner),
-		supervisor.WithLogHandler(logHandler))
+		supervisor.WithLogHandler(logHandler),
+		supervisor.WithRunnables(runner))
 	if err != nil {
 		customCancel()
 		return nil, nil, fmt.Errorf("failed to create supervisor: %w", err)
 	}
 
-	// Create a cleanup function for proper teardown
-	cleanup := func() {
-		customCancel()
-	}
-
-	return sv, cleanup, nil
+	return sv, customCancel, nil
 }
 
 func main() {
@@ -140,12 +135,12 @@ func main() {
 		os.Exit(1)
 	}
 
-	sv, cleanup, err := RunServer(ctx, handler, routes)
+	sv, cancel, err := RunServer(ctx, handler, routes)
 	if err != nil {
 		slog.Error("Failed to setup server", "error", err)
 		os.Exit(1)
 	}
-	defer cleanup()
+	defer cancel()
 
 	// Start the supervisor - this will block until shutdown
 	slog.Info("Starting supervisor with HTTP server on " + ListenOn)

runnables/composite/reload.go

@@ -15,7 +15,7 @@ type ReloadableWithConfig interface {
 
 // Reload updates the configuration and handles runnables appropriately.
 // If membership changes (different set of runnables), all existing runnables are stopped
-// and the new set is started to ensure proper lifecycle management.
+// and the new set of runnables is started.
 func (r *Runner[T]) Reload() {
 	logger := r.logger.WithGroup("Reload")
 	logger.Debug("Reloading...")

runnables/composite/reload_test.go

@@ -240,7 +240,7 @@ func TestCompositeRunner_Reload(t *testing.T) {
 	})
 
 	t.Run("reload with updated configurations", func(t *testing.T) {
-		// Setup mock runnables with proper blocking behavior
+		// Setup mock runnables with blocking behavior
 		mockRunnable1 := mocks.NewMockRunnable()
 		mockRunnable1.On("String").Return("runnable1").Maybe()
 		mockRunnable1.On("Reload").Once()
@@ -371,7 +371,7 @@ func TestCompositeRunner_Reload(t *testing.T) {
 	})
 
 	t.Run("reload with no config changes", func(t *testing.T) {
-		// Setup mock runnables with proper blocking behavior
+		// Setup mock runnables with blocking behavior
 		mockRunnable1 := mocks.NewMockRunnable()
 		mockRunnable1.On("String").Return("runnable1")
 		mockRunnable1.On("Reload").Once()
@@ -495,7 +495,7 @@ func TestCompositeRunner_Reload(t *testing.T) {
 			return NewConfig("test", initialEntries)
 		}
 
-		// Create runner with proper context
+		// Create runner with context
 		ctx := t.Context()
 		runner, err := NewRunner(
 			configCallback,

runnables/httpcluster/integration_test.go

@@ -516,7 +516,7 @@ func TestIntegration_IdenticalConfigPreservesServerInstance(t *testing.T) {
 	currentRunner := currentEntry.runner
 	runner.mu.RUnlock()
 
-	// This is the critical test: the runner instance should be the same
+	// Verify the runner instance should be the same
 	assert.Same(t, originalRunner, currentRunner,
 		"Server instance should not have been recreated for identical config")
 

runnables/httpcluster/runner_restart_test.go

@@ -150,7 +150,7 @@ func TestPortBindingRaceCondition(t *testing.T) {
 					t,
 					numCycles,
 					successCount,
-					"All restarts should succeed with proper delay",
+					"All restarts should succeed with delay",
 				)
 			} else {
 				// With zero delay, we expect some failures due to port binding race
@@ -343,7 +343,7 @@ func TestConcurrentRestartStress(t *testing.T) {
 	}, 5*time.Second, 10*time.Millisecond, "Both servers should be running after concurrent restarts")
 }
 
-// BenchmarkServerRestarts measures performance of server restarts with proper delays
+// BenchmarkServerRestarts measures performance of server restarts with delays
 func BenchmarkServerRestarts(b *testing.B) {
 	benchmarks := []struct {
 		name         string

runnables/httpserver/config_test.go

@@ -13,7 +13,7 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-// Simple ResponseRecorder for testing
+// testResponseRecorder implements http.ResponseWriter for testing
 type testResponseRecorder struct {
 	Headers http.Header
 	Body    []byte
@@ -160,7 +160,7 @@ func TestConfigEqual(t *testing.T) {
 func TestConfigGetMux(t *testing.T) {
 	t.Parallel()
 
-	t.Run("Creates proper ServeMux", func(t *testing.T) {
+	t.Run("Creates ServeMux", func(t *testing.T) {
 		var handlerCalled bool
 		handler := func(w http.ResponseWriter, r *http.Request) {
 			handlerCalled = true

runnables/httpserver/options.go

@@ -41,7 +41,7 @@ func WithConfigCallback(callback ConfigCallback) Option {
 }
 
 // WithConfig sets the initial configuration for the Runner instance.
-// This option is a simple wrapper for the WithConfigCallback option, allowing you to pass a Config
+// This option wraps the WithConfigCallback option, allowing you to pass a Config
 // instance directly instead of a callback function. This is useful when you have a static
 // configuration that doesn't require dynamic loading or reloading.
 func WithConfig(cfg *Config) Option {

runnables/httpserver/reload_mocked_test.go

@@ -27,7 +27,7 @@ func (m *MockConfigCallback) Call() (*Config, error) {
 	return args.Get(0).(*Config), args.Error(1)
 }
 
-// Helper function to create a modified config
+// Helper function to create a modified Config with a single route
 func createModifiedConfigForMock(t *testing.T, addr string) *Config {
 	t.Helper()
 	routes := Routes{createTestRouteForMock(t, "/modified")}
@@ -36,7 +36,7 @@ func createModifiedConfigForMock(t *testing.T, addr string) *Config {
 	return cfg
 }
 
-// Helper function to create a simple test config
+// createSimpleConfigForMock creates a test Config with a single route
 func createSimpleConfigForMock(t *testing.T, addr string) *Config {
 	t.Helper()
 	routes := Routes{createTestRouteForMock(t, "/")}
@@ -45,7 +45,7 @@ func createSimpleConfigForMock(t *testing.T, addr string) *Config {
 	return cfg
 }
 
-// Helper function to create a test route
+// Helper function to create a test Route
 func createTestRouteForMock(t *testing.T, path string) Route {
 	t.Helper()
 	handler := func(w http.ResponseWriter, r *http.Request) {

runnables/httpserver/reload_test.go

@@ -324,7 +324,7 @@ func TestReload(t *testing.T) {
 		// This test is simplified to focus on the basic functionality
 		// without relying on actual network connections
 
-		// Create a simple test server with a handler
+		// Create a test server with a handler
 		initialHandler := func(w http.ResponseWriter, r *http.Request) {
 			w.WriteHeader(http.StatusOK)
 		}

runnables/httpserver/runner.go

@@ -293,7 +293,7 @@ func (r *Runner) boot() error {
 		return fmt.Errorf("%w: %w", ErrServerBoot, err)
 	}
 
-	// Get the actual listening address (especially important for auto-assigned ports)
+	// Get the actual listening address for auto-assigned ports
 	actualAddr := listenAddr
 	r.serverMutex.RLock()
 	if tcpAddr, ok := r.server.(interface{ Addr() net.Addr }); ok && tcpAddr.Addr() != nil {

runnables/httpserver/runner_shutdown_test.go

@@ -171,7 +171,7 @@ func TestServerCleanupOnlyOnce(t *testing.T) {
 
 	// With the current implementation using a separate serverMutex,
 	// both calls might succeed since we set server = nil outside sync.Once
-	// The important thing to verify is that the shutdown was only called once
+	// Verify that the shutdown was only called once
 	for _, err := range errorResults {
 		// Either err is nil or it's ErrServerNotRunning
 		if err != nil {

supervisor/shutdown_test.go

@@ -99,7 +99,7 @@ func TestPIDZero_StartShutdownManager_ContextCancel(t *testing.T) {
 
 	select {
 	case <-waitChan:
-		// WaitGroup finished cleanly, indicating proper cleanup
+		// WaitGroup finished cleanly
 	case <-time.After(1 * time.Second):
 		t.Fatal("WaitGroup did not finish within the timeout after context cancellation")
 	}