Skip to content
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

[Alerts] Fixing skipped unit tests for metric threshold rule #162189

Merged
merged 5 commits into from
Jul 19, 2023
Merged

[Alerts] Fixing skipped unit tests for metric threshold rule #162189

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1a7ddb3
[Alerts] Fixing skipped unit tests for metric threshold rule
simianhacker Jul 18, 2023
9b85a38
Removing skipped from threshold executor
simianhacker Jul 19, 2023
5e47f7e
Merge branch 'main' of github.com:elastic/kibana into issue-155534-fi…
simianhacker Jul 19, 2023
47eb623
Skipping threshold_executor.test.ts because the test needs more work …
simianhacker Jul 19, 2023
c4a5c0b
Merge branch 'main' of github.com:elastic/kibana into issue-155534-fi…
simianhacker Jul 19, 2023
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
4 changes: 2 additions & 2 deletions x-pack/plugins/infra/server/lib/alerting/metric_threshold/metric_threshold_executor.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ const setEvaluationResults = (response: Array<Record<string, Evaluation>>) => {
};

// FAILING: https://github.com/elastic/kibana/issues/155534
describe.skip('The metric threshold alert type', () => {
describe('The metric threshold alert type', () => {
describe('querying the entire infrastructure', () => {
afterAll(() => clearInstances());
const instanceID = '*';
Expand Down Expand Up @@ -1401,7 +1401,7 @@ describe.skip('The metric threshold alert type', () => {
await execute(true);
const recentAction = mostRecentAction(instanceID);
expect(recentAction.action).toEqual({
alertDetailsUrl: 'http://localhost:5601/app/observability/alerts/mock-alert-uuid',
alertDetailsUrl: '',
alertState: 'NO DATA',
group: '*',
groupByKeys: undefined,
Expand Down
4 changes: 3 additions & 1 deletion x-pack/plugins/infra/server/lib/alerting/metric_threshold/metric_threshold_executor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -311,7 +311,9 @@ export const createMetricThresholdExecutor = (libs: InfraBackendLibs) =>
);

const evaluationValues = alertResults.reduce((acc: Array<number | null>, result) => {
acc.push(result[group].currentValue);
if (result[group]) {
acc.push(result[group].currentValue);
}
return acc;
}, []);

Expand Down
2 changes: 1 addition & 1 deletion x-pack/plugins/observability/server/lib/rules/threshold/threshold_executor.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1394,7 +1394,7 @@ describe.skip('The metric threshold alert type', () => {
await execute(true);
const recentAction = mostRecentAction(instanceID);
expect(recentAction.action).toEqual({
alertDetailsUrl: 'http://localhost:5601/app/observability/alerts/mock-alert-uuid',
alertDetailsUrl: '',
Copy link
Member

@maryam-saeidi maryam-saeidi Jul 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Can we also remove the skip from this test? If not, I will create a separate ticket for it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Whoops... I missed that.

Copy link
Member Author

Choose a reason for hiding this comment

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

That test needs a lot more work before we are ready to remove the skip statement. It's failing beyond just the single test this PR is fixing.

Copy link
Member

@maryam-saeidi maryam-saeidi Jul 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I thought so, here is the ticket for it: #162299

alertState: 'NO DATA',
group: '*',
groupByKeys: undefined,
Expand Down
4 changes: 3 additions & 1 deletion x-pack/plugins/observability/server/lib/rules/threshold/threshold_executor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -311,7 +311,9 @@ export const createMetricThresholdExecutor = ({
);

const evaluationValues = alertResults.reduce((acc: Array<number | null>, result) => {
acc.push(result[group].currentValue);
if (result[group]) {
acc.push(result[group].currentValue);
}
return acc;
}, []);

Expand Down