T/43: Added tests #44
Conversation
… instead. Closes #3.
lib/commands/bootstrap.js
Outdated
| // Package is already cloned. | ||
| if ( fs.existsSync( destinationPath ) ) { | ||
| log.info( `Package "${ data.packageName }" is already cloned. Skipping.` ); | ||
| let promise = Promise.resolve( '' ); |
There was a problem hiding this comment.
Logger requires string as a param. I will correct it.
lib/commands/bootstrap.js
Outdated
| return resolve( { logs: log.all() } ); | ||
| } | ||
| // Package is not cloned. | ||
| if ( !fs.existsSync( destinationPath ) ) { |
There was a problem hiding this comment.
Switch the order of ifs. It doesn't make sense to check if( !cond ) if you have else.
|
Please list which commit fixed which issue. Otherwise, this PR will need to be split. I must be able to review it and now it's a mix of new things and bug fixes. |
| } ); | ||
| } ); | ||
|
|
||
| it( 'installs dependencies of cloned package', () => { |
There was a problem hiding this comment.
Sorry. The other one is devDeps.
|
Try to use the list - https://github.com/cksource/mgit2/pull/44/commits. Maybe it will be helpful. |
tests/commands/update.js
Outdated
| const exec = stubs.execCommand.execute; | ||
|
|
||
| exec.returns( Promise.resolve( { | ||
| logs: getCommandLogs( ' M first-file.js\ ?? second-file.js' ) |
lib/commands/bootstrap.js
Outdated
|
|
||
| return resolve( { logs: log.all() } ); | ||
| // Package is not cloned. |
lib/commands/bootstrap.js
Outdated
| if ( fs.existsSync( destinationPath ) ) { | ||
| log.info( `Package "${ data.packageName }" is already cloned.` ); | ||
| } else { | ||
| const command = [ |
There was a problem hiding this comment.
Since this condition sets the promise to exec( cmd ), the previous condition could set it to Promise.resolve(). This would be clearer.
|
|
||
| exec( command ) | ||
| promise |
There was a problem hiding this comment.
Missing return statement (?)
There was a problem hiding this comment.
There was a problem hiding this comment.
I see now where resolve and reject parts are. But it looks a little bit strange for me to return a promise which has another promise inside and which is even not returned ;) Maybe it could be written step by step with .then() method and get 27-42 lines before the return statement?
There was a problem hiding this comment.
A promise inside another promise can not be returned because you need to call resolve or reject function.
OTOH maybe the "wrapper" Promise is not necessary. Who knows? :D
Fix: Fixed various minor issues with the commands. Introduced missing tests. Closes #31. Closes #41. Closes #3. Closes #43.