[Detections Response][FTR] Move remaining basic license FTRs to new folder by yctercero · Pull Request #172132 · elastic/kibana

yctercero · 2023-11-29T08:05:14Z

Summary

Following the initial work in this [Security Solution][Detection Engine] Running API tests in Serverless & ESS using Mocha Tagging #166755
Addresses part of [Security Solution] Restructure API integration tests into meaningful groups that run in parallel on CI #151902 for Rule management related tests

Files moved from old structure to new folders

`detection_engine_api_integration`	`security_solution_api_integration`
`/basic/tests/create_rules_bulk.ts`	`/test_suites/detections_response/basic_essentials_license/rule_creation/`
`/basic/tests/delete_rules.ts`	`/test_suites/detections_response/basic_essentials_license/rule_delete/`
`/basic/tests/delete_rules_bulk.ts`	`/test_suites/detections_response/basic_essentials_license/rule_delete/`
`/basic/tests/patch_rules.ts`	`/test_suites/detections_response/basic_essentials_license/rule_edit/`
`/basic/tests/patch_rules_rules_bulk.ts`	`/test_suites/detections_response/basic_essentials_license/rule_edit/`
`/basic/tests/update_rules.ts`	`/test_suites/detections_response/basic_essentials_license/rule_edit/`
`/basic/tests/update_rules_bulk.ts`	`/test_suites/detections_response/basic_essentials_license/rule_edit/`
`/basic/tests/import_rules.ts`	`/test_suites/detections_response/basic_essentials_license/rule_import_export/`
`/basic/tests/export_rules.ts`	`/test_suites/detections_response/basic_essentials_license/rule_import_export/`
`/basic/tests/coverate_overview.ts`	`/test_suites/detections_response/basic_essentials_license/rule_management/`
`/basic/tests/find_rules.ts`	`/test_suites/detections_response/basic_essentials_license/rule_read/`
`/basic/tests/read_rules.ts`	`/test_suites/detections_response/basic_essentials_license/rule_read/`

`detection_engine_api_integration`	`timeline`
`/basic/tests/import_timelines.ts`	`/security_and_spaces/tests/basic/import_timelines.ts`
`/basic/tests/install_prepackaged_timelines.ts`	`/timeline/security_and_spaces/tests/basic/install_prepackaged_timelines.ts`

Folders renamed

x-pack/test/security_solution_api_integration/test_suites/lists_and_exception_lists/ --> x-pack/test/security_solution_api_integration/test_suites/value_lists_and_exception_lists/
x-pack/test/security_solution_api_integration/test_suites/lists_and_exception_lists/default_license/exception_lists_items/ --> x-pack/test/security_solution_api_integration/test_suites/value_lists_and_exception_lists/default_license/exception_lists/
x-pack/test/security_solution_api_integration/test_suites/lists_and_exception_lists/default_license/lists_items/ --> x-pack/test/security_solution_api_integration/test_suites/value_lists_and_exception_lists/default_license/value_lists/

Test status

Test	ESS	Serverless	QA
`read_rules.ts`	✅	✅	✅
`find_rules.ts`	✅	✅	✅
`coverage_overview.ts`	✅	❌	❌
`export_rules.ts`	✅	✅	✅
`import_rules.ts`	✅	✅	✅
`patch_rules.ts`	✅	✅	✅
`patch_rules_bulk.ts`	✅	✅	✅
`patch_rules_ess.ts`	✅	-	-
`update_rules.ts`	✅	✅	✅
`update_rules_ess.ts`	✅	-	-
`update_rules_bulk.ts`	✅	✅	✅
`delete_rules.ts`	✅	✅	✅
`delete_rules_bulk.ts`	✅	✅	✅
`create_rules_bulk.ts`	✅	✅	✅

.../test_suites/detections_response/basic_essentials_license/rule_import_export/import_rules.ts

x-pack/test/security_solution_api_integration/package.json

WafaaNasr

Please adjust the CodeOwners list to align with the recent modifications, as well as this document for the new groups and execution time in different environments, thanks!

WafaaNasr

LGTM! thanks Yara!!

WafaaNasr · 2023-11-29T15:10:52Z

