Skip to content

Commit

Permalink
address some review comments
Browse files Browse the repository at this point in the history
  • Loading branch information
pgayvallet committed Dec 8, 2020
1 parent c72c20f commit 3aa0bc7
Show file tree
Hide file tree
Showing 5 changed files with 45 additions and 61 deletions.
26 changes: 0 additions & 26 deletions src/core/server/logging/appenders/rolling_file/strategies/fs.ts

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -20,18 +20,18 @@
export const readdirMock = jest.fn();
export const unlinkMock = jest.fn();
export const renameMock = jest.fn();
export const existsMock = jest.fn();
export const accessMock = jest.fn();

jest.doMock('../fs', () => ({
jest.doMock('fs/promises', () => ({
readdir: readdirMock,
unlink: unlinkMock,
rename: renameMock,
exists: existsMock,
access: accessMock,
}));

export const clearAllMocks = () => {
readdirMock.mockClear();
unlinkMock.mockClear();
renameMock.mockClear();
existsMock.mockClear();
accessMock.mockClear();
};
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@

import { join } from 'path';
import {
existsMock,
accessMock,
readdirMock,
renameMock,
unlinkMock,
Expand All @@ -42,16 +42,18 @@ describe('NumericRollingStrategy tasks', () => {
it('calls `exists` with the correct parameters', async () => {
await shouldSkipRollout({ logFilePath: 'some-file' });

expect(existsMock).toHaveBeenCalledTimes(1);
expect(existsMock).toHaveBeenCalledWith('some-file');
expect(accessMock).toHaveBeenCalledTimes(1);
expect(accessMock).toHaveBeenCalledWith('some-file');
});
it('returns `true` if the file is current log file does not exist', async () => {
existsMock.mockResolvedValue(false);
accessMock.mockImplementation(() => {
throw new Error('ENOENT');
});

expect(await shouldSkipRollout({ logFilePath: 'some-file' })).toEqual(true);
});
it('returns `false` if the file is current log file exists', async () => {
existsMock.mockResolvedValue(true);
accessMock.mockResolvedValue(undefined);

expect(await shouldSkipRollout({ logFilePath: 'some-file' })).toEqual(false);
});
Expand Down Expand Up @@ -128,15 +130,20 @@ describe('NumericRollingStrategy tasks', () => {

describe('getOrderedRolledFiles', () => {
it('returns the rolled files matching the pattern in order', async () => {
readdirMock.mockResolvedValue(['kibana-3.log', 'kibana-1.log', 'kibana-2.log']);
readdirMock.mockResolvedValue([
'kibana-10.log',
'kibana-1.log',
'kibana-12.log',
'kibana-2.log',
]);

const files = await getOrderedRolledFiles({
logFileFolder: 'log-folder',
logFileBaseName: 'kibana.log',
pattern: '-%i',
});

expect(files).toEqual(['kibana-1.log', 'kibana-2.log', 'kibana-3.log']);
expect(files).toEqual(['kibana-1.log', 'kibana-2.log', 'kibana-10.log', 'kibana-12.log']);
});

it('ignores files that do no match the pattern', async () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,19 @@
*/

import { join } from 'path';
import { readdir, rename, unlink, access } from 'fs/promises';
import { getFileNameMatcher, getRollingFileName } from './pattern_matcher';
import { readdir, rename, unlink, exists } from '../fs';

export const shouldSkipRollout = async ({ logFilePath }: { logFilePath: string }) => {
// in case of time-interval triggering policy, we can have an entire
// interval without any log event. In that case, the log file is not even
// present, and we should not perform the rollout
return !(await exists(logFilePath));
try {
await access(logFilePath);
return false;
} catch (e) {
return true;
}
};

/**
Expand Down Expand Up @@ -59,9 +64,7 @@ export const deleteFiles = async ({
logFileFolder: string;
filesToDelete: string[];
}) => {
for (const fileToDelete of filesToDelete) {
await unlink(join(logFileFolder, fileToDelete));
}
await Promise.all(filesToDelete.map((fileToDelete) => unlink(join(logFileFolder, fileToDelete))));
};

export const rollPreviousFilesInOrder = async ({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
*/

import { join } from 'path';
import { rmdirSync, mkdtempSync, readFileSync, readdirSync } from 'fs';
import { rmdir, mkdtemp, readFile, readdir } from 'fs/promises';
import moment from 'moment-timezone';
import * as kbnTestServer from '../../../test_helpers/kbn_server';
import { getNextRollingTime } from '../appenders/rolling_file/policies/time_interval/get_next_rolling_time';
Expand Down Expand Up @@ -50,17 +50,17 @@ describe('RollingFileAppender', () => {
let testDir: string;
let logFile: string;

const getFileContent = (basename: string) =>
readFileSync(join(testDir, basename)).toString('utf-8');
const getFileContent = async (basename: string) =>
(await readFile(join(testDir, basename))).toString('utf-8');

beforeEach(async () => {
testDir = mkdtempSync('rolling-test');
testDir = await mkdtemp('rolling-test');
logFile = join(testDir, 'kibana.log');
});

afterEach(async () => {
try {
rmdirSync(testDir);
await rmdir(testDir);
} catch (e) {
/* trap */
}
Expand Down Expand Up @@ -110,12 +110,12 @@ describe('RollingFileAppender', () => {

await flush();

const files = readdirSync(testDir).sort();
const files = await readdir(testDir);

expect(files).toEqual(['kibana.1.log', 'kibana.2.log', 'kibana.log']);
expect(getFileContent('kibana.log')).toEqual(expectedFileContent([7]));
expect(getFileContent('kibana.1.log')).toEqual(expectedFileContent([4, 5, 6]));
expect(getFileContent('kibana.2.log')).toEqual(expectedFileContent([1, 2, 3]));
expect(files.sort()).toEqual(['kibana.1.log', 'kibana.2.log', 'kibana.log']);
expect(await getFileContent('kibana.log')).toEqual(expectedFileContent([7]));
expect(await getFileContent('kibana.1.log')).toEqual(expectedFileContent([4, 5, 6]));
expect(await getFileContent('kibana.2.log')).toEqual(expectedFileContent([1, 2, 3]));
});

it('only keep the correct number of files', async () => {
Expand Down Expand Up @@ -157,12 +157,12 @@ describe('RollingFileAppender', () => {

await flush();

const files = readdirSync(testDir).sort();
const files = await readdir(testDir);

expect(files).toEqual(['kibana-1.log', 'kibana-2.log', 'kibana.log']);
expect(getFileContent('kibana.log')).toEqual(expectedFileContent([7, 8]));
expect(getFileContent('kibana-1.log')).toEqual(expectedFileContent([5, 6]));
expect(getFileContent('kibana-2.log')).toEqual(expectedFileContent([3, 4]));
expect(files.sort()).toEqual(['kibana-1.log', 'kibana-2.log', 'kibana.log']);
expect(await getFileContent('kibana.log')).toEqual(expectedFileContent([7, 8]));
expect(await getFileContent('kibana-1.log')).toEqual(expectedFileContent([5, 6]));
expect(await getFileContent('kibana-2.log')).toEqual(expectedFileContent([3, 4]));
});
});

Expand Down Expand Up @@ -210,11 +210,11 @@ describe('RollingFileAppender', () => {

await flush();

const files = readdirSync(testDir).sort();
const files = await readdir(testDir);

expect(files).toEqual(['kibana-1.log', 'kibana.log']);
expect(getFileContent('kibana.log')).toEqual(expectedFileContent([3, 4]));
expect(getFileContent('kibana-1.log')).toEqual(expectedFileContent([1, 2]));
expect(files.sort()).toEqual(['kibana-1.log', 'kibana.log']);
expect(await getFileContent('kibana.log')).toEqual(expectedFileContent([3, 4]));
expect(await getFileContent('kibana-1.log')).toEqual(expectedFileContent([1, 2]));
});
});
});

0 comments on commit 3aa0bc7

Please sign in to comment.