RedisServerHelper #33
Conversation
djpalumbo
force-pushed
the
master
branch
3 times, most recently
from
e435172
to
f1a271d
Compare
| @@ -28,8 +28,8 @@ describe('run-command', () => { | |||
| }); | |||
|
|
|||
| it('executes the redis command', async () => { | |||
| await runCommand('redis-command', ['args1', 'args2']); | |||
| await runCommand('redis-command', ['args1', 'args2'], { opt: true }); | |||
There was a problem hiding this comment.
What is opt? I can't find any documentation on it.
| @@ -2,7 +2,7 @@ import execa from 'execa'; | |||
| import redisDownload from 'redis-download'; | |||
| import { homedir } from 'os'; | |||
|
|
|||
| export default async function runCommand(command, args) { | |||
| export default async function runCommand(command, args, opts) { | |||
There was a problem hiding this comment.
| export default async function runCommand(command, args, opts) { | |
| export default async function runCommand(command, args, { stdio = 'inherit', detached = false } = {}) { |
This way you don't have to sprinkle { studio: 'inherit' } everywhere.
| @@ -15,5 +15,5 @@ export default async function runCommand(command, args) { | |||
| version, | |||
| }); | |||
|
|
|||
| return execa(`${directory}/src/${command}`, args, { stdio: 'inherit' }); | |||
| return { childProcess: execa(`${directory}/src/${command}`, args, opts) }; | |||
There was a problem hiding this comment.
| return { childProcess: execa(`${directory}/src/${command}`, args, opts) }; | |
| return execa(`${directory}/src/${command}`, args, { stdio, detached }); |
I'd prefer to control the options explicitly rather than allowing the caller to pass it anything.
Also, all of the CLI commands (redis-benchmark, redis-server, etc) expect this to have execas return value, aka a promise), so they'll all break. I don't currently have tests for those files, which is why it isn't currently failing, but they should be.
| } | ||
| } | ||
| return true; | ||
| } |
There was a problem hiding this comment.
I'd rather this less nested:
function containsValidPort(args) {
const flag = args.indexOf('--port');
if (flag < 0 || flag + 1 >= args.length) {
return true;
}
const port = args[flag + 1];
return typeof port === 'number' && port >= 0 && port <= 65535;
}| } | ||
|
|
||
| export class RedisServerHelper { | ||
| constructor(args, opts) { |
There was a problem hiding this comment.
| constructor(args, opts) { | |
| constructor(args = [], { downloadDir, version } = {}) { |
This lets us delete most of the args and opts validation.
| throw new Error('Invalid key in opts'); | ||
| } | ||
|
|
||
| if ('downloadDir' in opts) process.env.REDIS_DOWNLOADDIR = opts.downloadDir; |
There was a problem hiding this comment.
| if ('downloadDir' in opts) process.env.REDIS_DOWNLOADDIR = opts.downloadDir; | |
| if (downloadDir) process.env.REDIS_DOWNLOADDIR =downloadDir; |
| } | ||
|
|
||
| if ('downloadDir' in opts) process.env.REDIS_DOWNLOADDIR = opts.downloadDir; | ||
| if ('version' in opts) process.env.REDIS_VERSION = opts.version; |
There was a problem hiding this comment.
| if ('version' in opts) process.env.REDIS_VERSION = opts.version; | |
| if (version) process.env.REDIS_VERSION = version; |
| if (ready) return true; | ||
|
|
||
| return waitUntilReady(tries - 1, interval * backoff, backoff, args); | ||
| } |
There was a problem hiding this comment.
I would rather an iterative approach over a recursive one:
async function waitUntilReady(tries, interval, backoff, args) {
for (let i = 0; i < tries; i++) {
await wait(interval);
if (await checkIfReady(args)) {
return true;
}
}
throw new Error('Timed out');
}| @@ -0,0 +1,3 @@ | |||
| export default async function wait(interval) { | |||
| return new Promise((resolve) => setTimeout(resolve, interval)); | |||
There was a problem hiding this comment.
I'd rather this inline instead of as a separate file, since it's a one liner and it's only used in one place.
See issue #29.