-
Notifications
You must be signed in to change notification settings - Fork 5
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
Show command review #265
Show command review #265
Conversation
Codecov Report
@@ Coverage Diff @@
## master #265 +/- ##
============================================
+ Coverage 60.60% 61.25% +0.65%
- Complexity 823 832 +9
============================================
Files 126 126
Lines 2894 2891 -3
Branches 384 383 -1
============================================
+ Hits 1754 1771 +17
+ Misses 976 957 -19
+ Partials 164 163 -1
Continue to review full report at Codecov.
|
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. Just some nits.
String[] keywords = args.split(" "); | ||
if (keywords.length != ShowCommand.COMMAND_TOKENS) { |
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.
Nice way to work around the magic numbers! @timjkong @underthehai
|
||
public class ShowCommandTest { | ||
|
||
private Model model; |
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.
Maybe consider using a stub?
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.
Or is it suppose to use original model?
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.
Try out integration testing? haha
@@ -18,12 +45,112 @@ public void execute_show_success() { | |||
} | |||
|
|||
@Test | |||
public void execute_goto_failure() { | |||
public void execute_showAccommodation_success() { | |||
try { |
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 wonder why you didn't use the method in CommandTestUtil (assertCommandSuccess(), etc.)?
Increased test coverage for show command to 90%