[Security Solution][API testing][Rule Management] Move and restructures remaining rule managements trail-tests #172173
Conversation
WafaaNasr
added
release_note:skip
…ructure-remaining-rulemanagmenttests
…/github.com/WafaaNasr/kibana into move-structure-remaining-rulemanagmenttests
…remaining-rulemanagmenttests
yctercero
added
Team:Detection Rule Management
There was a problem hiding this comment.
This was very clearly a huge effort; thank you so much for your diligence here.
I had a few questions about test/script/report names, similar to on @yctercero's PR. I assume the inconsistency is due to these efforts being done in parallel, but perhaps it's worth agreeing to a standard naming convention.
I tried my best to track where all the tests/helpers went (I appreciated the table, of course); but on one of the first (create_container_with_exception_entries), that did not have a corresponding test, and seemed to be part of a whole area of dead code? I didn't see that mentioned in the description so I'm not positive; it would be a good callout to explain a bunch of deleted code without associated deletion of its use. But: excellent work identifying and deleting that, if that's the case :).
The concurrent renames + file changes make this branch messier than usual, but since it's entirely tests I think the only real risk here is that we accidentally delete some critical assertion. With that in mind, I was unable to find any such deletions.
LGTM.
| ...functionalConfig.getAll(), | ||
| testFiles: [require.resolve('..')], | ||
| junit: { | ||
| reportName: 'Rule Bulk Actions API Integration Tests - ESS - Rule bulk actions logic', |
There was a problem hiding this comment.
This is a little redundant, and also not all of the info? Perhaps you and @yctercero should agree to a template to be used for these, for consistency?
| reportName: 'Rule Bulk Actions API Integration Tests - ESS - Rule bulk actions logic', | |
| reportName: 'Rule Bulk Actions - API Integration Tests - ESS - Default License', |
| export default ({ getService }: FtrProviderContext): void => { | ||
| const supertest = getService('supertest'); | ||
| const esArchiver = getService('esArchiver'); | ||
| const log = getService('log'); | ||
| const es = getService('es'); | ||
|
|
||
| describe('create_rules_bulk', () => { | ||
| // Marking as ESS and brokenInServerless as it's currently exposed in both, but if this is already |
There was a problem hiding this comment.
Do you want to create a ticket to remove this endpoint? I'd say for now, if the endpoint exists and returns that deprecation header, we should verify it in each environment.
| @@ -75,7 +75,7 @@ export default ({ getService }: FtrProviderContext): void => { | |||
| }); | |||
|
|
|||
| beforeEach(async () => { | |||
| await createSignalsIndex(supertest, log); | |||
| await createAlertsIndex(supertest, log); | |||
There was a problem hiding this comment.
I appreciate these changes, but I wonder how much smaller the diff would've been had we chosen to do them in a separate/subsequent PR ...
There was a problem hiding this comment.
With git we usually have to make that decision at the commit level, (i.e. it's better to rename a file in one commit and make changes in a subsequent one, so as to better allow git to recognize the rename); our squashing strategy for PRs means we can (and probably should) be making that decision at the PR level.
Not to say that you didn't, or that this wasn't the correct decision here: just pontificating.
There was a problem hiding this comment.
That's a good call @rylnd and something we can keep in mind moving forward.
Note to self: something we can add in the PR guideline docs as "good practice".
There was a problem hiding this comment.
Yeah, this is an example of git getting really confused 😆
| const config = getService('config'); | ||
| const ELASTICSEARCH_USERNAME = config.get('servers.kibana.username'); | ||
|
|
||
| // Marking as ESS and brokenInServerless as it's currently exposed in both, but if this is already |
There was a problem hiding this comment.
Ditto on this one. @yctercero should we add a ticket to capture removing these deprecated APIs? Is that something we can/want to do for serverless only?
There was a problem hiding this comment.
Looks like there's an existing old ticket - I'm updating this comment to reference it and enable for serverless as we currently are exposing it there - #130963
|
|
||
| // Test to ensure that we have exactly 0 legacy actions by querying the Alerting client REST API directly | ||
| // See: https://www.elastic.co/guide/en/kibana/current/find-rules-api.html | ||
| // Note: We specifically query for both the filter of type "siem.notifications" and the "has_reference" to keep it very specific |
There was a problem hiding this comment.
Nit: "keep it very specific" is not very specific 😄 . Maybe:
| // Note: We specifically query for both the filter of type "siem.notifications" and the "has_reference" to keep it very specific | |
| // Note: We specifically filter for both the type "siem.notifications" and the "has_reference" field to ensure we only retrieve legacy actions |
| export default ({ getService }: FtrProviderContext): void => { | ||
| const supertest = getService('supertest'); | ||
| const log = getService('log'); | ||
| const es = getService('es'); | ||
|
|
||
| describe('delete_rules', () => { | ||
| describe('@ess delete_rules_legacy', () => { |
There was a problem hiding this comment.
Nit: the test description is another opportunity to provide detail/context to the reader. Repeating the name of the file does not add new information.
Of course I understand that you don't necessarily have the original context for this file, and there are a ton of files changed here, and that it originally did not do this. However, even this would be an improvement:
| describe('@ess delete_rules_legacy', () => { | |
| describe('@ess Delete Rules - Legacy Behaviors', () => { |
There was a problem hiding this comment.
As I'm addressing the feedback, I'll look to improve on any test descriptions.
..._integration/test_suites/detections_response/default_license/rule_execution_logic/runtime.ts
Show resolved
Hide resolved
| const rule = await fetchRule(supertest, { ruleId: 'rule-1' }); | ||
| expect(rule.investigation_fields).to.eql({ field_names: ['foo', 'bar'] }); | ||
| /** | ||
| * Confirm type on SO so that it's clear in the tests whether it's expected that |
There was a problem hiding this comment.
You seem to have repeated this a few times; maybe it's worth extracting this into a helper function so that you can document it once?
There was a problem hiding this comment.
Creating a util for it. 👍🏽
|
@yctercero Could you please rebase the PR? It's now behind |
There was a problem hiding this comment.
@yctercero @WafaaNasr Absolutely humongous work! I love the table in the PR description - it is super informative and clear, and will help me a lot with understanding what to do with Rule Management integration tests next.
I do have some suggestions and questions which I'd like to discuss with you @yctercero once you have some time for that. The main concern is the folder structure, I think. Could you please respond to comments and then we maybe could discuss them over zoom? Also, like in the other PR (#172173), I echo @rylnd's comments.
That all said, the size of this PR is crazy and for the sake of reducing the pain let's merge it as is and address comments in follow-up PRs.
LGTM 👍
| // Marking as ESS and brokenInServerless as it's currently exposed in both, but if this is already | ||
| // deprecated, it should cease being exposed in Serverless prior to GA, in which case this | ||
| // test would be run for ESS only. | ||
| describe('@ess @brokenInServerless @skipInQA create_rules_bulk', () => { |
There was a problem hiding this comment.
Is it really broken in Serverless, or you just skipped it in Serverless this way?
There was a problem hiding this comment.
I'm updating to test in serverless too. I'll add a note if it is broken in serverless.
| // TODO: add a new service | ||
| const config = getService('config'); |
There was a problem hiding this comment.
What does this mean? Can we please elaborate or remove this comment?
There was a problem hiding this comment.
It refers to adding a util for pulling in esarchiver, similar to something like getService('loadEsArchiver'). Instead, right now, it requires a couple of lines each time. I think it's ok to leave in, I'm just expanding on the comment in the code.
@WafaaNasr when you're back, maybe we can just create a ticket to track this and update the comment to reference the ticket with details.
| const expectedRule = updateUsername(getSimpleRuleOutput(), ELASTICSEARCH_USERNAME); | ||
|
|
||
| expect(bodyToCompare).to.eql(expectedRule); |
There was a problem hiding this comment.
In the cases where we have certain dynamic data in objects under test, like here with the created_by and updated_by fields not matching our mocks, we could exclude these fields from the mocks and assertions by using expect.objectContaining and (if we want) we could assert that these fields are being updated by the server on rule deletion in a separate test.
I'd highly encourage using expect.objectContaining and only asserting what is needed to be asserted according to the logic of a test, instead of adding/removing properties from objects under test as we do in removeServerGeneratedProperties and now in updateUsername.
More context: https://github.com/elastic/security-team/issues/3526#issuecomment-1096724323
There was a problem hiding this comment.
That would be great to add to the planned docs around FTRs and cypress guidelines. I think it's a bit outside the scope of moving the FTRs here, but definitely an improvement we can track.
| // TODO: add a new service | ||
| const config = getService('config'); |
There was a problem hiding this comment.
So I see 32 such comments in 32 different files. I'd suggest to remove them.
There was a problem hiding this comment.
It refers to adding a util for pulling in esarchiver, similar to something like getService('loadEsArchiver'). Instead, right now, it requires a couple of lines each time. I think it's ok to leave in, I'm just expanding on the comment in the code.
@WafaaNasr when you're back, maybe we can just create a ticket to track this and update the comment to reference the ticket with details.
| describe('@ess import_rules - ESS specific logic', () => { | ||
| beforeEach(async () => { | ||
| await deleteAllRules(supertest, log); | ||
| }); | ||
|
|
||
| it('should migrate legacy actions in existing rule if overwrite is set to true', async () => { |
There was a problem hiding this comment.
Tests covering legacy actions are scattered across many files that contain tests for various rules CRUD and other endpoints. I think we should extract them into their own group of tests, and this whole group would:
- run only in ESS
- be owned by the Detection Engine team
Not suggesting to do it in this PR, of course - let's do it separately 🙂
|
|
||
| describe('update_rules', () => { | ||
| describe('@ess @serverless @skipInQA update_rules', () => { |
There was a problem hiding this comment.
@skipInQA: Do you know what's our plan for enabling tests in MKI environments?
| /x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/rule_delete @elastic/security-detection-rule-management | ||
| x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/rule_update @elastic/security-detection-rule-management | ||
| /x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/rule_patch @elastic/security-detection-rule-management | ||
| /x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/rule_import_export @elastic/security-detection-rule-management | ||
| /x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/rule_management @elastic/security-detection-rule-management | ||
| /x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/rule_bulk_actions @elastic/security-detection-rule-management | ||
| /x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/rule_read @elastic/security-detection-rule-management |
There was a problem hiding this comment.
Not a big fan of the folder structure proposed in this PR.
I think these are too many folders to track them all separately in CODEOWNERS. It would make it much easier for both code navigation and CODEOWNERS if we grouped these test groups into higher-level folders reflecting our subdomains. In this case, all these folders could be under a rule_management folder so we'd only need to add a single line to this file here:
/x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/rule_management @elastic/security-detection-rule-management
For the sake of having less pain I'd suggest to merge this PR as is and restructure these folders in a follow-up one.
There was a problem hiding this comment.
Copying from other comment:
@banderror let's definitely chat! I plan to address the comments that don't deal with restructuring first, and then open a PR dedicated to any structure updates. For the ownership on the endpoints we largely followed what we saw in the folders structure in the code itself (these routes are under rule_management, but it definitely felt a bit confusing.
I'll find a time to sync!
| /x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/rule_management @elastic/security-detection-rule-management | ||
| /x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/rule_bulk_actions @elastic/security-detection-rule-management | ||
| /x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/rule_read @elastic/security-detection-rule-management | ||
| /x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/rule_creation/create_rules_bulk.ts @elastic/security-detection-rule-management |
There was a problem hiding this comment.
Why do we add a single file to CODEOWNERS? We shouldn't target individual files in CODEOWNERS because this can get out of sync with actual files pretty quickly.
There was a problem hiding this comment.
Copying from other comment:
@banderror let's definitely chat! I plan to address the comments that don't deal with restructuring first, and then open a PR dedicated to any structure updates. For the ownership on the endpoints we largely followed what we saw in the folders structure in the code itself (these routes are under rule_management, but it definitely felt a bit confusing.
I'll find a time to sync!
| /x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/rule_creation/create_new_terms @elastic/security-detection-engine | ||
| /x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/rule_creation/create_rules @elastic/security-detection-engine | ||
| /x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/rule_creation/preview_rules @elastic/security-detection-engine |
There was a problem hiding this comment.
We shouldn't target individual files in CODEOWNERS because this can get out of sync with actual files pretty quickly.
There was a problem hiding this comment.
Copying from other comment:
@banderror let's definitely chat! I plan to address the comments that don't deal with restructuring first, and then open a PR dedicated to any structure updates. For the ownership on the endpoints we largely followed what we saw in the folders structure in the code itself (these routes are under rule_management, but it definitely felt a bit confusing.
I'll find a time to sync!
| x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/rule_update/update_rules.ts @elastic/security-detection-engine | ||
| x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/rule_update/update_rules_ess.ts @elastic/security-detection-engine | ||
| x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/rule_patch/patch_rules.ts @elastic/security-detection-engine | ||
| x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/rule_patch/patch_rules_ess.ts @elastic/security-detection-engine | ||
| /x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/rule_delete/delete_rules.ts @elastic/security-detection-engine | ||
| /x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/rule_delete/delete_rules_ess.ts @elastic/security-detection-engine |
There was a problem hiding this comment.
Same here regarding targeting individual files.
Also, I'd like to chat with you @yctercero about which team should own which rules CRUD tests. I don't understand the proposed separation between our two teams, and I have some thoughts about how we should do it.
There was a problem hiding this comment.
@banderror let's definitely chat! I plan to address the comments that don't deal with restructuring first, and then open a PR dedicated to any structure updates. For the ownership on the endpoints we largely followed what we saw in the folders structure in the code itself (these routes are under rule_management, but it definitely felt a bit confusing.
I'll find a time to sync!
…remaining-rulemanagmenttests
|
@rylnd @banderror acknowledging all the comments here. Will follow up merge with a PR to address all comments. Thanks so much for reviewing the huge PR. |
💚 Build Succeeded
Metrics [docs]
History
To update your PR or re-run it, just comment with: cc @WafaaNasr |
kibanamachine
added
v8.13.0
backport:skip
## Summary Addressing feedback from #172173. This is not yet addressing file restructure, going to keep the file restructure in a separate PR to try to make reviews more readable.
Summary
rule_edit,rule_delete,rule_management,rule_import_exportandrule_readundersecurity_solution_api_integrationsecurity_solution_api_integration. Files that were not actively used in the previous folder were moved, while any duplicate files remained in their original positions.❓ - Deprecated and should not be exposed in serverless, but is currently
❌ - Some or all tests in suite are broken in serverless