-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[receiver/prometheusremotewrite] flaky test #37563
base: main
Are you sure you want to change the base?
Conversation
req := httptest.NewRequest(http.MethodPost, "/api/v1/write", bytes.NewBuffer(compressedBody)) | ||
req.Header.Set("Content-Encoding", "snappy") | ||
req.Header.Set("Content-Type", "") | ||
w := httptest.NewRecorder() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ArthurSens It's not done yet. It's just an idea. Did you see any problem to use httptest
instead of call setupServer
? Like you did here ⬇️
opentelemetry-collector-contrib/receiver/prometheusremotewritereceiver/receiver_test.go
Lines 74 to 75 in d74ba54
func TestHandlePRWContentTypeNegotiation(t *testing.T) { | |
setupServer(t) |
httptest take care of a bunch of things. Including the creation of a valid base url and ports. For that specific case where we are just validating the expected statusCode
given a header, I think that would be a good approach to avoid the flaky test thing
. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pjanotti some constraint about this approach?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I like your approach! Let's do it!
.chloggen/prw-flaky-test.yaml
Outdated
@@ -0,0 +1,27 @@ | |||
# Use this changelog template to create an entry for release notes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know I asked you to add this changelog entry, but I think the correct option is to add the skip changelog
label since the change isn't related to the collector user
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I remove it, then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes please
/label "Skip Changelog" not sure if this will work, let's see |
Yeah, I don't have it either; we need to wait for an approver/maintainer. All that's left to do here is remove the draft :) |
Description
Using httptest package instead of starting a real server avoiding port conflicts.
Link to tracking issue
Fixes #36654