Skip to content

[Security Solution] Unskip Cypress tests for rule execution log #232023

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.

Sign up for GitHub

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

Merged
banderror merged 2 commits into from
Aug 18, 2025
+23 −10
Merged

[Security Solution] Unskip Cypress tests for rule execution log #232023

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
59e0ff5
Unskip tests
banderror Aug 17, 2025
0eca460
Fix tests
banderror Aug 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 22 additions & 8 deletions ...n_cypress/cypress/e2e/detection_response/rule_management/rule_details/execution_log.cy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,19 +12,17 @@ import { login } from '../../../../tasks/login';
import { visit } from '../../../../tasks/navigation';
import { ruleDetailsUrl } from '../../../../urls/rule_details';
import { createRule } from '../../../../tasks/api_calls/rules';
import { waitForAlertsToPopulate } from '../../../../tasks/create_new_rule';
import {
goToExecutionLogTab,
getExecutionLogTableRow,
refreshRuleExecutionTable,
filterByRunType,
} from '../../../../tasks/rule_details';
import { getNewRule } from '../../../../objects/rule';
import { getCustomQueryRuleParams } from '../../../../objects/rule';
import { EXECUTION_SHOWING } from '../../../../screens/rule_details';
import { manualRuleRun } from '../../../../tasks/api_calls/backfill';

// FLAKY: https://github.com/elastic/kibana/issues/184360
describe.skip(
describe(
'Event log',
{
tags: ['@ess', '@serverless'],
Expand All @@ -38,19 +36,33 @@ describe.skip(
login();
deleteAlertsAndRules();
createRule({
...getNewRule(),
...getCustomQueryRuleParams({
enabled: true,
}),
}).then((rule) => {
cy.wrap(rule.body.id).as('ruleId');
});
});

it('should display the execution log', function () {
visit(ruleDetailsUrl(this.ruleId));
waitForAlertsToPopulate();
goToExecutionLogTab();

cy.waitUntil(
() => {
cy.log('Waiting for execution logs to appear in execution log table');
refreshRuleExecutionTable();
return getExecutionLogTableRow().then((rows) => {
return rows.length > 0;
});
},
{ interval: 5000, timeout: 20000 }
);
Comment on lines +51 to +60
Copy link
Copy Markdown
Contributor

@xcrzx xcrzx Aug 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to wait for a rule execution before scheduling a rule run? These actions don’t seem related. Would be nice to add a comment here explaining why this step is necessary.

Copy link
Copy Markdown
Contributor Author

@banderror banderror Aug 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xcrzx This block of code is a replacement for the removed waitForAlertsToPopulate() which was a weird logic for waiting for a rule execution to finish, and wasn't working anyways.

My goal was to just fix and unskip the test while keeping its original logic and structure introduced by @nkhristinin. Refactoring it or changing what we test here would be out of the scope of this PR. That said, I agree that "These actions don’t seem related" and we need to rework the whole test. I'd also argue that calling the manual rule run API should be owned by the DE team, so even more consideration and rework might be needed here.

I can create a ticket for follow-up work. Is there anything you want me to do in this PR?

Copy link
Copy Markdown
Contributor

@xcrzx xcrzx Aug 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I thought you might know the reason, but thanks for the explanation anyway.

This test kind of sits between the responsibilities of two teams, so it’s not that easy to say who should own it. But if it relies on intricate knowledge of how manual runs work under the hood, it would be better to transfer it to DE.

Copy link
Copy Markdown
Contributor Author

@banderror banderror Aug 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test kind of sits between the responsibilities of two teams, so it’s not that easy to say who should own it. But if it relies on intricate knowledge of how manual runs work under the hood, it would be better to transfer it to DE.

Yeah, I agree. Ok, I'll get back to this in the coming days and open a follow-up ticket.


cy.get(EXECUTION_SHOWING).contains('Showing 1 rule execution');
getExecutionLogTableRow().should('have.length', 1);

cy.log('Scheduling a manual rule run');
manualRuleRun({
ruleId: this.ruleId,
start: moment().subtract(5, 'm').toISOString(),
Expand All @@ -62,14 +74,16 @@ describe.skip(
cy.log('Waiting for execution logs to appear in execution log table');
refreshRuleExecutionTable();
return getExecutionLogTableRow().then((rows) => {
return rows.length === 2;
return rows.length > 1;
});
},
{ interval: 5000, timeout: 20000 }
);

cy.get(EXECUTION_SHOWING).contains('Showing 2 rule executions');
filterByRunType('Manual');
getExecutionLogTableRow().should('have.length', 2);

filterByRunType('Manual');
getExecutionLogTableRow().should('have.length', 1);
});
}
Expand Down
3 changes: 1 addition & 2 deletions x-pack/solutions/security/test/security_solution_cypress/cypress/tasks/api_calls/backfill.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,7 @@ export const manualRuleRun = ({
body: [
{
rule_id: ruleId,
start,
end,
ranges: [{ start, end }],
},
],
});
Expand Down