Skip to content
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

[chore] add more Windows versions for testing #37025

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

CodePrometheus
Copy link

Description

Link to tracking issue

Fixes #37024

Testing

Documentation

@CodePrometheus CodePrometheus requested a review from a team as a code owner January 6, 2025 02:29
@songy23 songy23 added ci-cd CI, CD, testing, build issues os:windows Run Windows Enable running windows test on a PR labels Jan 6, 2025
@songy23 songy23 requested a review from atoulme January 6, 2025 14:11
@atoulme
Copy link
Contributor

atoulme commented Jan 6, 2025

@pjanotti would you please review? Some CI failures are happening for both 2022 and 2025. I also see main failing.

@songy23 songy23 requested a review from pjanotti January 6, 2025 18:39
@pjanotti
Copy link
Contributor

pjanotti commented Jan 6, 2025

Group receiver-3 seems to be failing consistently on main too. I still have to look at the other failure (at first not happening in main).

@songy23
Copy link
Member

songy23 commented Jan 6, 2025

Test failures are unrelated to this PR: #10151, #36917

@pjanotti
Copy link
Contributor

pjanotti commented Jan 6, 2025

statsdreceiver failures are related to PR #36608 which introduces Unix Datagram support for the receiver, but, didn't skip the new tests on Windows.

@pjanotti
Copy link
Contributor

pjanotti commented Jan 6, 2025

Fix for statsdreceiver: #37036

@pjanotti
Copy link
Contributor

pjanotti commented Jan 6, 2025

#36923 fixes the other failures for receiver-3 group.

@pjanotti
Copy link
Contributor

pjanotti commented Jan 6, 2025

The error in exporter-3 group seems, at first, a concurrent behavior slightly different on Windows 2025. It seems that the test itself should ensure that the server used in the test is operational before blasting the concurrent requests.

@pjanotti
Copy link
Contributor

pjanotti commented Jan 6, 2025

@CodePrometheus apply the following patch to your change to fix the exporter-3 group failures (assuming that my hypothesis is right)

diff --git a/exporter/prometheusremotewriteexporter/exporter_concurrency_test.go b/exporter/prometheusremotewriteexporter/exporter_concurrency_test.go
index 689cbcf7b9..14fc6364c4 100644
--- a/exporter/prometheusremotewriteexporter/exporter_concurrency_test.go
+++ b/exporter/prometheusremotewriteexporter/exporter_concurrency_test.go
@@ -52,6 +52,11 @@ func Test_PushMetricsConcurrent(t *testing.T) {
 			t.Fatal(err)
 		}
 		assert.NotNil(t, body)
+		if len(body) == 0 {
+			// No content, nothing to do. The request is just checking that the server is up.
+			return
+		}
+
 		// Receives the http requests and unzip, unmarshalls, and extracts TimeSeries
 		assert.Equal(t, "0.1.0", r.Header.Get("X-Prometheus-Remote-Write-Version"))
 		assert.Equal(t, "snappy", r.Header.Get("Content-Encoding"))
@@ -124,6 +129,12 @@ func Test_PushMetricsConcurrent(t *testing.T) {
 		require.NoError(t, prwe.Shutdown(ctx))
 	}()
 
+	// Ensure that the test server is up before making the requests
+	assert.EventuallyWithT(t, func(c *assert.CollectT) {
+		_, checkRequestErr := http.Get(server.URL)
+		assert.NoError(c, checkRequestErr)
+	}, 5*time.Second, 100*time.Millisecond)
+
 	var wg sync.WaitGroup
 	wg.Add(n)
 	for _, m := range ms {

@CodePrometheus CodePrometheus force-pushed the add-more-archs-for-windows branch from 537492c to c855d73 Compare January 7, 2025 02:29
@pjanotti pjanotti added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Jan 7, 2025
strategy:
fail-fast: false
matrix:
os: [windows-2022, windows-2025]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually this one is fine to use windows-latest since it is building the binary, we should be fine with windows-latest - it also breaks because it tries to upload two artifacts with the same name.

@pjanotti
Copy link
Contributor

pjanotti commented Jan 7, 2025

@CodePrometheus (or someone with the rights) could you please merge latest main to clean up the CI failures that were unrelated to this PR?

@pjanotti
Copy link
Contributor

pjanotti commented Jan 7, 2025

It seems that my suggestion at #37025 (comment) wasn't enough to fix the concurrency issue on Windows 2025... 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci-cd CI, CD, testing, build issues exporter/prometheusremotewrite os:windows Run Windows Enable running windows test on a PR Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add testing for Windows 2025
4 participants