From 42964d07be3ebe9c8c1f1967d38972ea74f52916 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mike=20C=C3=B4t=C3=A9?= Date: Wed, 16 Aug 2023 12:34:36 -0400 Subject: [PATCH] Fix flaky test for task state validation (#163744) Fixes https://github.com/elastic/kibana/issues/161081. In this PR, I'm attempting to reduce the flakiness of the `task_state_validation.test.ts` jest integration test by doing the following changes: - Ensuring each test creates a task with a different ID in case there is a race condition with the previous test that would delete the task as the next test runs. - Modifying how the test waits for the task runner to be called given the previous approach wasn't 100% that the executor would be mocked before the task is scheduled. Since the flaky test runner doesn't work with jest integration tests, I instead added a for loop in my code to run the test 25x, and made CI run it a few times: - Commits creating for loop and setting to 25: https://github.com/elastic/kibana/pull/163744/commits/78561fe2ad71657096373c60ed914b7f53ffd8de and https://github.com/elastic/kibana/pull/163744/commits/716853733edca0c29ac063dde5b9bf275dcbba3a - Commits where CI ran 25x and succeeded: https://github.com/elastic/kibana/pull/163744/commits/fef05d7db204035d23211d7a2a8c755d0ee1367e, https://github.com/elastic/kibana/pull/163744/commits/f3b2a906fe9a6c74f9951de1b7e90710ac57bc63, https://github.com/elastic/kibana/pull/163744/commits/7415c934656d8e5f8ea782a0b9643460704a4d87, and https://github.com/elastic/kibana/pull/163744/commits/74bce2a24f81680e5c7997bbe1a429fe30571052 --------- Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> --- .../task_state_validation.test.ts | 84 +++++++++++-------- 1 file changed, 48 insertions(+), 36 deletions(-) diff --git a/x-pack/plugins/task_manager/server/integration_tests/task_state_validation.test.ts b/x-pack/plugins/task_manager/server/integration_tests/task_state_validation.test.ts index c161be0bbfb78e..3f0aafd563e9c3 100644 --- a/x-pack/plugins/task_manager/server/integration_tests/task_state_validation.test.ts +++ b/x-pack/plugins/task_manager/server/integration_tests/task_state_validation.test.ts @@ -5,6 +5,7 @@ * 2.0. */ +import { v4 as uuidV4 } from 'uuid'; import { type TestElasticsearchUtils, type TestKibanaUtils, @@ -77,8 +78,8 @@ jest.mock('../queries/task_claiming', () => { const taskManagerStartSpy = jest.spyOn(TaskManagerPlugin.prototype, 'start'); describe('task state validation', () => { - // FLAKY: https://github.com/elastic/kibana/issues/161081 - describe.skip('allow_reading_invalid_state: true', () => { + describe('allow_reading_invalid_state: true', () => { + const taskIdsToRemove: string[] = []; let esServer: TestElasticsearchUtils; let kibanaServer: TestKibanaUtils; let taskManagerPlugin: TaskManagerStartContract; @@ -110,19 +111,20 @@ describe('task state validation', () => { }); afterEach(async () => { - await taskManagerPlugin.removeIfExists('foo'); + while (taskIdsToRemove.length > 0) { + const id = taskIdsToRemove.pop(); + await taskManagerPlugin.removeIfExists(id!); + } }); it('should drop unknown fields from the task state', async () => { - const taskRunnerPromise = new Promise((resolve) => { - mockTaskTypeRunFn.mockImplementation(() => { - setTimeout(resolve, 0); - return { state: {} }; - }); + mockTaskTypeRunFn.mockImplementation(() => { + return { state: {} }; }); + const id = uuidV4(); await injectTask(kibanaServer.coreStart.elasticsearch.client.asInternalUser, { - id: 'foo', + id, taskType: 'fooType', params: { foo: true }, state: { foo: 'test', bar: 'test', baz: 'test', invalidProperty: 'invalid' }, @@ -136,8 +138,11 @@ describe('task state validation', () => { retryAt: null, ownerId: null, }); + taskIdsToRemove.push(id); - await taskRunnerPromise; + await retry(async () => { + expect(mockTaskTypeRunFn).toHaveBeenCalled(); + }); expect(mockCreateTaskRunner).toHaveBeenCalledTimes(1); const call = mockCreateTaskRunner.mock.calls[0][0]; @@ -150,22 +155,21 @@ describe('task state validation', () => { it('should fail to update the task if the task runner returns an unknown property in the state', async () => { const errorLogSpy = jest.spyOn(pollingLifecycleOpts.logger, 'error'); - const taskRunnerPromise = new Promise((resolve) => { - mockTaskTypeRunFn.mockImplementation(() => { - setTimeout(resolve, 0); - return { state: { invalidField: true, foo: 'test', bar: 'test', baz: 'test' } }; - }); + mockTaskTypeRunFn.mockImplementation(() => { + return { state: { invalidField: true, foo: 'test', bar: 'test', baz: 'test' } }; }); - await taskManagerPlugin.schedule({ - id: 'foo', + const task = await taskManagerPlugin.schedule({ taskType: 'fooType', params: {}, state: { foo: 'test', bar: 'test', baz: 'test' }, schedule: { interval: '1d' }, }); + taskIdsToRemove.push(task.id); - await taskRunnerPromise; + await retry(async () => { + expect(mockTaskTypeRunFn).toHaveBeenCalled(); + }); expect(mockCreateTaskRunner).toHaveBeenCalledTimes(1); const call = mockCreateTaskRunner.mock.calls[0][0]; @@ -175,21 +179,19 @@ describe('task state validation', () => { baz: 'test', }); expect(errorLogSpy).toHaveBeenCalledWith( - 'Task fooType "foo" failed: Error: [invalidField]: definition for this key is missing', + `Task fooType "${task.id}" failed: Error: [invalidField]: definition for this key is missing`, expect.anything() ); }); it('should migrate the task state', async () => { - const taskRunnerPromise = new Promise((resolve) => { - mockTaskTypeRunFn.mockImplementation(() => { - setTimeout(resolve, 0); - return { state: {} }; - }); + mockTaskTypeRunFn.mockImplementation(() => { + return { state: {} }; }); + const id = uuidV4(); await injectTask(kibanaServer.coreStart.elasticsearch.client.asInternalUser, { - id: 'foo', + id, taskType: 'fooType', params: { foo: true }, state: {}, @@ -202,8 +204,11 @@ describe('task state validation', () => { retryAt: null, ownerId: null, }); + taskIdsToRemove.push(id); - await taskRunnerPromise; + await retry(async () => { + expect(mockTaskTypeRunFn).toHaveBeenCalled(); + }); expect(mockCreateTaskRunner).toHaveBeenCalledTimes(1); const call = mockCreateTaskRunner.mock.calls[0][0]; @@ -216,15 +221,13 @@ describe('task state validation', () => { it('should debug log by default when reading an invalid task state', async () => { const debugLogSpy = jest.spyOn(pollingLifecycleOpts.logger, 'debug'); - const taskRunnerPromise = new Promise((resolve) => { - mockTaskTypeRunFn.mockImplementation(() => { - setTimeout(resolve, 0); - return { state: {} }; - }); + mockTaskTypeRunFn.mockImplementation(() => { + return { state: {} }; }); + const id = uuidV4(); await injectTask(kibanaServer.coreStart.elasticsearch.client.asInternalUser, { - id: 'foo', + id, taskType: 'fooType', params: { foo: true }, state: { foo: true, bar: 'test', baz: 'test' }, @@ -238,8 +241,11 @@ describe('task state validation', () => { retryAt: null, ownerId: null, }); + taskIdsToRemove.push(id); - await taskRunnerPromise; + await retry(async () => { + expect(mockTaskTypeRunFn).toHaveBeenCalled(); + }); expect(mockCreateTaskRunner).toHaveBeenCalledTimes(1); const call = mockCreateTaskRunner.mock.calls[0][0]; @@ -250,12 +256,13 @@ describe('task state validation', () => { }); expect(debugLogSpy).toHaveBeenCalledWith( - `[fooType][foo] Failed to validate the task's state. Allowing read operation to proceed because allow_reading_invalid_state is true. Error: [foo]: expected value of type [string] but got [boolean]` + `[fooType][${id}] Failed to validate the task's state. Allowing read operation to proceed because allow_reading_invalid_state is true. Error: [foo]: expected value of type [string] but got [boolean]` ); }); }); describe('allow_reading_invalid_state: false', () => { + const taskIdsToRemove: string[] = []; let esServer: TestElasticsearchUtils; let kibanaServer: TestKibanaUtils; let taskManagerPlugin: TaskManagerStartContract; @@ -293,14 +300,18 @@ describe('task state validation', () => { }); afterEach(async () => { - await taskManagerPlugin.removeIfExists('foo'); + while (taskIdsToRemove.length > 0) { + const id = taskIdsToRemove.pop(); + await taskManagerPlugin.removeIfExists(id!); + } }); it('should fail the task run when setting allow_reading_invalid_state:false and reading an invalid state', async () => { const errorLogSpy = jest.spyOn(pollingLifecycleOpts.logger, 'error'); + const id = uuidV4(); await injectTask(kibanaServer.coreStart.elasticsearch.client.asInternalUser, { - id: 'foo', + id, taskType: 'fooType', params: { foo: true }, state: { foo: true, bar: 'test', baz: 'test' }, @@ -314,6 +325,7 @@ describe('task state validation', () => { retryAt: null, ownerId: null, }); + taskIdsToRemove.push(id); await retry(async () => { expect(errorLogSpy).toHaveBeenCalledWith(