-
Notifications
You must be signed in to change notification settings - Fork 8k
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
[Detections Response] Finish moving remaining legacy FTRs #175837
Conversation
Pinging @elastic/security-detections-response (Team:Detections and Resp) |
Pinging @elastic/security-detection-rule-management (Team:Detection Rule Management) |
Pinging @elastic/security-solution (Team: SecuritySolution) |
Pinging @elastic/security-detection-engine (Team:Detection Engine) |
Thank you for compiling this info into a table @yctercero, this will be very helpful in the future! Once the PR is merged, I'll open follow-up tickets for @elastic/security-detection-rule-management to revisit and unskip the tests that were marked as |
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.
Restructuring tests is a huge undertaking, I really appreciate all the work you've done in this PR, in #175417, and other prior PRs. Thank you so much @yctercero!
The PR LGTM 👍 I left some concerns that I'm ok to address in follow-up PRs myself. Nothing blocking from my side.
I will defer approving it to @maximpn since he was assigned as a reviewer.
...ctions_response/rules_management/rule_creation/basic_license_essentials_tier/create_rules.ts
Show resolved
Hide resolved
...ctions_response/rules_management/rule_creation/basic_license_essentials_tier/create_rules.ts
Show resolved
Hide resolved
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.
@yctercero thank you for moving FTR tests in the new folder 🙏
I totally agree with Georgii's comment https://github.com/elastic/kibana/pull/175837/files#r1472814985 but I don't we should address all that issues in this PR. It should be focused on moving tests with minimal changes so it doesn't block any other PR requiring changing/adding FTR tests. The issues mentioned should be addressed in follow up PRs.
I see you made some little changes by using updateUsername()
and similar. It doesn't look to be necessary (correct me if I'm wrong here) for this PR. Ideally we need to get rid of removeServerGeneratedProperties()
, updateUsername()
, getSimpleRuleOutputWithoutRuleId()
and etc in favor of Jest expect
with partial matching like expect().toMatchObject()
and objectContaining
to focus on the changes the test does. It's a huge amount of work so it looks logical do this refactoring only when a test touched (updated/fixed) or a new test added.
As I don't see any critical changes required I approve this PR.
x-pack/plugins/security_solution/common/experimental_features.ts
Outdated
Show resolved
Hide resolved
@elasticmachine merge upstream |
…o move_remaining_ftrs
…o move_remaining_ftrs
@elasticmachine merge upstream |
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.
I had a few recommendations for cleaning up some of the cruft introduced by the expansion of these FTRs into the serverless era, but they were mostly just resurrecting @banderror's previous insights 😅 .
Kudos on finding and calling out the test duplication as well; that's easily overlooked. However, since these PRs are not introducing the duplication (rather, they're just not fixing it), I would agree that it can be fixed down the line.
Thanks for the helpful PR description and comment responses; this one was a lot so those are very much appreciated :)
export default ({ getService }: FtrProviderContext): void => { | ||
const esArchiver = getService('esArchiver'); | ||
const supertest = getService('supertest'); | ||
const log = getService('log'); | ||
const es = getService('es'); | ||
|
||
describe('create_rules_bulk', () => { | ||
// TODO: add a new service for loading archiver files similar to "getService('es')" |
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.
I agree with georgii that we probably don't need to keep adding this comment, but I appreciate the commitment!
@@ -51,7 +55,9 @@ export default ({ getService }: FtrProviderContext): void => { | |||
.expect(200); | |||
|
|||
const bodyToCompare = removeServerGeneratedProperties(body[0]); | |||
expect(bodyToCompare).to.eql(getSimpleRuleOutput()); | |||
const expectedRule = updateUsername(getSimpleRuleOutput(), ELASTICSEARCH_USERNAME); |
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.
Seeing all these updateUsername
s added here doesn't feel great; I'll reiterate @banderror's previous comment pointing out that we now modify both the actual value and the expected value, which does seem a little silly.
I really like expect.objectContaining
as referenced, but it can definitely get a little verbose and hard to read; maybe we could hide some of that verbosity in a declarative matcher? It's not obvious how one would go about adding a matcher to kbn-expect, but we could certainly just write a function that compares the appropriate fields and ignores the rest:
assertRuleEquality({ actual: body[0], expected: getSimpleRuleOutput() })
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.
I'll look to create some follow up issues to track these suggestions!
@elasticmachine merge upstream |
💚 Build Succeeded
Metrics [docs]
History
To update your PR or re-run it, just comment with: cc @yctercero |
…5837) **Resolves: elastic#151902 ## Summary After this PR, all D&R FTRs are moved to new folder where they can be run in ESS and serverless. Please see below table for a summary of what tests need revisiting by the teams. During the test migration there may have been some tests that failed on serverless, but not ESS. Some we were able to fix and get running on both, others are still marked as `brokenInServerless` and need triage.
…5837) **Resolves: elastic#151902 ## Summary After this PR, all D&R FTRs are moved to new folder where they can be run in ESS and serverless. Please see below table for a summary of what tests need revisiting by the teams. During the test migration there may have been some tests that failed on serverless, but not ESS. Some we were able to fix and get running on both, others are still marked as `brokenInServerless` and need triage.
…5837) **Resolves: elastic#151902 ## Summary After this PR, all D&R FTRs are moved to new folder where they can be run in ESS and serverless. Please see below table for a summary of what tests need revisiting by the teams. During the test migration there may have been some tests that failed on serverless, but not ESS. Some we were able to fix and get running on both, others are still marked as `brokenInServerless` and need triage.
Resolves: #151902
Summary
After this PR, all D&R FTRs are moved to new folder where they can be run in ESS and serverless. Please see below table for a summary of what tests need revisiting by the teams. During the test migration there may have been some tests that failed on serverless, but not ESS. Some we were able to fix and get running on both, others are still marked as
brokenInServerless
and need triage.Detection Engine FTRs
brokenInServerless
brokenInServerless
Rules Management FTRs
brokenInServerless
brokenInServerless
brokenInServerless
brokenInServerless
brokenInServerless
brokenInServerless
brokenInServerless
Shared D&R FTRs
brokenInServerless