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

[Security Solution] fix flaky endpoint ftr tests #152119

Merged
merged 1 commit into from
Feb 28, 2023

Conversation

joeypoon
Copy link
Member

@joeypoon joeypoon commented Feb 24, 2023

Summary

Fixes flaky endpoint FTR tests.

  • restrict CheckMetadataTransformsTask from running during tests so that it doesn't interfere with expected test transform setups will revisit in separate PR (see comment)
  • retry transform start during transform install

Flaky test runs:

Fixes: #72874

For maintainers

@joeypoon joeypoon force-pushed the fix/flaky-tests branch 10 times, most recently from c8dfa91 to ee7d39e Compare February 28, 2023 00:03
@joeypoon joeypoon added release_note:skip Skip the PR/issue when compiling release notes Team:Defend Workflows “EDR Workflows” sub-team of Security Solution labels Feb 28, 2023
@joeypoon joeypoon changed the title [WIP][Security Solution] fix flaky endpoint ftr tests [Security Solution] fix flaky endpoint ftr tests Feb 28, 2023
@joeypoon joeypoon marked this pull request as ready for review February 28, 2023 03:39
@joeypoon joeypoon requested review from a team as code owners February 28, 2023 03:39
@joeypoon joeypoon requested review from pzl and gergoabraham February 28, 2023 03:39
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-defend-workflows (Team:Defend Workflows)

Copy link
Member

@pzl pzl left a 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') {
Copy link
Member

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.

Copy link
Member Author

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.

@botelastic botelastic bot added the Team:Fleet Team label for Observability Data Collection Fleet team label Feb 28, 2023
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

Copy link
Contributor

@juliaElastic juliaElastic left a comment

Choose a reason for hiding this comment

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

Fleet change LGTM

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Unknown metric groups

ESLint disabled line counts

id before after diff
securitySolution 428 430 +2

Total ESLint disabled count

id before after diff
securitySolution 506 508 +2

History

  • 💚 Build #110930 succeeded cfaa11ce0f1825247da03d4967a14f8383f43708
  • 💔 Build #110894 failed c8dfa91b0e7bff3627f7cba88689bd947b69a6bf
  • 💚 Build #110873 succeeded 7900af52a863c84e9eab52c87a3257d2c92a8c63
  • 💛 Build #110760 was flaky 48454eb1f84bca12ffc9e04962489c7e474e05ae
  • 💔 Build #110729 failed c9c229e0d2c7aee17a0351ee4003ba4d8b1d9789
  • 💔 Build #110474 failed 8a94b6561e795d6c6659f7f615644f5a08b7f44a

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@joeypoon joeypoon merged commit 7449b0a into elastic:main Feb 28, 2023
@joeypoon joeypoon deleted the fix/flaky-tests branch February 28, 2023 23:29
@kibanamachine kibanamachine added v8.8.0 backport:skip This commit does not require backporting labels Feb 28, 2023
bmorelli25 pushed a commit to bmorelli25/kibana that referenced this pull request Mar 10, 2023
jen-huang added a commit that referenced this pull request Mar 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:Defend Workflows “EDR Workflows” sub-team of Security Solution Team:Fleet Team label for Observability Data Collection Fleet team v8.8.0
Projects
None yet
6 participants