-
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
[Security Solution] fix flaky endpoint ftr tests #152119
Conversation
c8dfa91
to
ee7d39e
Compare
ee7d39e
to
cfaa11c
Compare
Pinging @elastic/security-defend-workflows (Team:Defend Workflows) |
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.
looks good, would like to see one thing cleaned up
@@ -532,7 +532,7 @@ export class Plugin implements ISecuritySolutionPlugin { | |||
); | |||
|
|||
plugins.fleet?.fleetSetupCompleted().then(() => { | |||
if (plugins.taskManager) { | |||
if (plugins.taskManager && process.env.BUILDKITE !== 'true') { |
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.
End goal is good here, we don't need the watchdog running in the test environment (esp. if there are tests verifying watchdog behavior).
But good practice is to keep all knowledge of tests and test environments outside of the "production side" of the code. Probably a few ways to achieve this, but see if you can reorganize so you can control this from a .test.ts
file.
Could either try to hook the plugin start earlier/somewhere else and overwrite/inject the checkMetadataTransformTask
to be a mock or no-op so this code block still runs and does nothing.
Or if thats not feasible, could try to make a global or imported variable you can replace this check with like
if (plugins.taskManager && RUN_WATCHDOG)
and by default thats just set to RUN_WATCHDOG=true
somewhere in production code. And from the test side, give yourself the ability to mock the import or otherwise set to false.
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.
great point. since this is blocking all of our FTR tests and the retry logic alone is sufficient for restoring our tests, I'll remove this logic from this PR and work on it as a separate PR.
Pinging @elastic/fleet (Team:Fleet) |
cfaa11c
to
930784e
Compare
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.
Fleet change LGTM
💚 Build Succeeded
Metrics [docs]Unknown metric groupsESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
## Summary Resolves #150343. Similar to #153612, mitigated by #152119 and elastic/endpoint-package#353. Flaky test runner x 200 ✅ https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/2041
Summary
Fixes flaky endpoint FTR tests.
restrictwill revisit in separate PR (see comment)CheckMetadataTransformsTask
from running during tests so that it doesn't interfere with expected test transform setupsFlaky test runs:
Fixes: #72874
For maintainers