[Security Solution] Adds extra cypress and integration tests for setup guide field #180638
Conversation
dplumlee
added
test
release_note:skip
|
/ci |
banderror
added
Feature:Rule Creation
|
@elastic/security-detection-rule-management I wanted to double check the conclusions we came to in the tech time meeting this past wednesday. It was my understanding that for cypress tests, it’d be best to group most of the fields into one exhaustive test (common_flows.cy.ts) to not have a separate group of tests for each field. Are we planning on doing a similar thing with import/export tests where we have one major test that groups together all (or most) of the fields and tests the import/export logic on them? I ask because certain fields (like setup in this case) have no real edge cases in these actions and I know it was mentioned in the meeting as an option. |
| @@ -37,6 +36,5 @@ export const PrebuiltRuleAsset = BaseCreateProps.and(TypeSpecificCreateProps).an | |||
| version: RuleVersion, | |||
| related_integrations: RelatedIntegrationArray.optional(), | |||
| required_fields: RequiredFieldArray.optional(), | |||
| setup: SetupGuide.optional(), | |||
There was a problem hiding this comment.
This is in reference to this comment, so yes if you're adding those fields to BaseDefaultableFields, they should respectively be removed in this type.
There was a problem hiding this comment.
Got it. Removed "required_fields" in my required fields PR.
|
|
||
| it('should import a rule with "setup"', async () => { | ||
| const ndjson = combineToNdJson( | ||
| getCustomQueryRuleParams({ | ||
| setup: '# some setup markdown', | ||
| }) | ||
| ); | ||
|
|
||
| await supertest | ||
| .post(`${DETECTION_ENGINE_RULES_URL}/_import`) | ||
| .set('kbn-xsrf', 'true') | ||
| .attach('file', Buffer.from(ndjson), 'rules.ndjson') | ||
| .expect('Content-Type', 'application/json; charset=utf-8') | ||
| .expect(200); | ||
| }); |
There was a problem hiding this comment.
Interested in what other think here as well, but to answer your question, I think we should create a new describe block that has one test that simply imports a rule with all the fields we want to test for, then read it and asserts that all the fields we defined are present.
I think that should be enough:
const ruleToImport = getCustomQueryRuleParams({
rule_id: 'rule-1',
setup: 'testing',
investigation_fields: // whatever
// all the fields we want to test
// we should continously add fields to
// test in here
});
const ndjson = combineToNdJson(ruleToImport);
await supertest
.post(`${DETECTION_ENGINE_RULES_URL}/_import`)
.set('kbn-xsrf', 'true')
.set('elastic-api-version', '2023-10-31')
.attach('file', Buffer.from(ndjson), 'rules.ndjson')
.expect(200);
const importedRule = await fetchRule(supertest, { ruleId: 'rule-1' });
// Assert al the fields we imported in the rule are there
expect(importedRule).toMatchObject(ruleToImport);Also, we should use the securitySolutionApi that @xcrzx created; but I just noticed it needs fixing for the import endpoint (currently only takes a query)
There was a problem hiding this comment.
And same with exporting a rule
|
/ci |
|
Pinging @elastic/security-detections-response (Team:Detections and Resp) |
|
Pinging @elastic/security-solution (Team: SecuritySolution) |
...s_response/rules_management/rule_import_export/basic_license_essentials_tier/export_rules.ts
Outdated
Show resolved
Hide resolved
| @@ -63,6 +65,24 @@ export default ({ getService }: FtrProviderContext): void => { | |||
| expect(exportedRule).toMatchObject(ruleToExport); | |||
| }); | |||
|
|
|||
| it('should export defaultable fields when values are set', async () => { | |||
There was a problem hiding this comment.
Nit:
| it('should export defaultable fields when values are set', async () => { | |
| it('exports defaultable fields when non-default values are set', async () => { |
...s_response/rules_management/rule_import_export/basic_license_essentials_tier/import_rules.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Test changes LGTM. They are consistent with tests in @maximpn's "related integrations" PR. Now we'll just need to wait until Maxim merges his PR after the April 29th Serveless release and resolve (hopefully) a few conflicts.
|
FYI if tests in this PR depend on #178295 which targets 8.15.0, there's a chance it would be hard to backport this PR to 8.14. |
💚 Build Succeeded
Metrics [docs]
History
To update your PR or re-run it, just comment with: cc @dplumlee |
Summary
Adds extra tests to cover remaining areas not addressed in #178131
Adds cypress tests and adds
setupfield to utils to be used in import/export integration testsFlaky test run (internal)
Checklist
Delete any items that are not applicable to this PR.
For maintainers