Increase alerting test stability and reduce flakiness (elastic#50246) (…

…elastic#50944) * Increase alerting test stability * More changes to test methodology + bug fix in throttling * Fix comments * Cleanup * Typo * Fix broken tests * Fix integration tests * Fix typo
dedemorton · Nov 18, 2019 · a4a0e0a · a4a0e0a
1 parent 206ac7e
commit a4a0e0a
Show file tree

Hide file tree

Showing 16 changed files with 226 additions and 95 deletions.
diff --git a/x-pack/legacy/plugins/alerting/server/lib/alert_instance.test.ts b/x-pack/legacy/plugins/alerting/server/lib/alert_instance.test.ts
@@ -18,9 +18,17 @@ afterAll(() => clock.restore());
 describe('hasScheduledActions()', () => {
   test('defaults to false', () => {
     const alertInstance = new AlertInstance();
-    expect(alertInstance.hasScheduledActions(null)).toEqual(false);
+    expect(alertInstance.hasScheduledActions()).toEqual(false);
   });
 
+  test('returns true when scheduleActions is called', () => {
+    const alertInstance = new AlertInstance();
+    alertInstance.scheduleActions('default');
+    expect(alertInstance.hasScheduledActions()).toEqual(true);
+  });
+});
+
+describe('isThrottled', () => {
   test(`should throttle when group didn't change and throttle period is still active`, () => {
     const alertInstance = new AlertInstance({
       meta: {
@@ -32,7 +40,7 @@ describe('hasScheduledActions()', () => {
     });
     clock.tick(30000);
     alertInstance.scheduleActions('default');
-    expect(alertInstance.hasScheduledActions('1m')).toEqual(false);
+    expect(alertInstance.isThrottled('1m')).toEqual(true);
   });
 
   test(`shouldn't throttle when group didn't change and throttle period expired`, () => {
@@ -46,7 +54,7 @@ describe('hasScheduledActions()', () => {
     });
     clock.tick(30000);
     alertInstance.scheduleActions('default');
-    expect(alertInstance.hasScheduledActions('15s')).toEqual(true);
+    expect(alertInstance.isThrottled('15s')).toEqual(false);
   });
 
   test(`shouldn't throttle when group changes`, () => {
@@ -60,7 +68,7 @@ describe('hasScheduledActions()', () => {
     });
     clock.tick(5000);
     alertInstance.scheduleActions('other-group');
-    expect(alertInstance.hasScheduledActions('1m')).toEqual(true);
+    expect(alertInstance.isThrottled('1m')).toEqual(false);
   });
 });
 
@@ -75,9 +83,9 @@ describe('unscheduleActions()', () => {
   test('makes hasScheduledActions() return false', () => {
     const alertInstance = new AlertInstance();
     alertInstance.scheduleActions('default');
-    expect(alertInstance.hasScheduledActions(null)).toEqual(true);
+    expect(alertInstance.hasScheduledActions()).toEqual(true);
     alertInstance.unscheduleActions();
-    expect(alertInstance.hasScheduledActions(null)).toEqual(false);
+    expect(alertInstance.hasScheduledActions()).toEqual(false);
   });
 
   test('makes getScheduledActionOptions() return undefined', () => {
@@ -113,10 +121,10 @@ describe('scheduleActions()', () => {
       },
     });
     alertInstance.replaceState({ otherField: true }).scheduleActions('default', { field: true });
-    expect(alertInstance.hasScheduledActions(null)).toEqual(true);
+    expect(alertInstance.hasScheduledActions()).toEqual(true);
   });
 
-  test('makes hasScheduledActions() return false when throttled', () => {
+  test('makes isThrottled() return true when throttled', () => {
     const alertInstance = new AlertInstance({
       state: { foo: true },
       meta: {
@@ -127,10 +135,10 @@ describe('scheduleActions()', () => {
       },
     });
     alertInstance.replaceState({ otherField: true }).scheduleActions('default', { field: true });
-    expect(alertInstance.hasScheduledActions('1m')).toEqual(false);
+    expect(alertInstance.isThrottled('1m')).toEqual(true);
   });
 
-  test('make hasScheduledActions() return true when throttled expired', () => {
+  test('make isThrottled() return false when throttled expired', () => {
     const alertInstance = new AlertInstance({
       state: { foo: true },
       meta: {
@@ -142,7 +150,7 @@ describe('scheduleActions()', () => {
     });
     clock.tick(120000);
     alertInstance.replaceState({ otherField: true }).scheduleActions('default', { field: true });
-    expect(alertInstance.hasScheduledActions('1m')).toEqual(true);
+    expect(alertInstance.isThrottled('1m')).toEqual(false);
   });
 
   test('makes getScheduledActionOptions() return given options', () => {

diff --git a/x-pack/legacy/plugins/alerting/server/lib/alert_instance.ts b/x-pack/legacy/plugins/alerting/server/lib/alert_instance.ts
@@ -35,23 +35,24 @@ export class AlertInstance {
     this.meta = meta;
   }
 
-  hasScheduledActions(throttle: string | null) {
-    // scheduleActions function wasn't called
+  hasScheduledActions() {
+    return this.scheduledExecutionOptions !== undefined;
+  }
+
+  isThrottled(throttle: string | null) {
     if (this.scheduledExecutionOptions === undefined) {
       return false;
     }
-    // Shouldn't schedule actions if still within throttling window
-    // Reset if actionGroup changes
     const throttleMills = throttle ? parseDuration(throttle) : 0;
     const actionGroup = this.scheduledExecutionOptions.actionGroup;
     if (
       this.meta.lastScheduledActions &&
       this.meta.lastScheduledActions.group === actionGroup &&
       new Date(this.meta.lastScheduledActions.date).getTime() + throttleMills > Date.now()
     ) {
-      return false;
+      return true;
     }
-    return true;
+    return false;
   }
 
   getScheduledActionOptions() {
@@ -68,7 +69,7 @@ export class AlertInstance {
   }
 
   scheduleActions(actionGroup: string, context: Context = {}) {
-    if (this.hasScheduledActions(null)) {
+    if (this.hasScheduledActions()) {
       throw new Error('Alert instance execution has already been scheduled, cannot schedule twice');
     }
     this.scheduledExecutionOptions = { actionGroup, context, state: this.state };

diff --git a/x-pack/legacy/plugins/alerting/server/lib/task_runner_factory.ts b/x-pack/legacy/plugins/alerting/server/lib/task_runner_factory.ts
@@ -148,8 +148,12 @@ export class TaskRunnerFactory {
         await Promise.all(
           Object.keys(alertInstances).map(alertInstanceId => {
             const alertInstance = alertInstances[alertInstanceId];
-            if (alertInstance.hasScheduledActions(throttle)) {
-              if (muteAll || mutedInstanceIds.includes(alertInstanceId)) {
+            if (alertInstance.hasScheduledActions()) {
+              if (
+                alertInstance.isThrottled(throttle) ||
+                muteAll ||
+                mutedInstanceIds.includes(alertInstanceId)
+              ) {
                 return;
               }
               const { actionGroup, context, state } = alertInstance.getScheduledActionOptions()!;

diff --git a/x-pack/scripts/functional_tests.js b/x-pack/scripts/functional_tests.js
@@ -13,11 +13,7 @@ require('@kbn/test').runTestsCli([
   require.resolve('../test/api_integration/config_security_basic.js'),
   require.resolve('../test/api_integration/config.js'),
   require.resolve('../test/alerting_api_integration/spaces_only/config.ts'),
-  // FLAKY: https://github.com/elastic/kibana/issues/50079
-  // FLAKY: https://github.com/elastic/kibana/issues/50074
-  // FLAKY: https://github.com/elastic/kibana/issues/48709
-  // FLAKY: https://github.com/elastic/kibana/issues/50078
-  // require.resolve('../test/alerting_api_integration/security_and_spaces/config.ts'),
+  require.resolve('../test/alerting_api_integration/security_and_spaces/config.ts'),
   require.resolve('../test/plugin_api_integration/config.js'),
   require.resolve('../test/kerberos_api_integration/config'),
   require.resolve('../test/kerberos_api_integration/anonymous_access.config'),

diff --git a/x-pack/test/alerting_api_integration/common/lib/get_test_alert_data.ts b/x-pack/test/alerting_api_integration/common/lib/get_test_alert_data.ts
@@ -9,7 +9,7 @@ export function getTestAlertData(overwrites = {}) {
     enabled: true,
     name: 'abc',
     alertTypeId: 'test.noop',
-    interval: '10s',
+    interval: '1m',
     throttle: '1m',
     actions: [],
     alertTypeParams: {},

diff --git a/x-pack/test/alerting_api_integration/common/lib/index.ts b/x-pack/test/alerting_api_integration/common/lib/index.ts
@@ -9,3 +9,4 @@ export { getUrlPrefix } from './space_test_utils';
 export { ES_TEST_INDEX_NAME, ESTestIndexTool } from './es_test_index_tool';
 export { getTestAlertData } from './get_test_alert_data';
 export { AlertUtils } from './alert_utils';
+export { TaskManagerUtils } from './task_manager_utils';
diff --git a/x-pack/test/alerting_api_integration/common/lib/task_manager_utils.ts b/x-pack/test/alerting_api_integration/common/lib/task_manager_utils.ts
@@ -0,0 +1,46 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License;
+ * you may not use this file except in compliance with the Elastic License.
+ */
+
+export class TaskManagerUtils {
+  private readonly es: any;
+  private readonly retry: any;
+
+  constructor(es: any, retry: any) {
+    this.es = es;
+    this.retry = retry;
+  }
+
+  async waitForIdle(taskRunAtFilter: Date) {
+    return await this.retry.try(async () => {
+      const searchResult = await this.es.search({
+        index: '.kibana_task_manager',
+        body: {
+          query: {
+            bool: {
+              must: [
+                {
+                  terms: {
+                    'task.scope': ['actions', 'alerting'],
+                  },
+                },
+                {
+                  range: {
+                    'task.scheduledAt': {
+                      gte: taskRunAtFilter,
+                    },
+                  },
+                },
+              ],
+            },
+          },
+        },
+      });
+      if (searchResult.hits.total.value) {
+        throw new Error(`Expected 0 tasks but received ${searchResult.hits.total.value}`);
+      }
+    });
+  }
+}