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][API testing] Move and restructures Alerts' tests #170350

Merged
merged 17 commits into from
Nov 7, 2023

Conversation

WafaaNasr
Copy link
Contributor

@WafaaNasr WafaaNasr commented Nov 1, 2023

Summary

Action File New Path if moved
Delete group1/find_rule_exception_references.ts Already moved in previous PR
Delete security_and_spaces/group6 -
Move detection_engine_api_integration/security_and_spaces/group6 detections_response/default_license/alerts/alerts_compatibility.ts
Move detection_engine_api_integration/security_and_spaces/group1 detections_response/default_license/alerts/aliases.ts
Move detection_engine_api_integration/security_and_spaces/group1 detections_response/default_license/alerts/create_index.ts
Move detection_engine_api_integration/security_and_spaces/group10/create_signals_migrations detections_response/default_license/alerts/migrations/create_alerts_migrations.ts
Move detection_engine_api_integration/security_and_spaces/group10/delete_signals_migrations detections_response/default_license/alerts/migrations/delete_alerts_migrations.ts
Move detection_engine_api_integration/security_and_spaces/group10/finalize_signals_migrations detections_response/default_license/alerts/migrations/finalize_alerts_migrations.ts
Move detection_engine_api_integration/security_and_spaces/group10/get_signals_migration_status detections_response/default_license/alerts/migrations/get_alerts_migration_status.ts
Move detection_engine_api_integration/security_and_spaces/group10/open_close_signals detections_response/default_license/alerts/open_close_alerts.ts
Move detection_engine_api_integration/security_and_spaces/group10/alert_tags detections_response/default_license/alerts/alert_tags.ts

Note: these tests are skipped on the main branch

https://github.com/elastic/kibana/blob/main/x-pack/test/detection_engine_api_integration/security_and_spaces/group10/open_close_signals.ts#L215

https://github.com/elastic/kibana/blob/main/x-pack/test/detection_engine_api_integration/security_and_spaces/group10/open_close_signals.ts#L252

https://github.com/elastic/kibana/blob/main/x-pack/test/detection_engine_api_integration/security_and_spaces/group10/finalize_signals_migrations.ts#L192

https://github.com/elastic/kibana/blob/main/x-pack/test/detection_engine_api_integration/security_and_spaces/group1/create_index.ts#L42

@WafaaNasr WafaaNasr self-assigned this Nov 1, 2023
@WafaaNasr WafaaNasr added release_note:skip Skip the PR/issue when compiling release notes FTR labels Nov 2, 2023
@WafaaNasr WafaaNasr marked this pull request as ready for review November 2, 2023 16:47
@WafaaNasr WafaaNasr requested review from a team as code owners November 2, 2023 16:47
x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/exceptions @elastic/security-detection-engine
x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/rule_creation @elastic/security-detection-engine
x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/actions @elastic/security-detection-engine
x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/rule_creation @elastic/security-detection-engine
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is a duplicate of line 1315?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct! replaced it with alerts, Thank you!

Copy link
Contributor

@rylnd rylnd left a comment

Choose a reason for hiding this comment

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

Changes here look correct, and CI seems to confirm things are working. I had a few nits about the names of these new test suites, but overall LGTM.

x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/rule_creation @elastic/security-detection-engine
x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/actions @elastic/security-detection-engine
x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/alerts @elastic/security-detection-engine
x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/alerts @elastic/security-detection-engine
Copy link
Contributor

Choose a reason for hiding this comment

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

Duplicate line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please find it fixed here

