Skip to content

Commit

Permalink
Fix flaky test for task state validation (elastic#163744)
Browse files Browse the repository at this point in the history
Fixes elastic#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:
elastic@78561fe
and
elastic@7168537
- Commits where CI ran 25x and succeeded:
elastic@fef05d7,
elastic@f3b2a90,
elastic@7415c93,
and
elastic@74bce2a

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
  • Loading branch information
mikecote and kibanamachine authored Aug 16, 2023
1 parent 643ead4 commit 42964d0
Showing 1 changed file with 48 additions and 36 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
* 2.0.
*/

import { v4 as uuidV4 } from 'uuid';
import {
type TestElasticsearchUtils,
type TestKibanaUtils,
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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' },
Expand All @@ -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];
Expand All @@ -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];
Expand All @@ -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: {},
Expand All @@ -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];
Expand All @@ -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' },
Expand All @@ -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];
Expand All @@ -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;
Expand Down Expand Up @@ -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' },
Expand All @@ -314,6 +325,7 @@ describe('task state validation', () => {
retryAt: null,
ownerId: null,
});
taskIdsToRemove.push(id);

await retry(async () => {
expect(errorLogSpy).toHaveBeenCalledWith(
Expand Down

0 comments on commit 42964d0

Please sign in to comment.