Skip to content

Commit 6cce0ab

Browse files
iblancasaiblancasa
committed
Fix flaky TestComponentStatus test
Signed-off-by: Israel Blancas <[email protected]>
1 parent 79724bc commit 6cce0ab

File tree

3 files changed

+100
-60
lines changed

3 files changed

+100
-60
lines changed

‎internal/healthcheck/extension.go‎

Lines changed: 12 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ package healthcheck // import "github.com/open-telemetry/opentelemetry-collector
55

66
import (
77
"context"
8+
"sync"
89

910
"go.opentelemetry.io/collector/component"
1011
"go.opentelemetry.io/collector/component/componentstatus"
@@ -32,6 +33,7 @@ type HealthCheckExtension struct {
3233
eventCh chan *eventSourcePair
3334
readyCh chan struct{}
3435
host component.Host
36+
shutdownOnce sync.Once
3537
}
3638

3739
var (
@@ -110,17 +112,18 @@ func (hc *HealthCheckExtension) Start(ctx context.Context, host component.Host)
110112

111113
// Shutdown implements the component.Component interface.
112114
func (hc *HealthCheckExtension) Shutdown(ctx context.Context) error {
113-
// Preemptively send the stopped event, so it can be exported before shutdown
114-
componentstatus.ReportStatus(hc.host, componentstatus.NewEvent(componentstatus.StatusStopped))
115-
116-
close(hc.eventCh)
117-
hc.aggregator.Close()
118-
119115
var err error
120-
for _, comp := range hc.subcomponents {
121-
err = multierr.Append(err, comp.Shutdown(ctx))
122-
}
116+
hc.shutdownOnce.Do(func() {
117+
// Preemptively send the stopped event, so it can be exported before shutdown
118+
componentstatus.ReportStatus(hc.host, componentstatus.NewEvent(componentstatus.StatusStopped))
119+
120+
close(hc.eventCh)
121+
hc.aggregator.Close()
123122

123+
for _, comp := range hc.subcomponents {
124+
err = multierr.Append(err, comp.Shutdown(ctx))
125+
}
126+
})
124127
return err
125128
}
126129

‎internal/healthcheck/extension_test.go‎

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
package healthcheck
55

66
import (
7+
"context"
78
"fmt"
89
"io"
910
"net"
@@ -32,6 +33,11 @@ func TestComponentStatus(t *testing.T) {
3233
cfg.GRPCConfig.NetAddr.Endpoint = testutil.GetAvailableLocalAddress(t)
3334
cfg.UseV2 = true
3435
ext := NewHealthCheckExtension(t.Context(), *cfg, extensiontest.NewNopSettings(extensiontest.NopType))
36+
defer func() {
37+
ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
38+
defer cancel()
39+
require.NoError(t, ext.Shutdown(ctx))
40+
}()
3541

3642
// Status before Start will be StatusNone
3743
st, ok := ext.aggregator.AggregateStatus(status.ScopeAll, status.Concise)
@@ -80,7 +86,9 @@ func TestComponentStatus(t *testing.T) {
8086
}, time.Second, 10*time.Millisecond)
8187

8288
require.NoError(t, ext.NotReady())
83-
require.NoError(t, ext.Shutdown(t.Context()))
89+
ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
90+
defer cancel()
91+
require.NoError(t, ext.Shutdown(ctx))
8492

8593
// Events sent after shutdown will be discarded
8694
for _, id := range traces.InstanceIDs() {
@@ -111,18 +119,24 @@ func TestNotifyConfig(t *testing.T) {
111119
cfg.HTTPConfig.Config.Path = "/config"
112120

113121
ext := NewHealthCheckExtension(t.Context(), *cfg, extensiontest.NewNopSettings(extensiontest.NopType))
122+
defer func() {
123+
ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
124+
defer cancel()
125+
require.NoError(t, ext.Shutdown(ctx))
126+
}()
114127

115128
require.NoError(t, ext.Start(t.Context(), componenttest.NewNopHost()))
116-
t.Cleanup(func() { require.NoError(t, ext.Shutdown(t.Context())) })
117129

118130
client := &http.Client{}
131+
defer client.CloseIdleConnections()
119132
url := fmt.Sprintf("http://%s/config", endpoint)
120133

121134
var resp *http.Response
122135

123136
resp, err = client.Get(url)
124137
require.NoError(t, err)
125138
assert.Equal(t, http.StatusServiceUnavailable, resp.StatusCode)
139+
require.NoError(t, resp.Body.Close())
126140

127141
require.NoError(t, ext.NotifyConfig(t.Context(), confMap))
128142

@@ -132,6 +146,7 @@ func TestNotifyConfig(t *testing.T) {
132146

133147
body, err := io.ReadAll(resp.Body)
134148
require.NoError(t, err)
149+
require.NoError(t, resp.Body.Close())
135150
assert.JSONEq(t, string(confJSON), string(body))
136151
}
137152

@@ -151,6 +166,8 @@ func TestShutdown(t *testing.T) {
151166
// Get address already in use here
152167
require.Error(t, ext.Start(t.Context(), componenttest.NewNopHost()))
153168

154-
require.NoError(t, ext.Shutdown(t.Context()))
169+
ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
170+
defer cancel()
171+
require.NoError(t, ext.Shutdown(ctx))
155172
})
156173
}

‎internal/healthcheck/internal/http/server_test.go‎

Lines changed: 68 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
package http
55

66
import (
7+
"context"
78
"encoding/json"
89
"fmt"
910
"io"
@@ -2944,17 +2945,22 @@ func TestStatus(t *testing.T) {
29442945
status.NewAggregator(internalhelpers.ErrPriority(tc.componentHealthConfig)),
29452946
)
29462947

2947-
require.NoError(t, server.Start(t.Context(), componenttest.NewNopHost()))
2948-
defer func() { require.NoError(t, server.Shutdown(t.Context())) }()
2948+
require.NoError(t, server.Start(t.Context(), componenttest.NewNopHost()))
2949+
defer func() {
2950+
ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
2951+
defer cancel()
2952+
require.NoError(t, server.Shutdown(ctx))
2953+
}()
29492954

2950-
var url string
2951-
if tc.legacyConfig.UseV2 {
2952-
url = fmt.Sprintf("http://%s%s", tc.config.Endpoint, tc.config.Status.Path)
2953-
} else {
2954-
url = fmt.Sprintf("http://%s%s", tc.legacyConfig.Endpoint, tc.legacyConfig.Path)
2955-
}
2955+
var url string
2956+
if tc.legacyConfig.UseV2 {
2957+
url = fmt.Sprintf("http://%s%s", tc.config.Endpoint, tc.config.Status.Path)
2958+
} else {
2959+
url = fmt.Sprintf("http://%s%s", tc.legacyConfig.Endpoint, tc.legacyConfig.Path)
2960+
}
29562961

2957-
client := &http.Client{}
2962+
client := &http.Client{}
2963+
defer client.CloseIdleConnections()
29582964

29592965
for _, ts := range tc.teststeps {
29602966
if ts.step != nil {
@@ -2966,38 +2972,46 @@ func TestStatus(t *testing.T) {
29662972
stepURL = fmt.Sprintf("%s?%s", stepURL, ts.queryParams)
29672973
}
29682974

2969-
var err error
2970-
var resp *http.Response
2971-
2972-
if ts.eventually {
2973-
assert.EventuallyWithT(t, func(tt *assert.CollectT) {
2974-
resp, err = client.Get(stepURL)
2975-
require.NoError(tt, err)
2976-
assert.Equal(tt, ts.expectedStatusCode, resp.StatusCode)
2977-
}, time.Second, 10*time.Millisecond)
2978-
} else {
2979-
resp, err = client.Get(stepURL)
2980-
require.NoError(t, err)
2981-
assert.Equal(t, ts.expectedStatusCode, resp.StatusCode)
2982-
}
2975+
var err error
2976+
var resp *http.Response
2977+
var body []byte
29832978

2984-
body, err := io.ReadAll(resp.Body)
2979+
if ts.eventually {
2980+
assert.EventuallyWithT(t, func(tt *assert.CollectT) {
2981+
localResp, localErr := client.Get(stepURL)
2982+
require.NoError(tt, localErr)
2983+
defer localResp.Body.Close()
2984+
assert.Equal(tt, ts.expectedStatusCode, localResp.StatusCode)
2985+
}, time.Second, 10*time.Millisecond)
2986+
// Make a final request to get the body for assertions
2987+
resp, err = client.Get(stepURL)
2988+
require.NoError(t, err)
2989+
body, err = io.ReadAll(resp.Body)
2990+
require.NoError(t, err)
2991+
require.NoError(t, resp.Body.Close())
2992+
} else {
2993+
resp, err = client.Get(stepURL)
29852994
require.NoError(t, err)
2995+
body, err = io.ReadAll(resp.Body)
2996+
require.NoError(t, err)
2997+
require.NoError(t, resp.Body.Close())
2998+
assert.Equal(t, ts.expectedStatusCode, resp.StatusCode)
2999+
}
29863000

2987-
assert.Contains(t, string(body), ts.expectedBody)
3001+
assert.Contains(t, string(body), ts.expectedBody)
29883002

2989-
if ts.expectedComponentStatus != nil {
2990-
st := &serializableStatus{}
2991-
require.NoError(t, json.Unmarshal(body, st))
2992-
if strings.Contains(ts.queryParams, "verbose") && !strings.Contains(ts.queryParams, "verbose=false") {
2993-
assertStatusDetailed(t, ts.expectedComponentStatus, st)
2994-
continue
2995-
}
2996-
assertStatusSimple(t, ts.expectedComponentStatus, st)
3003+
if ts.expectedComponentStatus != nil {
3004+
st := &serializableStatus{}
3005+
require.NoError(t, json.Unmarshal(body, st))
3006+
if strings.Contains(ts.queryParams, "verbose") && !strings.Contains(ts.queryParams, "verbose=false") {
3007+
assertStatusDetailed(t, ts.expectedComponentStatus, st)
3008+
continue
29973009
}
3010+
assertStatusSimple(t, ts.expectedComponentStatus, st)
29983011
}
2999-
})
3000-
}
3012+
}
3013+
})
3014+
}
30013015
}
30023016

