-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Attempt to fix flaky test by adding host.mac to all the fake_hosts documents #178648
Attempt to fix flaky test by adding host.mac to all the fake_hosts documents #178648
Conversation
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
@@ -91,6 +91,7 @@ export const generateEvent: GeneratorFunction = (config, schedule, index, timest | |||
'@timestamp': timestamp.toISOString(), | |||
host: { | |||
name: `host-${index}`, | |||
mac: ['00-00-5E-00-53-23', '00-00-5E-00-53-24'], |
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.
@simianhacker I've added host.mac
to all the docs in fake_hosts, please let me know if you see an issue.
Thanks for pointing to this fix!
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.
@maryam-saeidi I created a flaky-test-runner job for this PR and see how it goes
https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5478
@maryam-saeidi do you know why it expected |
Thanks, but I don't think we need one for this PR, I created one in the other PR and this issue happens rarely (only |
It is mentioned in the error message of the related issue: |
True, but the question is why it is only failing sometimes. Were you able to reproduce the test failure locally? |
The hypothesis is that during adding context variables, it uses a document that does not have |
@maryam-saeidi thanks for clarifying. I wander if keeping documents with varying fields was intentional (e.g. to replicate real-life scenario). I suspect the recent test failures are due to inconsistencies in source data documents. For "rule is active" test failure also, I saw some documents have To me it looks like, the fields should be consistent in all documents for specific instance like @simianhacker do you know if having some documents with certain fields like |
@simianhacker pointed me to this fix, and I added a comment to verify that this change is fine (which I think it should be). |
@maryam-saeidi I had seen the comment about the fix. This PR looks good. My question to @simianhacker was rather general, to see if it makes sense to improve all test data with consistent fields. But we can discuss it offline.
I will check the test locally. |
💚 Build Succeeded
Metrics [docs]
History
To update your PR or re-run it, just comment with: |
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.
The flaky test runner passed. I approve the PR. But I would wait for Chris input for this #178648 (comment)
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!
Fixes #178578
The hypothesis is that during adding context variables, it uses a document that does not have
host.mac
, and in some of thefake_host
documents we have this condition, so I fixed that.