testFiles: [require.resolve('.')],
testFiles: [require.resolve('..')],
junit: {
reportName: 'Detection Engine ESS/Alerts API Integration Tests',
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: the slash here makes it look like "ESS/Alerts" is a concept. These seem to be pretty freeform, so I'll throw out a suggestion:

Suggested change
reportName: 'Detection Engine ESS/Alerts API Integration Tests',
reportName: 'Detection Engine API Integration Tests - ESS - Alerts ',

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense to me! I changed it here for the Alerts now and then I will iterate all of them too
Thanks Ryland!!
Please find it fixed here

export default createTestConfig({
testFiles: [require.resolve('..')],
junit: {
reportName: 'Detection Engine Serverless/Alerts API Integration Tests',
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto here on the name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please find it fixed here

@@ -35,7 +39,6 @@ function sleep(ms: number) {
return new Promise((resolve) => setTimeout(resolve, ms));
}

// eslint-disable-next-line import/no-default-export
Copy link
Contributor

Choose a reason for hiding this comment

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

💯

@@ -106,7 +106,6 @@
"@kbn/journeys",
"@kbn/stdio-dev-helpers",
"@kbn/alerting-api-integration-helpers",
"@kbn/securitysolution-ecs",
Copy link
Contributor

Choose a reason for hiding this comment

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

Clarification: is this being removed since only the child security_solution_api_integration now references it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Usually these changes are done by the CI, so I am not 100% sure but I kinda have the same explanation since it used only in the new security_solution_api_integration folder now

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #67 / Serverless Common UI - Platform Security API keys "before all" hook for "should create and delete API keys correctly"

Metrics [docs]

✅ unchanged

History

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

cc @WafaaNasr

@WafaaNasr WafaaNasr merged commit 2b5f4f7 into elastic:main Nov 7, 2023
28 checks passed
@WafaaNasr WafaaNasr deleted the move-structure-alerts-api branch November 7, 2023 12:42
@kibanamachine kibanamachine added v8.12.0 backport:skip This commit does not require backporting labels Nov 7, 2023
Ruhshan pushed a commit to Ruhshan/kibana that referenced this pull request Nov 7, 2023
…lastic#170350)

## Summary

- Following the initial work in this
elastic#166755
- Addresses part of elastic#151902 for
Alerts
- Introduced a new folder called `alerts` under
`security_solution_api_integration`
- Moved the utility files associated with alerts to the new directory
`security_solution_api_integration`. Files that were not actively used
in the previous folder were moved, while any duplicate files remained in
their original positions.
- Updated the CodeOwner file for the newly moved tests
- Old / new groups details and execution time
[document](https://docs.google.com/document/d/1CRFfDWMzw3ob03euWIvT4-IoiLXjoiPWI8mTBqP4Zks/edit)

| Action | File | New Path if moved |
|--------|------|----------|
| Delete| group1/find_rule_exception_references.ts| Already moved in
previous PR |
| Delete|security_and_spaces/group6  | - |
| Move|detection_engine_api_integration/security_and_spaces/group6
|detections_response/default_license/alerts/alerts_compatibility.ts|
| Move|detection_engine_api_integration/security_and_spaces/group1|
detections_response/default_license/alerts/aliases.ts |
| Move |detection_engine_api_integration/security_and_spaces/group1|
detections_response/default_license/alerts/create_index.ts|
|
Move|detection_engine_api_integration/security_and_spaces/group10/create_signals_migrations|
detections_response/default_license/alerts/migrations/create_alerts_migrations.ts
|
|
Move|detection_engine_api_integration/security_and_spaces/group10/delete_signals_migrations|
detections_response/default_license/alerts/migrations/delete_alerts_migrations.ts|
|
Move|detection_engine_api_integration/security_and_spaces/group10/finalize_signals_migrations|
detections_response/default_license/alerts/migrations/finalize_alerts_migrations.ts
|
|
Move|detection_engine_api_integration/security_and_spaces/group10/get_signals_migration_status|
detections_response/default_license/alerts/migrations/get_alerts_migration_status.ts|
|
Move|detection_engine_api_integration/security_and_spaces/group10/open_close_signals|
detections_response/default_license/alerts/open_close_alerts.ts|
|
Move|detection_engine_api_integration/security_and_spaces/group10/alert_tags|
detections_response/default_license/alerts/alert_tags.ts|


Note: these tests are skipped on the main branch


https://github.com/elastic/kibana/blob/main/x-pack/test/detection_engine_api_integration/security_and_spaces/group10/open_close_signals.ts#L215



https://github.com/elastic/kibana/blob/main/x-pack/test/detection_engine_api_integration/security_and_spaces/group10/open_close_signals.ts#L252


https://github.com/elastic/kibana/blob/main/x-pack/test/detection_engine_api_integration/security_and_spaces/group10/finalize_signals_migrations.ts#L192


https://github.com/elastic/kibana/blob/main/x-pack/test/detection_engine_api_integration/security_and_spaces/group1/create_index.ts#L42

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
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 FTR release_note:skip Skip the PR/issue when compiling release notes v8.12.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants