[APM] Cleanup alerting api tests#164438
Conversation
|
Pinging @elastic/apm-ui (Team:APM) |
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
There was a problem hiding this comment.
This is problematic: rules and alerts are created per test but not deleted until all tests have run. This means that the rules and alerts created in the first test still exists when running the second test. This means that tests are required to run in a specific order, and it is not possible to run tests in isolation.
This was changed so rules and alerts are created per test, and also removed after each individual test. This ensures that subsequent tests are not polluted with state created in previous tests.
There was a problem hiding this comment.
I've removed all reference to the index connector action. We don't need it to test whether an alert was produced since we can just query the alerts index directly
There was a problem hiding this comment.
There are two things that we are testing:
- whether an alert is triggered (and shown in Kibana UI).
- alert notification was sent (with index connector in this case) with correct action variables.
I think the tests to check alert notifications are useful.
There was a problem hiding this comment.
alert notification was sent (with index connector in this case) with correct action variables.
Are we testing the APM rule in this case, or the connector functionality (and thus the alerting framework). By exercising the indexing connector it sounds like we are testing the latter where we should focus on the former.
There was a problem hiding this comment.
.. or put another way: what is currently not covered anymore? What could break in the APM rule that would no longer go noticed?
There was a problem hiding this comment.
We are setting context action variables during rule execution https://github.com/elastic/kibana/blob/f7e36a96d035e1cb4feec334a3bab270be9eac4e/x-pack/plugins/apm/server/routes/alerts/rule_types/transaction_duration/register_transaction_duration_rule_type.ts#L288C69-L288C69.
In the tests, we are checking if the action variables are set correctly in alert notification. Without it, any changes around context action variables would not be checked anymore.
There was a problem hiding this comment.
This was also problematic: a variable was set in one test and consumed in another. This means that tests depend on each other and cannot be run in isolation.
There was a problem hiding this comment.
This was an incorrect assertion caused by the alert pollution. Now alerts are cleaned up between runs.
daa0e75 to
3ca87f3
Compare
benakansara
left a comment
There was a problem hiding this comment.
LGTM. Thank you for addressing the issues and cleaning up the tests.
As discussed offline, tests around context variables will be added in a separate PR.
x-pack/test/apm_api_integration/tests/alerts/helpers/alerting_api_helper.ts
Outdated
Show resolved
Hide resolved
x-pack/test/apm_api_integration/tests/alerts/helpers/alerting_api_helper.ts
Outdated
Show resolved
Hide resolved
kpatticha
left a comment
There was a problem hiding this comment.
LGTM, thanks for pushing this work forward. I left only 2 comments.
Also, have you run the flaky test runner?
953aa0c to
0927bcd
Compare
💛 Build succeeded, but was flaky
Failed CI StepsMetrics [docs]
History
To update your PR or re-run it, just comment with: |
* main: (3152 commits) [Security Solution][Detection Engine] fixes 410 error on index legacy template call (#164682) [SavedObjects] Create serverless roots for jest integration tests (#164157) Create upselling package and implement EntityAnalytics serverless upselling (#164136) [Fleet] Change 'Out-of-date' to 'Outdated policy' in agent list table (#164673) [IndexManagement] Use internal base path for API (#164665) [Profiling] removing ~ symbol (#164595) [Telemetry] Fetch snapshot: allow specifying the version via querystring (#164670) [Cases] Show warning when all cases table reaches 10k cases message (#164323) [ML] Removing token list from text expansion model testing (#164560) [Fleet] Add secrets package API integration test (#164583) [Fleet] Fix security solution tag id (#164582) [Security Solution] Modal says "duplicating 0 rules" when you duplicate an individual rule (#163908) [api-docs] 2023-08-24 Daily api_docs build (#164658) [APM] Cleanup alerting api tests (#164438) Upgrade EUI to 87.2.0 (#164385) [ML] Fix query bar autocompletion for ML and AIOps embeddables (#164485) [Fleet] Fix flaky unit test for the details page (#164641) [Security Solution] update codeowner for serverless security subdir (#164640) [Security Solution] Fixes Assistant Connector and Actions RBAC Flow (#164382) [Discover] Removing large string truncation from doc viewer (#164236) ...
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
This PR cleans up and refactors the APM API tests for rules and alerting. - introduces some new helper methods like `deleteRuleById` - removes dependency on index actions to test alerts (we can just use the alert index) - improve flaky tests and ensure that tests can be run in isolation and in any order (cherry picked from commit d440288)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
This PR cleans up and refactors the APM API tests for rules and alerting. - introduces some new helper methods like `deleteRuleById` - removes dependency on index actions to test alerts (we can just use the alert index) - improve flaky tests and ensure that tests can be run in isolation and in any order (cherry picked from commit d440288)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
This PR cleans up and refactors the APM API tests for rules and alerting. - introduces some new helper methods like `deleteRuleById` - removes dependency on index actions to test alerts (we can just use the alert index) - improve flaky tests and ensure that tests can be run in isolation and in any order (cherry picked from commit d440288)
This PR cleans up and refactors the APM API tests for rules and alerting. - introduces some new helper methods like `deleteRuleById` - removes dependency on index actions to test alerts (we can just use the alert index) - improve flaky tests and ensure that tests can be run in isolation and in any order (cherry picked from commit d440288)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation and see the Github Action logs for details |
# Backport This will backport the following commits from `main` to `8.10`: - [[APM] Cleanup alerting api tests (#164438)](#164438) <!--- Backport version: 8.9.7 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Søren Louv-Jansen","email":"soren.louv@elastic.co"},"sourceCommit":{"committedDate":"2023-08-24T00:22:26Z","message":"[APM] Cleanup alerting api tests (#164438)\n\nThis PR cleans up and refactors the APM API tests for rules and\r\nalerting.\r\n\r\n- introduces some new helper methods like `deleteRuleById`\r\n- removes dependency on index actions to test alerts (we can just use\r\nthe alert index)\r\n- improve flaky tests and ensure that tests can be run in isolation and\r\nin any order","sha":"d4402886a12e6c429d72f441ee988b384645fd57","branchLabelMapping":{"^v8.11.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Team:APM","release_note:skip","backport:skip","auto-backport","v8.10.0","v8.11.0"],"number":164438,"url":"https://github.com/elastic/kibana/pull/164438","mergeCommit":{"message":"[APM] Cleanup alerting api tests (#164438)\n\nThis PR cleans up and refactors the APM API tests for rules and\r\nalerting.\r\n\r\n- introduces some new helper methods like `deleteRuleById`\r\n- removes dependency on index actions to test alerts (we can just use\r\nthe alert index)\r\n- improve flaky tests and ensure that tests can be run in isolation and\r\nin any order","sha":"d4402886a12e6c429d72f441ee988b384645fd57"}},"sourceBranch":"main","suggestedTargetBranches":["8.10"],"targetPullRequestStates":[{"branch":"8.10","label":"v8.10.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.11.0","labelRegex":"^v8.11.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/164438","number":164438,"mergeCommit":{"message":"[APM] Cleanup alerting api tests (#164438)\n\nThis PR cleans up and refactors the APM API tests for rules and\r\nalerting.\r\n\r\n- introduces some new helper methods like `deleteRuleById`\r\n- removes dependency on index actions to test alerts (we can just use\r\nthe alert index)\r\n- improve flaky tests and ensure that tests can be run in isolation and\r\nin any order","sha":"d4402886a12e6c429d72f441ee988b384645fd57"}}]}] BACKPORT--> Co-authored-by: Søren Louv-Jansen <soren.louv@elastic.co>
This PR cleans up and refactors the APM API tests for rules and alerting.
deleteRuleById