-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
ci: enable windows for testing heartbeat #32937
Conversation
None of this is supported or works there, so we should just disable it. Fixes elastic#32937 No release notes necessary since there are no user facing changes
…2939) None of this is supported or works there, so we should just disable it. Fixes elastic#32937 No release notes necessary since there are no user facing changes
/test |
Apologies, I missed some build tags, mind if I push them directly here to make testing them easier? |
Yes, please, contribute to this PR if needed 👍 |
…ts' into fix-remaining-win-issues
I hope you don't mind @v1v , I've pushed directly to here to see if I can resolve the test failures with a few more build tags |
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.
LGTM
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.
Well, that was premature, I thought I'd seen them all passing
…st that will come back in an improved form in elastic#30632
@@ -70,30 +69,5 @@ var Scenarios = &ScenarioDB{ | |||
}, func() {}, nil | |||
}, | |||
}, | |||
{ | |||
Name: "simple-browser", |
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.
Temporarily remove browser test from scenarios. This is a very new test that will come
back in an improved form in #30632
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.
LGTM now
I can see some errors in the logs:
I just compared with a different build for a different component and similar outcome but in this case it ran some python tests
I don't know if those errors are genuine or flaky... and what's the reason the mage test does not fail, any hints? |
@andrewvc , what's your feedback for #32937 (comment) ? It's the expected behaviour? |
@v1v that's a bit of a mystery to me TBH, we don't do any python stuff outside of the standard tasks defined in libbeat's mage helpers. I would have assumed that error would be present for all beats on windows if it's present for us. Are you certain other beats are running Running locally on my win box (which I never normally do) I get a different, but similar error on metricbeat and heartbeat:
They're both running pip commands. TBH, I would like to keep those disabled on windows, the value those tests add is minimal, not nearly high enough to justify the maintenance cost, esp. given that we've never had a windows specific bug in many years. |
Yes, with similar outcome but running some tests afterwards, for instance, https://beats-ci.elastic.co/blue/organizations/jenkins/Beats%2Fbeats%2FPR-32937/detail/PR-32937/6/pipeline/1446#step-2249-log-15 for I assume, since it happens for other components it's okey to merge this PR. Do you want me to do it now? |
Yes, ++ on merging |
(cherry picked from commit 8552e34) # Conflicts: # x-pack/heartbeat/Jenkinsfile.yml # x-pack/heartbeat/monitors/browser/source/project.go # x-pack/heartbeat/monitors/browser/source/project_test.go # x-pack/heartbeat/monitors/browser/source/source.go # x-pack/heartbeat/scenarios/basics_test.go # x-pack/heartbeat/scenarios/scenarios.go
(cherry picked from commit 8552e34) # Conflicts: # x-pack/heartbeat/monitors/browser/source/source.go
This reverts commit 8552e34.
None of this is supported or works there, so we should just disable it. Fixes #32937 No release notes necessary since there are no user facing changes
What does this PR do?
Enable testing for
heartbeat
onwindows
.Why is it important?
It was disabled since some tests were failing, and reported those tests but no further actions were done.
Let's enable back so the @elastic/uptime can figure out what's going on