🔴 Obsolete - [Security Solution] E2E Tests - Alerts Table - Convert Manual Regression to Automated Cypress tests#151027
🔴 Obsolete - [Security Solution] E2E Tests - Alerts Table - Convert Manual Regression to Automated Cypress tests#151027logeekal wants to merge 4 commits intoelastic:mainfrom
Conversation
logeekal
added
release_note:fix
Team:Threat Hunting:Investigations
logeekal
changed the title
💔 Build FailedFailed CI StepsTest FailuresMetrics [docs]Async chunks
HistoryTo update your PR or re-run it, just comment with: |
PhilippeOberti
left a comment
PhilippeOberti
left a comment
There was a problem hiding this comment.
@logeekal I stopped half way as I feel like we could (should?) have a team meeting discussing Cypress tests. I feel like getting aligned on what we really want these test to do would be nice, as we have a slightly different vision of what they should do at the moment :)
Also, same comment I made on Christine PR, I feel like test titles should be more descriptive. For example Host Summary could be checks that host summary flyout opens and closes or mayeb follow the Jest unit test patterns and be something like should open the host summary flyout after clicking on a table cell hover action...
| import { login, visit } from '../../tasks/login'; | ||
| import { ALERTS_URL } from '../../urls/navigation'; | ||
|
|
||
| describe('Alert Table Contorls', { testIsolation: false }, () => { |
There was a problem hiding this comment.
this structure confused me at first: I've never seen Cypress tests that don't check user actions. I feel like some of the tests here would be better as Jest unit tests as you don't really check user interactions, but just existence of dom elements...
| .should('have.attr', 'aria-label', 'Enter fullscreen') | ||
| .trigger('click') | ||
| .should('have.attr', 'aria-label', 'Exit fullscreen') | ||
| .trigger('click'); |
There was a problem hiding this comment.
this trigger here is not useful, you're not testing anything after clicking
There was a problem hiding this comment.
@PhilippeOberti , if you see the describe clause, there is a option called testIsolation: false which makes sure that the page does not refresh after every test so as to save time. Since, page does not refresh, test has to cleanup after itself.
Also, since all tests are different, cleanup cannot same and in afterEach clause.
There was a problem hiding this comment.
ok that makes sense! I wonder if it would make it more readable to extract this into a method like exitFullScreen(). But if we only use it once here, not sure its valuable...
| it('column sorting control exists', () => { | ||
| cy.get(DATA_GRID_COLUMN_ORDER_BTN).should('be.visible'); | ||
| }); | ||
|
|
||
| it('field Browser Exists', () => { | ||
| cy.get(FIELDS_BROWSER_BTN).should('be.visible'); | ||
| }); |
There was a problem hiding this comment.
these really feel like Jest unit test and not Cypress tests to me, there isn't any user interaction
| cy.get(GET_DATA_GRID_HEADER_CELL_ACTION_GROUP(timestampField)) | ||
| .should('be.visible') | ||
| .should('contain.text', 'Sort Old-New'); | ||
| cy.get(GET_DATA_GRID_HEADER(timestampField)).trigger('click'); |
There was a problem hiding this comment.
this isn't testing anything
| cy.get(GET_DATA_GRID_HEADER_CELL_ACTION_GROUP(riskScoreField)) | ||
| .should('be.visible') | ||
| .should('contain.text', 'Sort Low-High'); | ||
| cy.get(GET_DATA_GRID_HEADER(riskScoreField)).trigger('click'); |
| cy.get(GET_DATA_GRID_HEADER_CELL_ACTION_GROUP(ruleField)) | ||
| .should('be.visible') | ||
| .should('contain.text', 'Sort A-Z'); | ||
| cy.get(GET_DATA_GRID_HEADER(ruleField)).trigger('click'); |
| cy.get(ID_COLUMN_VALUES).first().trigger('mouseover', { timeout: 5000 }); | ||
| }); | ||
|
|
||
| it('Filter For', () => { |
There was a problem hiding this comment.
similar comment I made on Christine PR, I feel like the filter in and out tests could be combined into one that test the filter functionality?
There was a problem hiding this comment.
these 2 tests are failing locally for me btw
cannot find [data-test-subj="filter-for-value"]
There was a problem hiding this comment.
@PhilippeOberti yes.. it is because of new PR that has merged. I will resolve conflicts soon.
| }); | ||
| }); | ||
|
|
||
| it('Add To Timeline', () => { |
There was a problem hiding this comment.
and this one got stuck in a weird loop checking the same thing over and over again, then finally failed
There was a problem hiding this comment.
@PhilippeOberti , I will check why this is happening, thanks for checking it out.
| cy.get(CELL_EXPAND_VALUE).first().click({ force: true }); | ||
| }); | ||
|
|
||
| it('Host Summary', () => { |
There was a problem hiding this comment.
if I understand correctly, this test is basically testing that the flyout shows up when clicking on an hover action, then that it disappears when clicking on the close (cross) button?
I feel like testing the closing is unnecessary as it's being handled by the EuiFlyout isn't it?
Also I feel like testing the opening when clicking on the hover action could be bundled with a test that actually tests something within the flyout?
|
@logeekal also quick question: are we keeping this one PR with all the tests, or are we going to break it down in multiple PRs? |
I am breaking these up in multiple PRs and will also keep some of your comments in mind.. Will let you know when donee. |
logeekal
changed the title
3 participants
Summary
This PR adds only Cypress Tests in lieu of below Manual Regression tests. Complete list of regression tests that need to automated is given here: [Epic][Investigations] - Automate manual regression tests #59
Alert table