30033017
func assertStatusDetailed(
@@ -3122,23 +3136,29 @@ func TestConfig(t *testing.T) {
31223136
status.NewAggregator(status.PriorityPermanent),
31233137
)
31243138

3125-
require.NoError(t, server.Start(t.Context(), componenttest.NewNopHost()))
3126-
defer func() { require.NoError(t, server.Shutdown(t.Context())) }()
3139+
require.NoError(t, server.Start(t.Context(), componenttest.NewNopHost()))
3140+
defer func() {
3141+
ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
3142+
defer cancel()
3143+
require.NoError(t, server.Shutdown(ctx))
3144+
}()
31273145

3128-
client := &http.Client{}
3129-
url := fmt.Sprintf("http://%s%s", tc.config.Endpoint, tc.config.Config.Path)
3146+
client := &http.Client{}
3147+
defer client.CloseIdleConnections()
3148+
url := fmt.Sprintf("http://%s%s", tc.config.Endpoint, tc.config.Config.Path)
31303149

3131-
if tc.setup != nil {
3132-
tc.setup()
3133-
}
3150+
if tc.setup != nil {
3151+
tc.setup()
3152+
}
31343153

3135-
resp, err := client.Get(url)
3136-
require.NoError(t, err)
3137-
assert.Equal(t, tc.expectedStatusCode, resp.StatusCode)
3154+
resp, err := client.Get(url)
3155+
require.NoError(t, err)
3156+
assert.Equal(t, tc.expectedStatusCode, resp.StatusCode)
31383157

3139-
body, err := io.ReadAll(resp.Body)
3140-
require.NoError(t, err)
3141-
assert.Equal(t, tc.expectedBody, body)
3158+
body, err := io.ReadAll(resp.Body)
3159+
require.NoError(t, err)
3160+
require.NoError(t, resp.Body.Close())
3161+
assert.Equal(t, tc.expectedBody, body)
31423162
})
31433163
}
31443164
}

0 commit comments

Comments
 (0)