Please adjust the CodeOwners list to align with the recent modifications, as well as this document for the new groups and execution time in different environments, thanks!

I updated the document regarding the groups but the execution time is still missing

elasticmachine · 2023-11-29T23:23:46Z

Pinging @elastic/security-detections-response (Team:Detections and Resp)

elasticmachine · 2023-11-29T23:23:47Z

Pinging @elastic/security-threat-hunting (Team:Threat Hunting)

jbudz

ftr_configs.yml

yctercero · 2023-11-30T20:51:30Z

@elasticmachine merge upstream

rylnd

I had a few comments about the naming/description/categorization of these tests, but those are mostly DX optimizations and don't need to block here.

Any test changes were consistent and clear.

LGTM!

rylnd · 2023-12-04T21:47:05Z

x-pack/test/security_solution_api_integration/package.json

-    "lists_items:server:ess": "npm run initialize-server:lists:default lists_items ess",
-    "lists_items:runner:ess": "npm run run-tests:lists:default lists_items ess essEnv"
+
+    "alerts_essentials:server:serverless": "npm run initialize-server:dr:basicEssentials alerts serverless",


IMO it would be less confusing to split up the suite name from the license, but maybe there's a reason for that?

Suggested change

"alerts_essentials:server:serverless": "npm run initialize-server:dr:basicEssentials alerts serverless",

"alerts:essentials:server:serverless": "npm run initialize-server:dr:basicEssentials alerts serverless",

rylnd · 2023-12-04T21:57:32Z

...ion_api_integration/test_suites/detections_response/basic_essentials_license/alerts/index.ts

