From 4c29f45f3a2e006389f35bf1c7663922d81cec40 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Grzegorz=20Burzy=C5=84ski?= Date: Fri, 7 Feb 2025 15:08:53 +0100 Subject: [PATCH] Apply suggestions from code review Co-authored-by: Jakub Warczarek --- pkg/manager/multiinstance/diagnostics.go | 2 +- pkg/manager/multiinstance/manager_test.go | 13 +++++-------- .../multiinstance_manager_diagnostics_test.go | 14 ++++++++------ 3 files changed, 14 insertions(+), 15 deletions(-) diff --git a/pkg/manager/multiinstance/diagnostics.go b/pkg/manager/multiinstance/diagnostics.go index e33bbd8c5c..afdad649a4 100644 --- a/pkg/manager/multiinstance/diagnostics.go +++ b/pkg/manager/multiinstance/diagnostics.go @@ -24,8 +24,8 @@ type DiagnosticsServer struct { listenerPort int handlers map[manager.ID]http.Handler - mux *http.ServeMux muxLock sync.Mutex + mux *http.ServeMux } func NewDiagnosticsServer(listenerPort int) *DiagnosticsServer { diff --git a/pkg/manager/multiinstance/manager_test.go b/pkg/manager/multiinstance/manager_test.go index 64edf126fc..9f992a40c8 100644 --- a/pkg/manager/multiinstance/manager_test.go +++ b/pkg/manager/multiinstance/manager_test.go @@ -96,12 +96,9 @@ func TestManager_WithDiagnosticsExposer(t *testing.T) { diagnosticsExposer := newMockDiagnosticsExposer() multiManager := multiinstance.NewManager(testr.New(t), multiinstance.WithDiagnosticsExposer(diagnosticsExposer)) - managerRunning := make(chan struct{}) go func() { - close(managerRunning) require.NoError(t, multiManager.Run(ctx)) }() - <-managerRunning // Wait for the manager to start. instanceID1 := manager.NewRandomID() instanceID2 := manager.NewRandomID() @@ -112,8 +109,8 @@ func TestManager_WithDiagnosticsExposer(t *testing.T) { t.Log("Expecting the diagnostics exposer to have the first instance registered") require.EventuallyWithT(t, func(t *assert.CollectT) { - require.Contains(t, diagnosticsExposer.RegisteredInstances(), instanceID1) - require.NotContains(t, diagnosticsExposer.RegisteredInstances(), instanceID2) + assert.Contains(t, diagnosticsExposer.RegisteredInstances(), instanceID1) + assert.NotContains(t, diagnosticsExposer.RegisteredInstances(), instanceID2) }, waitTime, tickTime) t.Log("Scheduling second instance") @@ -122,7 +119,7 @@ func TestManager_WithDiagnosticsExposer(t *testing.T) { t.Log("Expecting the diagnostics exposer to have both instances registered") require.EventuallyWithT(t, func(t *assert.CollectT) { - require.ElementsMatch(t, diagnosticsExposer.RegisteredInstances(), []manager.ID{instanceID1, instanceID2}) + assert.ElementsMatch(t, diagnosticsExposer.RegisteredInstances(), []manager.ID{instanceID1, instanceID2}) }, waitTime, tickTime) t.Log("Stopping first instance") @@ -131,8 +128,8 @@ func TestManager_WithDiagnosticsExposer(t *testing.T) { t.Log("Expecting the diagnostics exposer to have only the second instance registered") require.EventuallyWithT(t, func(t *assert.CollectT) { - require.Contains(t, diagnosticsExposer.RegisteredInstances(), instanceID2) - require.NotContains(t, diagnosticsExposer.RegisteredInstances(), instanceID1) + assert.Contains(t, diagnosticsExposer.RegisteredInstances(), instanceID2) + assert.NotContains(t, diagnosticsExposer.RegisteredInstances(), instanceID1) }, waitTime, tickTime) t.Log("Stopping second instance") diff --git a/test/envtest/multiinstance_manager_diagnostics_test.go b/test/envtest/multiinstance_manager_diagnostics_test.go index 802b71e8d8..9a34fb2735 100644 --- a/test/envtest/multiinstance_manager_diagnostics_test.go +++ b/test/envtest/multiinstance_manager_diagnostics_test.go @@ -51,14 +51,16 @@ func TestMultiInstanceManagerDiagnostics(t *testing.T) { t.Log("Waiting for the diagnostics server to expose instances' diagnostics endpoints") require.EventuallyWithT(t, func(t *assert.CollectT) { resp, err := http.Get(fmt.Sprintf("http://localhost:%d/%s/debug/config/successful", diagPort, mgrInstance1.ID())) - require.NoError(t, err) - resp.Body.Close() - require.Equal(t, http.StatusOK, resp.StatusCode) + if assert.NoError(t, err) { + resp.Body.Close() + assert.Equal(t, http.StatusOK, resp.StatusCode) + } resp, err = http.Get(fmt.Sprintf("http://localhost:%d/%s/debug/config/successful", diagPort, mgrInstance2.ID())) - require.NoError(t, err) - resp.Body.Close() - require.Equal(t, http.StatusOK, resp.StatusCode) + if assert.NoError(t, err) { + resp.Body.Close() + assert.Equal(t, http.StatusOK, resp.StatusCode) + } }, waitTime, tickTime, "diagnostics should be exposed under /{instanceID}/debug/config prefix for both instances") t.Log("Stopping the first instance and waiting for its diagnostics endpoints to be removed from the server")