-
Notifications
You must be signed in to change notification settings - Fork 6
Add tests for the CLI command #581
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
src/cli.test.ts
Outdated
| const formattedArgs = args | ||
| .map(([c, a]) => { | ||
| // if args is array then quote each elem and separate them with space | ||
| if (Array.isArray(a)) return `${c} ${a.map((element) => quote(element)).join(' ')}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CommandArg type definition above says a's type can be string | number | boolean. I don't see why we're handling the case where a is an array here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right it is unnecessary. It was checking string[] at first which is not necessary after iterating the code.
src/cli.test.ts
Outdated
| ['--chain-id', '5000'], | ||
| ['--dapi-name', 'UNSUPPORTED/USD'] | ||
| ); | ||
| }).toThrow(/⚠️ Attempted to read the feed and failed/); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This probably shouldn't be a partial match (/ are extra)
src/cli.test.ts
Outdated
|
|
||
| describe('cli tests', () => { | ||
| const execCommand = (command: string, ...args: CommandArg[]) => { | ||
| const quote = (val: string) => `"${val}"`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should also get rid of quote() considering that we're not recommending the user to enclose argument values in quotation marks anywhere. That was an airnode-admin thing where seed phrases needed to be enclosed because they have spaces in them.
bbenligiray
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Related to #363
There are some limitations testing the CLI command
The child process spawned by execSync:
to test with mocks the code should be refactored so that core logic is a function and can be exported.