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

[RAM] Remove errors and warning in triggers_actions_ui jest test #144443

Merged
merged 10 commits into from
Nov 8, 2022

Conversation

jcger
Copy link
Contributor

@jcger jcger commented Nov 2, 2022

Summary

Closes #144445

There are warnings/erros that are not going to be fixed here, but I've created issues for them:

In this PR we mostly improved the rule list:

It went down from rendering 17 times to 11 when no rule defined
It went down from rendering 31 times when 1 rule defined to 12

This made the tests much faster than before:

Before ~36s
Screenshot 2022-11-04 at 15 02 11

After ~16s
Screenshot 2022-11-04 at 15 02 53

@jcger jcger changed the title fix action_form test [RAM] Remove errors and warning in triggers_actions_ui jest test Nov 2, 2022
@jcger jcger marked this pull request as ready for review November 4, 2022 14:05
@jcger jcger requested a review from a team as a code owner November 4, 2022 14:05
const [isPopoverOpen, setIsPopoverOpen] = useState<boolean>(false);

useEffect(() => {
Copy link
Contributor Author

@jcger jcger Nov 4, 2022

Choose a reason for hiding this comment

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

this was causing rerenders that also caused duplicated calls to the api. Having the bug duplicated in another component meant to be calling the api 4 times

useEffect(() => {
loadData();
}, [loadData, percentileOptions]);
}, [loadData, refresh, percentileOptions]);
Copy link
Contributor Author

@jcger jcger Nov 4, 2022

Choose a reason for hiding this comment

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

this was doing 2 api calls, joining it means only one api call

await setup();
expect(wrapper.find('EuiBasicTable')).toHaveLength(1);
expect(wrapper.find('EuiTableRow')).toHaveLength(mockedRulesData.length);
describe('render table of rules', () => {
Copy link
Contributor Author

@jcger jcger Nov 4, 2022

Choose a reason for hiding this comment

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

instead of one test that took over 4 seconds we split into smaller tests that won't be working near the timeout limit. We avoid creating the wrapper every time by using beforeAll so there is no effective time loss

@jcger jcger added technical debt Improvement of the software architecture and operational architecture Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) labels Nov 4, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/response-ops (Team:ResponseOps)

@chandlerprall
Copy link
Contributor

I don't see the triggers_actions_ui suite executing in this build (https://buildkite.com/elastic/kibana-pull-request/builds/85480#01844303-94db-4a41-b093-7623154677d2) or the one that happened in my test with the EUI upgrade (https://buildkite.com/elastic/kibana-pull-request/builds/85556#018443a0-01a4-4c7d-b67b-9b9b1c6fd136)

@jcger
Copy link
Contributor Author

jcger commented Nov 7, 2022

@chandlerprall

For https://buildkite.com/elastic/kibana-pull-request/builds/85480#01844303-94db-4a41-b093-7623154677d2
triggers_actions_ui is here https://buildkite.com/elastic/kibana-pull-request/builds/85480#01844303-94e0-47fd-9164-5d0bafa6d5df/6469-6470 if you download the logs you'll see a lot of noise but after executing grep "triggers_actions_ui" file.log you'll see that everything is passing

The same happens for the second build you shared, but this time you'll find it here https://buildkite.com/elastic/kibana-pull-request/builds/85556#018443a0-01a4-4c7d-b67b-9b9b1c6fd136

Copy link
Contributor

@XavierM XavierM left a comment

Choose a reason for hiding this comment

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

LGTM

@chandlerprall
Copy link
Contributor

@jcger thanks for pointing to those logs; I wasn't expecting the truncation to eat up the log rollups, but that makes sense

@guskovaue
Copy link
Contributor

It's really nice PR! 🥇

@jcger
Copy link
Contributor Author

jcger commented Nov 8, 2022

After updating the branch to the newest changes, test running times have increased dramatically. Although it's still better than before, it might be worth to understand what's behind EUI changes to have a bigger picture and how it's affecting us and our users

Screenshot 2022-11-08 at 11 24 02

@jcger jcger enabled auto-merge (squash) November 8, 2022 10:31
@jcger jcger merged commit 8094003 into elastic:main Nov 8, 2022
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] Security Solution Tests #1 / Alerts timeline Privileges: read only "before each" hook for "should not allow user with read only privileges to attach alerts to a new case"

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
triggersActionsUi 658.9KB 658.7KB -153.0B
Unknown metric groups

ESLint disabled in files

id before after diff
osquery 1 2 +1

ESLint disabled line counts

id before after diff
enterpriseSearch 19 21 +2
fleet 58 64 +6
osquery 108 113 +5
securitySolution 440 446 +6
triggersActionsUi 132 130 -2
total +17

Total ESLint disabled count

id before after diff
enterpriseSearch 20 22 +2
fleet 66 72 +6
osquery 109 115 +6
securitySolution 517 523 +6
triggersActionsUi 135 133 -2
total +18

History

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

@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Nov 8, 2022
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 release_note:enhancement Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) technical debt Improvement of the software architecture and operational architecture v8.6.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RAM] Triggers_actions_ui jest test with async issues
7 participants