[SecuritySolution] Unskip fields browser timeline tests #174597

janmonschke · 2024-01-10T13:25:04Z

Summary

Unskips the fields browser tests.

The fields browser tests have been skipped because of issues during the population of the timeline (as described in #169363, #169271, #169197). As investigated in previous PRs (e.g. #174593) that flakiness has been removed in #173413 so it is safe to unskip the tests now.

In addition to that, unnecessary timeline population was removed, force clicks/checks/unchecks cleaned up. Also, parts of the acceptance test have been moved to the unit test.

Flaky test runner (200/200)

fixes #169197
fixes #169271
fixes #169363

The fields browser is opened in beforeEach

The timeline's data / its rows are not actually tested

elasticmachine · 2024-01-10T17:30:59Z

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

elasticmachine · 2024-01-10T17:34:32Z

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

janmonschke · 2024-01-10T17:39:01Z

/ci

PhilippeOberti

left a few comments on some cleanup and potential merge of tests to reduce run time :)

PhilippeOberti · 2024-01-10T21:04:58Z

x-pack/test/security_solution_cypress/cypress/tasks/timeline.ts

@@ -369,7 +369,7 @@ export const markAsFavorite = () => {
 };

 export const openTimelineFieldsBrowser = () => {
-  cy.get(TIMELINE_FIELDS_BUTTON).first().click({ force: true });
+  cy.get(TIMELINE_FIELDS_BUTTON).first().click();


I just want to note that the first() here is pretty dangerous/brittle. I was trying to remove it and realized that we have 3 instances of the FieldBrowserComponent which contains the EuiButtonEmpty that has the data-test-subj="show-field-browser" property.
It happens that in the DOM the first one is the one that we mostly want, but that could change.

I don't think this PR is the right place to do it, so this is just a note, but we should at some point modify the FieldBrowserComponent to take the `data-test-subj- as a prop so we can guarantee the uniqueness.

Agreed, first() often is a source of flakiness/trouble in the future. I wouldn't want to touch it in this PR. Also, it seems like the component is owned by ResponseOps and not us...

PhilippeOberti · 2024-01-10T21:09:41Z

x-pack/test/security_solution_cypress/cypress/e2e/investigations/timelines/fields_browser.cy.ts

-    it('displays "view all" option by default', () => {
-      cy.get(FIELDS_BROWSER_VIEW_BUTTON).should('contain.text', 'View: all');
-    });
-
    it('displays the expected count of categories that match the filter input', () => {


what do you think about merging some of the tests below?

I can see the first (displays the expected count of categories that match the filter input) and second (displays a search results label with the expected count of fields matching the filter input) together. They have the same input and only check a small UI element each...

I can also see the last 3 (or 2) in this describe merged together, under a title that would say something like should correctly create and search category or something more descriptive. A few cy.log in the test would clarify what exactly happens in each step.

The whole test suite would run much faster

Yep, nice catch. I'll merge them.

PhilippeOberti · 2024-01-10T21:11:07Z

x-pack/test/security_solution_cypress/cypress/e2e/investigations/timelines/fields_browser.cy.ts

    beforeEach(() => {
      login();
      visitWithTimeRange(hostsUrl('allHosts'));
      openTimelineUsingToggle();
-      populateTimeline();
      openTimelineFieldsBrowser();
    });



same comment here, I feel like the first 3 tests could easily be merged into a single one that would test add, remove and reset fields.

Thoughts?

actually even the fourth one (restores focus to the Customize Columns button when Reset Fields is clicked) can easily be merged with the other 3. Just check the focus after you've reset. It would just be a this single line added at the end

cy.get(TIMELINE_FIELDS_BUTTON).should('have.focus');

It doesn't really make sense to have a whole test that verifies just that

x-pack/test/security_solution_cypress/cypress/e2e/investigations/timelines/fields_browser.cy.ts

janmonschke · 2024-01-11T09:55:59Z

@PhilippeOberti Thanks for looking into how to simplify these tests. I consolidated a bunch of them into combined tests now. However, I didn't merge them all as that would have created super long tests that, even with cy.log, would become quite hard to follow. In total we're now down to 5 tests. This is considerably less than before. Let me know if you think we should consolidate them further.

PhilippeOberti

looks super clean, thank you!!

PhilippeOberti · 2024-01-11T23:45:43Z

@PhilippeOberti Thanks for looking into how to simplify these tests. I consolidated a bunch of them into combined tests now. However, I didn't merge them all as that would have created super long tests that, even with cy.log, would become quite hard to follow. In total we're now down to 5 tests. This is considerably less than before. Let me know if you think we should consolidate them further.

it looks awesome. I took the liberty to push a commit that just changes the name of 2 tests to use should a the beginning

kibana-ci · 2024-01-12T02:32:38Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: 9f97807

Failed CI Steps

Jest Tests #15

Metrics [docs]

✅ unchanged

History

💔 Build #187088 failed 8e9520f
💛 Build #186943 was flaky 2c76383
💛 Build #186862 was flaky 92e3917

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

cc @janmonschke

janmonschke added 4 commits January 10, 2024 13:53

context -> describe

1895256

remove force clicks/checks/unchecks

91a2073

remove unnecessary calls to openTimelineFieldsBrowser

04ddc10

The fields browser is opened in beforeEach

Remove populateTimeline

d5140f7

The timeline's data / its rows are not actually tested

janmonschke added release_note:skip Skip the PR/issue when compiling release notes Team:Threat Hunting:Investigations Security Solution Investigations Team labels Jan 10, 2024

janmonschke self-assigned this Jan 10, 2024

janmonschke added 4 commits January 10, 2024 14:25

prepare flaky test runner

c8cf895

undo flaky test runner changes

42d1970

replace part of the acceptance test with a unit test

57eeecc

Merge branch 'main' into security/unskip-fields-browser-timeline-tests

92e3917

janmonschke marked this pull request as ready for review January 10, 2024 17:30

janmonschke requested review from a team as code owners January 10, 2024 17:30

janmonschke added the Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) label Jan 10, 2024

XavierM approved these changes Jan 10, 2024

View reviewed changes

PhilippeOberti reviewed Jan 10, 2024

View reviewed changes

janmonschke added 2 commits January 11, 2024 10:12

simplify tests

04390a0

simplify tests

2c76383

rename 2 tests to use should... convention

8e9520f

PhilippeOberti approved these changes Jan 11, 2024

View reviewed changes

Merge branch 'main' into security/unskip-fields-browser-timeline-tests

9f97807

PhilippeOberti merged commit a8d3173 into elastic:main Jan 12, 2024
38 checks passed

kibanamachine added the v8.13.0 label Jan 12, 2024

kibanamachine added the backport:skip This commit does not require backporting label Jan 12, 2024

semd pushed a commit to semd/kibana that referenced this pull request Jan 12, 2024

[SecuritySolution] Unskip fields browser timeline tests (elastic#174597)

a6f7bbf

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SecuritySolution] Unskip fields browser timeline tests #174597

[SecuritySolution] Unskip fields browser timeline tests #174597

janmonschke commented Jan 10, 2024 •

edited

Loading

elasticmachine commented Jan 10, 2024

elasticmachine commented Jan 10, 2024

janmonschke commented Jan 10, 2024

PhilippeOberti left a comment

PhilippeOberti Jan 10, 2024

janmonschke Jan 11, 2024

PhilippeOberti Jan 10, 2024

janmonschke Jan 11, 2024

PhilippeOberti Jan 10, 2024

PhilippeOberti Jan 10, 2024

janmonschke commented Jan 11, 2024

PhilippeOberti left a comment

PhilippeOberti commented Jan 11, 2024

kibana-ci commented Jan 12, 2024

[SecuritySolution] Unskip fields browser timeline tests #174597

[SecuritySolution] Unskip fields browser timeline tests #174597

Conversation

janmonschke commented Jan 10, 2024 • edited Loading

Summary

elasticmachine commented Jan 10, 2024

elasticmachine commented Jan 10, 2024

janmonschke commented Jan 10, 2024

PhilippeOberti left a comment

Choose a reason for hiding this comment

PhilippeOberti Jan 10, 2024

Choose a reason for hiding this comment

janmonschke Jan 11, 2024

Choose a reason for hiding this comment

PhilippeOberti Jan 10, 2024

Choose a reason for hiding this comment

janmonschke Jan 11, 2024

Choose a reason for hiding this comment

PhilippeOberti Jan 10, 2024

Choose a reason for hiding this comment

PhilippeOberti Jan 10, 2024

Choose a reason for hiding this comment

janmonschke commented Jan 11, 2024

PhilippeOberti left a comment

Choose a reason for hiding this comment

PhilippeOberti commented Jan 11, 2024

kibana-ci commented Jan 12, 2024

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

History

janmonschke commented Jan 10, 2024 •

edited

Loading