Skip to content

Commit 54657a5

Browse files
dario-piotrowiczdario-piotrowicz
committed
test: refactor repl save-load tests
refactor the test/parallel/test-repl-save-load.js file by: - making the tests in the file self-contained (instead of all of them sharing the same REPL instance and constantly calling `.clear` on it) - clearly separe and commend the various tests to make clearer what is being tested
1 parent 139c2e1 commit 54657a5

File tree

2 files changed

+181
-130
lines changed

2 files changed

+181
-130
lines changed

‎test/common/tmpdir.js‎

Lines changed: 41 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -37,8 +37,11 @@ const testRoot = process.env.NODE_TEST_DIR ?
3737

3838
// Using a `.` prefixed name, which is the convention for "hidden" on POSIX,
3939
// gets tools to ignore it by default or by simple rules, especially eslint.
40-
const tmpdirName = '.tmp.' +
41-
(process.env.TEST_SERIAL_ID || process.env.TEST_THREAD_ID || '0');
40+
const tmpDirPrefix = '.tmp.';
41+
42+
const processTestId = (process.env.TEST_SERIAL_ID || process.env.TEST_THREAD_ID || '0');
43+
44+
const tmpdirName = tmpDirPrefix + processTestId;
4245
let tmpPath = path.join(testRoot, tmpdirName);
4346

4447
let firstRefresh = true;
@@ -56,28 +59,48 @@ function refresh(useSpawn = false) {
5659
}
5760
}
5861

