Migrate block command - Closes #563 #570
Conversation
shuse2
force-pushed
the
556_migrate_config_command
branch
2 times, most recently
from
4669cd4 to
865d40c
Compare
shuse2
force-pushed
the
563_migrate_block
branch
from
bb6924d to
07a131f
Compare
src/commands/block/get.js
Outdated
| blockId, | ||
| }, | ||
| placeholder: { | ||
| blockId, |
There was a problem hiding this comment.
This should be id: blockId to conform to the API payload.
src/commands/block/get.js
Outdated
| name: 'blockIds', | ||
| required: true, | ||
| description: | ||
| 'Comma separated block id(s) which you want to get the information of.', |
There was a problem hiding this comment.
Comma-separated block ID(s) to get information about.
test/commands/block/get.test.js
Outdated
| }; | ||
| const printMethodStub = sandbox.stub(); | ||
| const apiClientStub = sandbox.stub(); | ||
| const setupStub = test |
There was a problem hiding this comment.
setupTest. Also we made this a function in the account tests.
test/commands/block/get.test.js
Outdated
| .stub(config, 'getConfig', sandbox.stub().returns({ api: apiConfig })) | ||
| .stub(api, 'default', sandbox.stub().returns(apiClientStub)); | ||
|
|
||
| setupStub |
There was a problem hiding this comment.
Can we match the structure in the account test?
test/commands/block/get.test.js
Outdated
| .catch(error => expect(error.message).to.contain('Missing 1 required arg')) | ||
| .it('should throw an error when arg is not provided'); | ||
|
|
||
| describe('block:get block', () => { |
test/commands/block/get.test.js
Outdated
| .stdout() | ||
| .stub(query, 'default', sandbox.stub().resolves(queryResult)) | ||
| .command(['block:get', block]) | ||
| .it('should get an block info and display as an object', () => { |
There was a problem hiding this comment.
should get block info and display as an object
test/commands/block/get.test.js
Outdated
| }); | ||
| }); | ||
|
|
||
| describe('block:get blocks', () => { |
test/commands/block/get.test.js
Outdated
| }); | ||
|
|
||
| describe('block:get blocks', () => { | ||
| const blocks = ['3520445367460290306L', '2802325248134221536L']; |
test/commands/block/get.test.js
Outdated
| const queryResult = [ | ||
| { | ||
| blockId: blocks[0], | ||
| name: 'i am owner', |
There was a problem hiding this comment.
This example didn't make much sense in the account case but it makes even less sense here.
| }, | ||
| ]); | ||
| return expect(printMethodStub).to.be.calledWithExactly(queryResult); | ||
| }); |
There was a problem hiding this comment.
Wouldn't it be better to put these expectations in separate .it calls?
There was a problem hiding this comment.
.it is the end of chain, so it cannot have multiple it in one test ='(
There was a problem hiding this comment.
There was a problem hiding this comment.
Looks good, but I get some weird behaviour running this command:
./bin/run block:get 17108498772892203620,
For account:get this kind of input errors with a 400 (validation errors) because the second address is an empty string. But with block:get I get the result for the first ID twice.
src/commands/block/get.js
Outdated
| @@ -41,7 +41,7 @@ GetCommand.args = [ | |||
| name: 'blockIds', | |||
| required: true, | |||
| description: 'Comma-separated block ID(s) to get information about.', | |||
| parse: input => input.split(','), | |||
| parse: input => input.split(',').filter(val => val), | |||
There was a problem hiding this comment.
I like .filter(Boolean) for this kind of check.
There was a problem hiding this comment.
something like
.filter(isNonEmptyString)?
test/commands/block/get.test.js
Outdated
| @@ -115,5 +116,33 @@ describe('block:get', () => { | |||
| ]); | |||
| return expect(printMethodStub).to.be.calledWithExactly(queryResult); | |||
| }); | |||
|
|
|||
| setupTest() | |||
There was a problem hiding this comment.
Should we add a similar filter + test to accounts?
There was a problem hiding this comment.
yes! i will make it on another issue/PR
Description
Migration from
npm t./bin/run block:getReview checklist