Skip to content

Commit 3aa0bc7

Browse files
pgayvalletpgayvallet
committed
address some review comments
1 parent c72c20f commit 3aa0bc7

5 files changed

Lines changed: 45 additions & 61 deletions

File tree

‎src/core/server/logging/appenders/rolling_file/strategies/fs.ts‎

Lines changed: 0 additions & 26 deletions
This file was deleted.

‎src/core/server/logging/appenders/rolling_file/strategies/numeric/rolling_tasks.test.mocks.ts‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -20,18 +20,18 @@
2020
export const readdirMock = jest.fn();
2121
export const unlinkMock = jest.fn();
2222
export const renameMock = jest.fn();
23-
export const existsMock = jest.fn();
23+
export const accessMock = jest.fn();
2424

25-
jest.doMock('../fs', () => ({
25+
jest.doMock('fs/promises', () => ({
2626
readdir: readdirMock,
2727
unlink: unlinkMock,
2828
rename: renameMock,
29-
exists: existsMock,
29+
access: accessMock,
3030
}));
3131

3232
export const clearAllMocks = () => {
3333
readdirMock.mockClear();
3434
unlinkMock.mockClear();
3535
renameMock.mockClear();
36-
existsMock.mockClear();
36+
accessMock.mockClear();
3737
};

‎src/core/server/logging/appenders/rolling_file/strategies/numeric/rolling_tasks.test.ts‎

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@
1919

2020
import { join } from 'path';
2121
import {
22-
existsMock,
22+
accessMock,
2323
readdirMock,
2424
renameMock,
2525
unlinkMock,
@@ -42,16 +42,18 @@ describe('NumericRollingStrategy tasks', () => {
4242
it('calls `exists` with the correct parameters', async () => {
4343
await shouldSkipRollout({ logFilePath: 'some-file' });
4444

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

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

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

129131
describe('getOrderedRolledFiles', () => {
130132
it('returns the rolled files matching the pattern in order', async () => {
131-
readdirMock.mockResolvedValue(['kibana-3.log', 'kibana-1.log', 'kibana-2.log']);
133+
readdirMock.mockResolvedValue([
134+
'kibana-10.log',
135+
'kibana-1.log',
136+
'kibana-12.log',
137+
'kibana-2.log',
138+
]);
132139

133140
const files = await getOrderedRolledFiles({
134141
logFileFolder: 'log-folder',
135142
logFileBaseName: 'kibana.log',
136143
pattern: '-%i',
137144
});
138145

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

142149
it('ignores files that do no match the pattern', async () => {

‎src/core/server/logging/appenders/rolling_file/strategies/numeric/rolling_tasks.ts‎

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -18,14 +18,19 @@
1818
*/
1919

2020
import { join } from 'path';
21+
import { readdir, rename, unlink, access } from 'fs/promises';
2122
import { getFileNameMatcher, getRollingFileName } from './pattern_matcher';
22-
import { readdir, rename, unlink, exists } from '../fs';
2323

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

3136
/**
@@ -59,9 +64,7 @@ export const deleteFiles = async ({
5964
logFileFolder: string;
6065
filesToDelete: string[];
6166
}) => {
62-
for (const fileToDelete of filesToDelete) {
63-
await unlink(join(logFileFolder, fileToDelete));
64-
}
67+
await Promise.all(filesToDelete.map((fileToDelete) => unlink(join(logFileFolder, fileToDelete))));
6568
};
6669

6770
export const rollPreviousFilesInOrder = async ({

‎src/core/server/logging/integration_tests/rolling_file_appender.test.ts‎

Lines changed: 19 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@
1818
*/
1919

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

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

5656
beforeEach(async () => {
57-
testDir = mkdtempSync('rolling-test');
57+
testDir = await mkdtemp('rolling-test');
5858
logFile = join(testDir, 'kibana.log');
5959
});
6060

6161
afterEach(async () => {
6262
try {
63-
rmdirSync(testDir);
63+
await rmdir(testDir);
6464
} catch (e) {
6565
/* trap */
6666
}
@@ -110,12 +110,12 @@ describe('RollingFileAppender', () => {
110110

111111
await flush();
112112

113-
const files = readdirSync(testDir).sort();
113+
const files = await readdir(testDir);
114114

115-
expect(files).toEqual(['kibana.1.log', 'kibana.2.log', 'kibana.log']);
116-
expect(getFileContent('kibana.log')).toEqual(expectedFileContent([7]));
117-
expect(getFileContent('kibana.1.log')).toEqual(expectedFileContent([4, 5, 6]));
118-
expect(getFileContent('kibana.2.log')).toEqual(expectedFileContent([1, 2, 3]));
115+
expect(files.sort()).toEqual(['kibana.1.log', 'kibana.2.log', 'kibana.log']);
116+
expect(await getFileContent('kibana.log')).toEqual(expectedFileContent([7]));
117+
expect(await getFileContent('kibana.1.log')).toEqual(expectedFileContent([4, 5, 6]));
118+
expect(await getFileContent('kibana.2.log')).toEqual(expectedFileContent([1, 2, 3]));
119119
});
120120

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

158158
await flush();
159159

160-
const files = readdirSync(testDir).sort();
160+
const files = await readdir(testDir);
161161

162-
expect(files).toEqual(['kibana-1.log', 'kibana-2.log', 'kibana.log']);
163-
expect(getFileContent('kibana.log')).toEqual(expectedFileContent([7, 8]));
164-
expect(getFileContent('kibana-1.log')).toEqual(expectedFileContent([5, 6]));
165-
expect(getFileContent('kibana-2.log')).toEqual(expectedFileContent([3, 4]));
162+
expect(files.sort()).toEqual(['kibana-1.log', 'kibana-2.log', 'kibana.log']);
163+
expect(await getFileContent('kibana.log')).toEqual(expectedFileContent([7, 8]));
164+
expect(await getFileContent('kibana-1.log')).toEqual(expectedFileContent([5, 6]));
165+
expect(await getFileContent('kibana-2.log')).toEqual(expectedFileContent([3, 4]));
166166
});
167167
});
168168

@@ -210,11 +210,11 @@ describe('RollingFileAppender', () => {
210210

211211
await flush();
212212

213-
const files = readdirSync(testDir).sort();
213+
const files = await readdir(testDir);
214214

215-
expect(files).toEqual(['kibana-1.log', 'kibana.log']);
216-
expect(getFileContent('kibana.log')).toEqual(expectedFileContent([3, 4]));
217-
expect(getFileContent('kibana-1.log')).toEqual(expectedFileContent([1, 2]));
215+
expect(files.sort()).toEqual(['kibana-1.log', 'kibana.log']);
216+
expect(await getFileContent('kibana.log')).toEqual(expectedFileContent([3, 4]));
217+
expect(await getFileContent('kibana-1.log')).toEqual(expectedFileContent([1, 2]));
218218
});
219219
});
220220
});

0 commit comments

Comments
 (0)