62+
const randomTmpDirPrefix = `${tmpDirPrefix}__random__${processTestId}__`;
63+
64+
const randomTmpDirs = [];
65+
66+
function random() {
67+
const randomUUID = crypto.randomUUID();
68+
const randomTmpdirName = `${randomTmpDirPrefix}${randomUUID}`;
69+
const randomTmpPath = path.join(testRoot, randomTmpdirName);
70+
randomTmpDirs.push(randomTmpPath);
71+
fs.mkdirSync(randomTmpPath);
72+
return randomTmpPath;
73+
}
74+
5975
function onexit(useSpawn) {
6076
// Change directory to avoid possible EBUSY
6177
if (isMainThread)
6278
process.chdir(testRoot);
6379

64-
try {
65-
rmSync(tmpPath, useSpawn);
66-
} catch (e) {
67-
console.error('Can\'t clean tmpdir:', tmpPath);
68-
69-
const files = fs.readdirSync(tmpPath);
70-
console.error('Files blocking:', files);
80+
for (const tmpDirPath of [ tmpPath, ...randomTmpDirs ]) {
81+
try {
82+
rmSync(tmpDirPath, useSpawn);
83+
} catch (e) {
84+
const errorMessage =
85+
tmpDirPath.startsWith(randomTmpDirPrefix) ?
86+
"Can't clean random tmpdir:" :
87+
"Can't clean tmpdir:";
88+
89+
console.error(errorMessage, tmpDirPath);
90+
91+
const files = fs.readdirSync(tmpDirPath);
92+
console.error('Files blocking:', files);
93+
94+
if (files.some((f) => f.startsWith('.nfs'))) {
95+
// Warn about NFS "silly rename"
96+
console.error('Note: ".nfs*" might be files that were open and ' +
97+
'unlinked but not closed.');
98+
console.error('See http://nfs.sourceforge.net/#faq_d2 for details.');
99+
}
71100

72-
if (files.some((f) => f.startsWith('.nfs'))) {
73-
// Warn about NFS "silly rename"
74-
console.error('Note: ".nfs*" might be files that were open and ' +
75-
'unlinked but not closed.');
76-
console.error('See http://nfs.sourceforge.net/#faq_d2 for details.');
101+
console.error();
102+
throw e;
77103
}
78-
79-
console.error();
80-
throw e;
81104
}
82105
}
83106

@@ -102,6 +125,7 @@ module.exports = {
102125
hasEnoughSpace,
103126
refresh,
104127
resolve,
128+
random,
105129

106130
get path() {
107131
return tmpPath;

‎test/parallel/test-repl-save-load.js‎

Lines changed: 140 additions & 113 deletions
Original file line numberDiff line numberDiff line change
@@ -20,162 +20,189 @@
2020
// USE OR OTHER DEALINGS IN THE SOFTWARE.
2121

2222
'use strict';
23+
2324
const common = require('../common');
2425
const ArrayStream = require('../common/arraystream');
25-
const assert = require('assert');
26-
const fs = require('fs');
26+
27+
const assert = require('node:assert');
28+
const fs = require('node:fs');
29+
const repl = require('node:repl');
30+
const path = require('node:path');
31+
2732
const tmpdir = require('../common/tmpdir');
2833
tmpdir.refresh();
2934

30-
// TODO: the following async IIFE and the completePromise function are necessary because
31-
// the reply tests are all run against the same repl instance (testMe) and thus coordination
32-
// needs to be in place for the tests not to interfere with each other, this is really
33-
// not ideal, the tests in this file should be refactored so that each use its own isolated
34-
// repl instance, making sure that no special coordination needs to be in place for them
35-
// and also allowing the tests to all be run in parallel
36-
(async () => {
37-
const repl = require('repl');
38-
39-
const works = [['inner.one'], 'inner.o'];
40-
41-
const putIn = new ArrayStream();
42-
const testMe = repl.start('', putIn);
43-
44-
// Some errors might be passed to the domain.
45-
testMe._domain.on('error', function(reason) {
46-
const err = new Error('Test failed');
47-
err.reason = reason;
48-
throw err;
35+
function prepareREPL(replOpts = {}) {
36+
const input = new ArrayStream();
37+
const output = new ArrayStream();
38+
39+
const replServer = repl.start({
40+
prompt: '',
41+
input,
42+
output,
43+
allowBlockingCompletions: true,
44+
...replOpts,
4945
});
5046

51-
async function completePromise(query, callback) {
52-
return new Promise((resolve) => {
53-
testMe.complete(query, (...args) => {
54-
callback(...args);
55-
resolve();
56-
});
57-
});
58-
}
47+
// Some errors are passed to the domain, but do not callback
48+
replServer._domain.on('error', assert.ifError);
49+
50+
return { replServer, input, output };
51+
}
5952

60-
const testFile = [
53+
// The tests in this file test the REPL saving and loading session data to/from a file
54+
55+
56+
// The REPL can save a session's data to a file and load it back
57+
{
58+
const { replServer, input } = prepareREPL();
59+
const tmpDirPath = tmpdir.random();
60+
61+
const filePath = path.resolve(tmpDirPath, 'test.save.js');
62+
63+
const testFileContents = [
6164
'let inner = (function() {',
6265
' return {one:1};',
6366
'})()',
6467
];
65-
const saveFileName = tmpdir.resolve('test.save.js');
6668

67-
// Add some data.
68-
putIn.run(testFile);
69+
input.run(testFileContents);
70+
input.run([`.save ${filePath}`]);
6971

70-
// Save it to a file.
71-
putIn.run([`.save ${saveFileName}`]);
72+
assert.strictEqual(fs.readFileSync(filePath, 'utf8'),
73+
testFileContents.join('\n'));
7274

73-
// The file should have what I wrote.
74-
assert.strictEqual(fs.readFileSync(saveFileName, 'utf8'),
75-
testFile.join('\n'));
75+
const innerOCompletions = [['inner.one'], 'inner.o'];
7676

77-
// Make sure that the REPL data is "correct".
78-
await completePromise('inner.o', common.mustSucceed((data) => {
79-
assert.deepStrictEqual(data, works);
77+
// Double check that the data is still present in the repl after the save
78+
replServer.completer('inner.o', common.mustSucceed((data) => {
79+
assert.deepStrictEqual(data, innerOCompletions);
8080
}));
8181

82-
// Clear the REPL.
83-
putIn.run(['.clear']);
82+
// Clear the repl context
83+
input.run(['.clear']);
84+
85+
// Double check that the data is no longer present in the repl
86+
replServer.completer('inner.o', common.mustSucceed((data) => {
87+
assert.deepStrictEqual(data, [[], 'inner.o']);
88+
}));
8489

85-
testMe._sawKeyPress = true;
8690
// Load the file back in.
87-
putIn.run([`.load ${saveFileName}`]);
91+
input.run([`.load ${filePath}`]);
8892

8993
// Make sure loading doesn't insert extra indentation
9094
// https://github.com/nodejs/node/issues/47673
91-
assert.strictEqual(testMe.line, '');
95+
assert.strictEqual(replServer.line, '');
9296

93-
// Make sure that the REPL data is "correct".
94-
await completePromise('inner.o', common.mustSucceed((data) => {
95-
assert.deepStrictEqual(data, works);
97+
// Make sure that the loaded data is present
98+
replServer.complete('inner.o', common.mustSucceed((data) => {
99+
assert.deepStrictEqual(data, innerOCompletions);
96100
}));
97101

98-
// Clear the REPL.
99-
putIn.run(['.clear']);
102+
replServer.close();
103+
}
100104

101-
let loadFile = tmpdir.resolve('file.does.not.exist');
105+
// An appropriate error is displayed if .load is called without a filename
106+
{
107+
const { replServer, input, output } = prepareREPL();
102108

103-
// Should not break.
104-
putIn.write = common.mustCall(function(data) {
105-
// Make sure I get a failed to load message and not some crazy error.
106-
assert.strictEqual(data, `Failed to load: ${loadFile}\n`);
107-
// Eat me to avoid work.
108-
putIn.write = () => {};
109+
output.write = common.mustCall(function(data) {
110+
assert.strictEqual(data, 'The "file" argument must be specified\n');
111+
output.write = () => {};
109112
});
110-
putIn.run([`.load ${loadFile}`]);
111113

112-
// Throw error on loading directory.
113-
loadFile = tmpdir.path;
114-
putIn.write = common.mustCall(function(data) {
115-
assert.strictEqual(data, `Failed to load: ${loadFile} is not a valid file\n`);
116-
putIn.write = () => {};
114+
input.run(['.load']);
115+
116+
replServer.close();
117+
}
118+
119+
// An appropriate error is displayed if .save is called without a filename
120+
{
121+
const { replServer, input, output } = prepareREPL();
122+
123+
output.write = common.mustCall(function(data) {
124+
assert.strictEqual(data, 'The "file" argument must be specified\n');
125+
output.write = () => {};
117126
});
118-
putIn.run([`.load ${loadFile}`]);
119127

120-
// Clear the REPL.
121-
putIn.run(['.clear']);
128+
input.run(['.save']);
129+
130+
replServer.close();
131+
}
132+
133+
// The case in which the user tries to load a non existing file is appropriately handled
134+
{
135+
const { replServer, input, output } = prepareREPL();
136+
137+
const filePath = tmpdir.resolve('file.does.not.exist');
138+
139+
output.write = common.mustCall(function(data) {
140+
assert.strictEqual(data, `Failed to load: ${filePath}\n`);
141+
output.write = () => {};
142+
});
143+
144+
input.run([`.load ${filePath}`]);
145+
146+
replServer.close();
147+
}
148+
149+
// The case in which the user tries to load a directory instead of a file is appropriately handled
150+
{
151+
const { replServer, input, output } = prepareREPL();
152+
153+
const dirPath = tmpdir.path;
154+
155+
output.write = common.mustCall(function(data) {
156+
assert.strictEqual(data, `Failed to load: ${dirPath} is not a valid file\n`);
157+
output.write = () => {};
158+
});
159+
160+
input.run([`.load ${dirPath}`]);
161+
162+
replServer.close();
163+
}
164+
165+
// The case in which a file save fails is appropriately handled
166+
{
167+
const { replServer, input, output } = prepareREPL();
122168

123169
// NUL (\0) is disallowed in filenames in UNIX-like operating systems and
124170
// Windows so we can use that to test failed saves.
125-
const invalidFileName = tmpdir.resolve('\0\0\0\0\0');
126-
127-
// Should not break.
128-
putIn.write = common.mustCall(function(data) {
129-
// Make sure I get a failed to save message and not some other error.
130-
assert.strictEqual(data, `Failed to save: ${invalidFileName}\n`);
131-
// Reset to no-op.
132-
putIn.write = () => {};
171+
const invalidFilePath = tmpdir.resolve('\0\0\0\0\0');
172+
173+
output.write = common.mustCall(function(data) {
174+
assert.strictEqual(data, `Failed to save: ${invalidFilePath}\n`);
175+
output.write = () => {};
133176
});
134177

135-
// Save it to a file.
136-
putIn.run([`.save ${invalidFileName}`]);
178+
input.run([`.save ${invalidFilePath}`]);
137179

138-
{
139-
// Save .editor mode code.
140-
const cmds = [
141-
'function testSave() {',
142-
'return "saved";',
143-
'}',
144-
];
145-
const putIn = new ArrayStream();
146-
const replServer = repl.start({ terminal: true, stream: putIn });
180+
replServer.close();
181+
}
147182

148-
putIn.run(['.editor']);
149-
putIn.run(cmds);
150-
replServer.write('', { ctrl: true, name: 'd' });
183+
// Saving in editor mode works
184+
{
185+
const { replServer, input } = prepareREPL({ terminal: true });
186+
const tmpDirPath = tmpdir.random();
151187

152-
putIn.run([`.save ${saveFileName}`]);
153-
replServer.close();
154-
assert.strictEqual(fs.readFileSync(saveFileName, 'utf8'),
155-
`${cmds.join('\n')}\n`);
156-
}
188+
input.run(['.editor']);
157189

158-
// Check if the file is present when using save
190+
const commands = [
191+
'function testSave() {',
192+
'return "saved";',
193+
'}',
194+
];
159195

160-
// Clear the REPL.
161-
putIn.run(['.clear']);
196+
input.run(commands);
162197

163-
// Error message when using save without a file
164-
putIn.write = common.mustCall(function(data) {
165-
assert.strictEqual(data, 'The "file" argument must be specified\n');
166-
putIn.write = () => {};
167-
});
168-
putIn.run(['.save']);
198+
replServer.write('', { ctrl: true, name: 'd' });
169199

170-
// Check if the file is present when using load
200+
const filePath = path.resolve(tmpDirPath, 'test.save.js');
171201

172-
// Clear the REPL.
173-
putIn.run(['.clear']);
202+
input.run([`.save ${filePath}`]);
174203

175-
// Error message when using load without a file
176-
putIn.write = common.mustCall(function(data) {
177-
assert.strictEqual(data, 'The "file" argument must be specified\n');
178-
putIn.write = () => {};
179-
});
180-
putIn.run(['.load']);
181-
})().then(common.mustCall());
204+
assert.strictEqual(fs.readFileSync(filePath, 'utf8'),
205+
`${commands.join('\n')}\n`);
206+
207+
replServer.close();
208+
}

0 commit comments

Comments
 (0)