-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
🔴 Obsolete - [Security Solution] E2E Tests - Alerts Table - Convert Manual Regression to Automated Cypress tests #151027
Conversation
💔 Build FailedFailed CI StepsTest FailuresMetrics [docs]Async chunks
HistoryTo update your PR or re-run it, just comment with: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this trigger here is not useful, you're not testing anything after clicking
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
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'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and here
cy.get(ID_COLUMN_VALUES).first().trigger('mouseover', { timeout: 5000 }); | ||
}); | ||
|
||
it('Filter For', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these 2 tests are failing locally for me btw
cannot find [data-test-subj="filter-for-value"]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
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