-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Security Solution][RAC][Cypress] Unskip some tests #117596
Conversation
Pinging @elastic/security-solution (Team: SecuritySolution) |
@elasticmachine merge upstream |
merge conflict between base and head |
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.
LGTM - left a few comments for my own understanding, code looks great
@@ -219,7 +219,6 @@ describe('Detection rules, sequence EQL', () => { | |||
cy.log('ALERT_DATA_GRID', text); | |||
expect(text).contains(this.rule.name); | |||
expect(text).contains(this.rule.severity.toLowerCase()); | |||
expect(text).contains(this.rule.riskScore); |
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.
is there a particular reason we are removing the riskScore checks
@@ -25,7 +26,7 @@ const InvestigationGuideViewComponent: React.FC<{ | |||
data: TimelineEventsDetailsItem[]; | |||
}> = ({ data }) => { | |||
const ruleId = useMemo(() => { | |||
const item = data.find((d) => d.field === 'signal.rule.id'); | |||
const item = data.find((d) => d.field === 'signal.rule.id' || d.field === ALERT_RULE_UUID); |
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.
why do we have an or
here? don't we need just d.field === ALERT_RULE_UUID
[data] | ||
); | ||
const ruleId = useMemo(() => { | ||
const siemSignalsRuleId = getFieldValue({ category: 'signal', field: 'signal.rule.id' }, data); |
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.
so rules continue to have signal.rule.id
- does that include newly created rules in 8.0 as well?
: category === 'kibana' | ||
? field.replace('kibana.alert', 'signal').replace('rule.uuid', 'rule.id') | ||
: field; | ||
return ( |
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.
I think it's coming together now. We keep both versions and search for each if we can't find one or the other. very interesting
@@ -53,9 +58,12 @@ export const RenderCellValue: React.FC<EuiDataGridCellValueElementProps & CellVa | |||
<Status data-test-subj="alert-status" status={random(0, 1) ? 'recovered' : 'active'} /> | |||
); | |||
case ALERT_DURATION: | |||
case 'signal.duration.us': |
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.
it would be nice if the older versions were consts as well
() => find({ category: 'signal', field: 'signal.rule.index' }, detailsData)?.values, | ||
() => | ||
find({ category: 'signal', field: 'signal.rule.index' }, detailsData)?.values ?? | ||
find({ category: 'kibana', field: 'kibana.alert.rule.index' }, detailsData)?.values, |
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.
is there an ALERT_RULE_INDEX?
@@ -18,7 +18,7 @@ import { | |||
StyledContent, | |||
} from '../../../../common/lib/cell_actions/expanded_cell_value_actions'; | |||
|
|||
const FIELDS_WITHOUT_CELL_ACTIONS = ['signal.rule.risk_score', 'signal.reason']; | |||
const FIELDS_WITHOUT_CELL_ACTIONS = ['kibana.alert.rule.risk_score', 'kibana.alert.reason']; |
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.
interesting, so for the cells the data only appears with the new field name (even if it might be read from an older field)?
jenkins test this |
@elasticmachine merge upstream |
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.
Tested with @MadameSheema. Thanks @madirey !
💛 Build succeeded, but was flaky
Test Failures
Metrics [docs]Module Count
Async chunks
History
To update your PR or re-run it, just comment with: |
* Reenable cypress tests for rules * Indicator match is not yet passing * Update refs * Fix eql alert generation original_time and building_block_type * Unskip a few more tests * Update field names in jest tests * Fix unit tests / cypress tests * Have to keep this one skipped for now * Fix some more tests? * cleanup * Fix translation
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
* Reenable cypress tests for rules * Indicator match is not yet passing * Update refs * Fix eql alert generation original_time and building_block_type * Unskip a few more tests * Update field names in jest tests * Fix unit tests / cypress tests * Have to keep this one skipped for now * Fix some more tests? * cleanup * Fix translation Co-authored-by: Madison Caldwell <[email protected]>
* Reenable cypress tests for rules * Indicator match is not yet passing * Update refs * Fix eql alert generation original_time and building_block_type * Unskip a few more tests * Update field names in jest tests * Fix unit tests / cypress tests * Have to keep this one skipped for now * Fix some more tests? * cleanup * Fix translation
* Reenable cypress tests for rules * Indicator match is not yet passing * Update refs * Fix eql alert generation original_time and building_block_type * Unskip a few more tests * Update field names in jest tests * Fix unit tests / cypress tests * Have to keep this one skipped for now * Fix some more tests? * cleanup * Fix translation
Summary
Critical fixes for 8.0
building_block_type
and setskibana.alert.original_time
to the timestamp of the first event in the sequence.siem.signals
(for backward compatibility) and new AAD fields in alertsChecklist
Delete any items that are not applicable to this PR.
For maintainers