[Cloud Security] 10973 migrate flaky e2e tests to jest #208345

alexreal1314 · 2025-01-27T11:20:48Z

Summary

This PR tries to fix the following issues - which are flaky FTR tests:

Failing test: X-Pack Cloud Security Posture Functional Tests.x-pack/test/cloud_security_posture_functional/pages/findings_onboarding·ts - Cloud Security Posture Findings Page onboarding Vulnerabilities - clicking on the No integrations installed prompt action button - install CNVM: navigates to the CNVM integration installation page #197143 - convert to unit test
Failing test: X-Pack Cloud Security Posture Functional Tests.x-pack/test/cloud_security_posture_functional/pages/cis_integrations/cnvm/cis_integration_cnvm·ts - Cloud Security Posture Test adding Cloud Security Posture Integrations CNVM CNVM AWS Hyperlink on PostInstallation Modal should have the correct URL #190967 - fixed
Failing test: X-Pack Cloud Security Posture Functional Tests.x-pack/test/cloud_security_posture_functional/pages/cis_integrations/cspm/cis_integration_aws·ts - Cloud Security Posture Test adding Cloud Security Posture Integrations CSPM AWS CIS_AWS Organization Manual Assume Role CIS_AWS Organization Manual Assume Role Workflow #187470 - fixed
Failing test: X-Pack Cloud Security Posture Functional Tests.x-pack/test/cloud_security_posture_functional/pages/cis_integrations/cnvm/cis_integration_cnvm·ts - Cloud Security Posture Test adding Cloud Security Posture Integrations CNVM CNVM AWS Clicking on Launch CloudFormation on post intall modal should lead user to Cloud Formation page #186302 - convert to unit test
Failing test: X-Pack Cloud Security Posture Functional Tests.x-pack/test/cloud_security_posture_functional/pages/cis_integrations/cspm/cis_integration_aws·ts - Cloud Security Posture Test adding Cloud Security Posture Integrations CSPM AWS CIS_AWS Organization Cloud Formation Clicking on Launch CloudFormation on post intall modal should lead user to Cloud Formation page #186438 - convert to unit test

There will be an RFC document which is going to be released to help us better understand and decide which tests are more suitable to make as E2E tests and which as unit tests.

Checklist

Unit or functional tests were updated or added to match the most common scenarios
Flaky Test Runner was used on any tests changed

Closes

this PR closes the above mentioned issues in relation for this ticket - https://github.com/elastic/security-team/issues/10973

elasticmachine · 2025-01-29T19:27:23Z

Pinging @elastic/kibana-cloud-security-posture (Team:Cloud Security)

elasticmachine · 2025-01-29T19:45:24Z

💔 Build Failed

Buildkite Build
Commit: ba885fc

Failed CI Steps

Check Types

History

💚 Build #271408 succeeded 25177ad
💔 Build #271260 failed 89b295b
💔 Build #271167 failed 7df442e
💔 Build #270981 failed 013d213
💔 Build #270952 failed a5bc2bc

cc @alexreal1314

elasticmachine · 2025-01-30T08:45:44Z

Pinging @elastic/fleet (Team:Fleet)

remove flaky test cases related to vulnerabilities empty states

…ve the correct URL flaky test case

…file add retry.try wrapper to assertion in when adding CIS_AWS with manual assume role

maxcold · 2025-01-30T14:27:20Z

...security/plugins/cloud_security_posture/public/components/no_vulnerabilities_states.test.tsx

+
+  it('Vulnerabilities - `Add Wiz integration`: should have link element to CNVM integration installation page', async () => {
+    await waitFor(() =>
+      expect(screen.getByText(/Already using a\s+cloud security product?/i)).toBeInTheDocument()


@alexreal1314 checking the exact text makes the test quite brittle. Every change to the wording would mean a failing test. Ideally, we should adda test id for every selector we use in the tests

@maxcold agree, I'll convert it to test id in the next PR that should fix some more flaky tests.

@maxcold I see your point about avoiding brittle tests due to wording changes, and I agree that test IDs can provide a stable way to locate elements. However, I believe there’s value in also asserting visible text, especially for key user-facing elements.

Relying solely on test IDs can make tests less reflective of the real user experience since test IDs are not part of what the user actually interacts with. By checking for meaningful text in assertions (like in this case with getByText), we ensure that the UI presents the correct messaging, which is a crucial part of user expectations.

A balanced approach could be to use test IDs for structural assertions (e.g., ensuring an element exists or checking attributes like href), while still verifying key text content where appropriate. In this case, the assertion for the message ensures that users see the correct prompt, while the button's testId allows for stable selection.

What do you think about maintaining a mix of both approaches based on what’s being tested rather than applying test IDs universally?

@opauloh I think tests should avoid testing implementation details. For me the litmus test of a good test is when I change the implementation and the test is still green if the business logic didn't change. In the case of the wording, you will be updating test with every wording change. Another way to check if it makes sense is to push it to the absurd level: imagine we test for every visible text in Kibana, does it make sense to you? I believe it will make tests maintenance unbearable.

Plus you don't test for what's really important when it comes to texts anyway. Does it have typos? Does it provide outdated information that is not relevant anymore? etc.
Maybe in the future with the help of AI we will be testing texts in the way it makes sense, eg. screen.prompt('check that text promoting connecting existing 3p product integration is present ') But we are not there yet :)

opauloh · 2025-01-30T23:39:42Z

x-pack/test/cloud_security_posture_functional/constants/test_subject_ids.ts

+ * 2.0.
+ */
+
+export const testSubjectIds = {


nice initiative concentrating the test subject ids!

opauloh · 2025-01-30T23:44:02Z

...security/plugins/cloud_security_posture/public/components/no_vulnerabilities_states.test.tsx

+
+  it('Vulnerabilities - `Add Wiz integration`: should have link element to CNVM integration installation page', async () => {
+    await waitFor(() =>
+      expect(screen.getByText(/Already using a\s+cloud security product?/i)).toBeInTheDocument()


@maxcold I see your point about avoiding brittle tests due to wording changes, and I agree that test IDs can provide a stable way to locate elements. However, I believe there’s value in also asserting visible text, especially for key user-facing elements.

Relying solely on test IDs can make tests less reflective of the real user experience since test IDs are not part of what the user actually interacts with. By checking for meaningful text in assertions (like in this case with getByText), we ensure that the UI presents the correct messaging, which is a crucial part of user expectations.

A balanced approach could be to use test IDs for structural assertions (e.g., ensuring an element exists or checking attributes like href), while still verifying key text content where appropriate. In this case, the assertion for the message ensures that users see the correct prompt, while the button's testId allows for stable selection.

What do you think about maintaining a mix of both approaches based on what’s being tested rather than applying test IDs universally?

…210746) # Backport This will backport the following commits from `main` to `8.x`: - #208345) ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sorenlouv/backport)

kibanamachine · 2025-02-13T13:49:18Z