-    loadTestFile(require.resolve('./alerts/open_close_alerts'));
-    loadTestFile(require.resolve('./alerts/query_alerts'));
-    loadTestFile(require.resolve('./alerts/query_alerts_backword_compatibility'));
+  describe('Detection alerts Basic and Essentials API', function () {


Similar to my above comment about keeping that taxonomy in these names:

Suggested change

describe('Detection alerts Basic and Essentials API', function () {

describe('Detection Engine - Alerts API - Basic and Essentials', function () {

++ to @rylnd's suggestion. I find the naming of all our pipelines, steps, JUnit reports etc not hierarchical enough to be able to understand quickly (under 1 sec) what every name actually means, so I have to read each name carefully and "parse" it.

This could be improved in follow-up PRs, of course, not a blocker from my side.

rylnd · 2023-12-04T22:01:17Z

...test_suites/detections_response/basic_essentials_license/rule_creation/configs/ess.config.ts

+    ...functionalConfig.getAll(),
+    testFiles: [require.resolve('..')],
+    junit: {
+      reportName: 'Rule creation ESS - Basic Integration Tests',


I'm trying to recall the context in which this string is presented, but my question is similar to above: should we just fully-qualify these report names so that they're easier to find/correlate?

Suggested change

reportName: 'Rule creation ESS - Basic Integration Tests',

reportName: 'Detection Engine - Rule Creation Integration Tests - ESS Env - Basic and Essential License ',

rylnd · 2023-12-04T22:02:21Z

.../test_suites/detections_response/basic_essentials_license/rule_creation/create_rules_bulk.ts

+  const ELASTICSEARCH_USERNAME = config.get('servers.kibana.username');

-  describe('create_rules_bulk', () => {
+  // TODO: add a new service


It's not clear to me what this comment means; I think we need more context here if we want this comment to persist/be useful

banderror

@yctercero Thank you so much for moving Rule Management tests for us to the new folder. The PR is huge. I haven't scanned all the diffs in every single file that was changed, but I reviewed the overall structure.

Posting a few comments + agree with the comments that @rylnd left.

banderror · 2023-12-11T14:08:39Z

.github/CODEOWNERS

+/x-pack/test/security_solution_api_integration/test_suites/detections_response/basic_essentials_license/rule_read @elastic/security-detection-rule-management
+/x-pack/test/security_solution_api_integration/test_suites/detections_response/basic_essentials_license/rule_management @elastic/security-detection-rule-management
+/x-pack/test/security_solution_api_integration/test_suites/detections_response/basic_essentials_license/rule_import_export @elastic/security-detection-rule-management


Could you also add x-pack/test/security_solution_api_integration/test_suites/detections_response/basic_essentials_license/rule_delete here?

banderror · 2023-12-11T14:11:42Z

.github/CODEOWNERS

+/x-pack/test/security_solution_api_integration/test_suites/detections_response/basic_essentials_license/rule_creation @elastic/security-detection-engine
+/x-pack/test/security_solution_api_integration/test_suites/detections_response/basic_essentials_license/rule_edit @elastic/security-detection-engine


I think I might agree that although the corresponding endpoints are owned by Rule Management, the Detection Engine team should probably own integration tests for rule creation and editing. Or at least some of these tests - those related to specific rule types and parameters.

I'm just wondering what is your reasoning behind this, could you please elaborate?

banderror · 2023-12-11T14:28:12Z

...ion_api_integration/test_suites/detections_response/basic_essentials_license/alerts/index.ts

-    loadTestFile(require.resolve('./alerts/open_close_alerts'));
-    loadTestFile(require.resolve('./alerts/query_alerts'));
-    loadTestFile(require.resolve('./alerts/query_alerts_backword_compatibility'));
+  describe('Detection alerts Basic and Essentials API', function () {


++ to @rylnd's suggestion. I find the naming of all our pipelines, steps, JUnit reports etc not hierarchical enough to be able to understand quickly (under 1 sec) what every name actually means, so I have to read each name carefully and "parse" it.

This could be improved in follow-up PRs, of course, not a blocker from my side.

banderror · 2023-12-11T14:33:44Z

...ntegration/test_suites/detections_response/basic_essentials_license/rule_edit/patch_rules.ts

-      it('should return a "403 forbidden" using a rule_id of type "machine learning"', async () => {
-        await createRule(supertest, log, getSimpleRule('rule-1'));
-
-        // patch a simple rule's type to machine learning
-        const { body } = await supertest
-          .patch(DETECTION_ENGINE_RULES_URL)
-          .set('kbn-xsrf', 'true')
-          .set('elastic-api-version', '2023-10-31')
-          .send({ rule_id: 'rule-1', type: 'machine_learning' })
-          .expect(403);
-
-        expect(body).to.eql({
-          message: 'Your license does not support machine learning. Please upgrade your license.',
-          status_code: 403,
-        });
-      });


Did you intentionally delete the test?

yctercero · 2023-12-13T00:15:51Z

@banderror are you ok with me merging this one along with https://github.com/elastic/kibana/issues?q=author%3AWafaaNasr and then following up with all the comments?

kibana-ci · 2023-12-13T05:57:57Z

💔 Build Failed

Failed CI Steps

Metrics [docs]

✅ unchanged

History

💚 Build #181173 succeeded 885fee3
💔 Build #181117 failed c90f81d
💔 Build #180690 failed c86ce48
💔 Build #180310 failed b62bafb

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

cc @yctercero

banderror

@banderror are you ok with me merging this one along with https://github.com/elastic/kibana/issues?q=author%3AWafaaNasr and then following up with all the comments?

Sure @yctercero. CI is not happy though

yctercero · 2023-12-13T16:36:10Z

@banderror are you ok with me merging this one along with https://github.com/elastic/kibana/issues?q=author%3AWafaaNasr and then following up with all the comments?

Sure @yctercero. CI is not happy though

Yea, I am going to close this PR as it has gotten too messy. I'll reopen a smaller PR that addresses the existing comments here @rylnd .

yctercero added 2 commits November 28, 2023 22:54

moving remaining basic license tests

52379af

updating tags

9b2c3ec

WafaaNasr mentioned this pull request Nov 29, 2023

[Security Solution] Enable API integration tests in Serverless: Detection Engine area #164445

Closed

WafaaNasr reviewed Nov 29, 2023

View reviewed changes

.../test_suites/detections_response/basic_essentials_license/rule_import_export/import_rules.ts Outdated Show resolved Hide resolved

WafaaNasr reviewed Nov 29, 2023

View reviewed changes

x-pack/test/security_solution_api_integration/package.json Show resolved Hide resolved

WafaaNasr reviewed Nov 29, 2023

View reviewed changes

WafaaNasr approved these changes Nov 29, 2023

View reviewed changes

yctercero mentioned this pull request Nov 29, 2023

[Security Solution] [FTR Test] Remove @skipInQA Tags from Tests #169770

Closed

updating tags and utils

abe2ffd

yctercero self-assigned this Nov 29, 2023

yctercero added release_note:skip Skip the PR/issue when compiling release notes Team:Detections and Resp Security Detection Response Team Team:Threat Hunting Security Solution Threat Hunting Team Team:Detection Rule Management Security Detection Rule Management Team v8.12.0 labels Nov 29, 2023

update codeowners

b62bafb

yctercero marked this pull request as ready for review November 29, 2023 23:23

yctercero requested review from a team as code owners November 29, 2023 23:23

yctercero requested a review from rylnd November 29, 2023 23:23

jbudz approved these changes Nov 29, 2023

View reviewed changes

szaffarano approved these changes Nov 30, 2023

View reviewed changes

pjhampton approved these changes Nov 30, 2023

View reviewed changes

banderror self-requested a review November 30, 2023 13:05

kibanamachine and others added 2 commits November 30, 2023 15:51

Merge branch 'main' into ftrs/basic

4158a84

update lint errors

c86ce48

banderror mentioned this pull request Dec 1, 2023

[Security Solution] Adds tests for coverage overview page #168058

Merged

3 tasks

yctercero added 3 commits December 1, 2023 20:04

adding to qa run configs

99136f6

Merge branch 'main' of github.com:elastic/kibana into ftrs/basic

c90f81d

remove old configs

885fee3

rylnd approved these changes Dec 4, 2023

View reviewed changes

rylnd mentioned this pull request Dec 4, 2023

[Security Solution][API testing][Rule Management] Move and restructures remaining rule managements trail-tests #172173

Merged

banderror reviewed Dec 11, 2023

View reviewed changes

yctercero added 3 commits December 12, 2023 21:21

Merge branch 'main' of github.com:elastic/kibana into ftrs/basic

7232183

remove detection_engine_api_integration folder

63802fa

cleanup after merge with upstream

079d045

yctercero requested a review from a team as a code owner December 13, 2023 05:34

banderror approved these changes Dec 13, 2023

View reviewed changes

yctercero closed this Dec 13, 2023

	"alerts_essentials:server:serverless": "npm run initialize-server:dr:basicEssentials alerts serverless",
	"alerts:essentials:server:serverless": "npm run initialize-server:dr:basicEssentials alerts serverless",

	describe('Detection alerts Basic and Essentials API', function () {
	describe('Detection Engine - Alerts API - Basic and Essentials', function () {

	reportName: 'Rule creation ESS - Basic Integration Tests',
	reportName: 'Detection Engine - Rule Creation Integration Tests - ESS Env - Basic and Essential License ',

		/x-pack/test/security_solution_api_integration/test_suites/detections_response/basic_essentials_license/rule_creation @elastic/security-detection-engine
		/x-pack/test/security_solution_api_integration/test_suites/detections_response/basic_essentials_license/rule_edit @elastic/security-detection-engine

Conversation

yctercero commented Nov 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Files moved from old structure to new folders

Folders renamed

Test status

Uh oh!

Uh oh!

Uh oh!

WafaaNasr left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WafaaNasr left a comment

Choose a reason for hiding this comment

Uh oh!

WafaaNasr commented Nov 29, 2023

Uh oh!

elasticmachine commented Nov 29, 2023

Uh oh!

elasticmachine commented Nov 29, 2023

Uh oh!

jbudz left a comment

Choose a reason for hiding this comment

Uh oh!

yctercero commented Nov 30, 2023

Uh oh!

rylnd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

banderror left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yctercero commented Dec 13, 2023

Uh oh!

kibana-ci commented Dec 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💔 Build Failed

Failed CI Steps

Metrics [docs]

History

Uh oh!

banderror left a comment

Choose a reason for hiding this comment

Uh oh!

yctercero commented Dec 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

yctercero commented Nov 29, 2023 •

edited

Loading

WafaaNasr left a comment •

edited

Loading

kibana-ci commented Dec 13, 2023 •

edited

Loading