-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Add unit tests for ViewAllCommand and ViewCommand classes #111 #131
Changes from 37 commits
20e25f1
017b996
9d049e3
f97c6b1
e942a7e
42f0c7f
b414b98
7140ae2
5457976
ffd7b0f
8b3d3a3
2486c44
2d1260d
73147c3
915c4d9
acfb6a3
ea54ab7
3a068e6
2d4cf4f
3048a59
ea22dbf
5334baf
defefe1
4d30dde
d64b9f4
261fcab
fe3fa09
b01b1e6
39ad9a7
d6a6987
d2e9c4d
449af76
5fcc198
f565130
f6c2d90
f584b82
2a11bd5
beef8ab
c3af8e8
9c60233
b9db7e5
5b654c4
b4aba03
eb7f500
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
package seedu.addressbook.commands; | ||
|
||
public class ViewAllCommandTest { | ||
// ViewAllCommand is tested together with ViewCommand in ViewCommandTest. | ||
// This is because they function similarly but ViewCommand hides private information. | ||
// They are tested with same test data input. | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,139 @@ | ||
package seedu.addressbook.commands; | ||
|
||
import java.util.ArrayList; | ||
import java.util.Arrays; | ||
import java.util.Collections; | ||
import java.util.List; | ||
import java.util.Optional; | ||
|
||
import org.junit.Test; | ||
import static org.junit.Assert.assertEquals; | ||
|
||
import seedu.addressbook.common.Messages; | ||
import seedu.addressbook.data.AddressBook; | ||
import seedu.addressbook.data.person.Address; | ||
import seedu.addressbook.data.person.Email; | ||
import seedu.addressbook.data.person.Name; | ||
import seedu.addressbook.data.person.Person; | ||
import seedu.addressbook.data.person.Phone; | ||
import seedu.addressbook.data.person.ReadOnlyPerson; | ||
import seedu.addressbook.data.tag.UniqueTagList; | ||
import seedu.addressbook.util.TestUtil; | ||
import seedu.addressbook.util.TypicalPersons; | ||
|
||
public class ViewCommandTest { | ||
private TypicalPersons td = new TypicalPersons(); | ||
|
||
private AddressBook typicalAddressBook = td.getTypicalAddressBook(); | ||
private AddressBook emptyAddressBook = TestUtil.createAddressBook(); | ||
private List<ReadOnlyPerson> emptyDisplayList = Collections.emptyList(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe rename |
||
private List<ReadOnlyPerson> displayListWithAllTypicalPersons = Arrays.asList(td.getTypicalPersons()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe rename |
||
private List<ReadOnlyPerson> listWithSome = Arrays.asList(td.amy, td.candy, td.dan); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe rename |
||
|
||
@Test | ||
public void execute_invalidIndex_returnsInvalidIndexMessage() { | ||
// empty addressbook | ||
assertViewErrorInvalidIndex(emptyAddressBook, emptyDisplayList, 1); | ||
|
||
// non-empty addressbook | ||
assertViewErrorInvalidIndex(typicalAddressBook, displayListWithAllTypicalPersons, -1); | ||
assertViewErrorInvalidIndex(typicalAddressBook, displayListWithAllTypicalPersons, 0); | ||
assertViewErrorInvalidIndex(typicalAddressBook, displayListWithAllTypicalPersons, displayListWithAllTypicalPersons.size() + 1); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Overlong line way above 110 chars. |
||
} | ||
|
||
@Test | ||
public void execute_personNotInAddressBook_returnsPersonNotInAddressBookMessage() throws Exception { | ||
// generate person not in addressbook, add to displayList | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. More descriptive: |
||
ReadOnlyPerson someone = new Person(new Name("me"), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
new Phone("123", true), | ||
new Email("[email protected]", true), | ||
new Address("nus", false), | ||
new UniqueTagList(Collections.emptySet())); | ||
List <ReadOnlyPerson> displayListWithExtraPerson = new ArrayList<ReadOnlyPerson>(displayListWithAllTypicalPersons); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Overlong line way above 110 chars. |
||
displayListWithExtraPerson.add(someone); | ||
|
||
// empty addressbook | ||
assertViewErrorPersonNotInAddressBook(emptyAddressBook, displayListWithExtraPerson, 1); | ||
|
||
// non-empty addressbook | ||
assertViewErrorPersonNotInAddressBook(typicalAddressBook, displayListWithExtraPerson, displayListWithExtraPerson.size()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Overlong line way above 110 chars. |
||
} | ||
|
||
@Test | ||
public void execute_validIndex_returnsPersonDetails() { | ||
// person with no private information | ||
assertViewSuccess(typicalAddressBook, displayListWithAllTypicalPersons, 1); | ||
|
||
// person with some private information | ||
assertViewSuccess(typicalAddressBook, displayListWithAllTypicalPersons, 2); | ||
|
||
// person with all private information | ||
assertViewSuccess(typicalAddressBook, displayListWithAllTypicalPersons, 4); | ||
|
||
// addressbook has more people than displayList | ||
// This can happen when view from filtered list caused by some commands(eg. FindCommand) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This sentence is not very clear. Also, full sentences should end with full stop. |
||
assertViewSuccess(typicalAddressBook, listWithSome, 1); | ||
} | ||
|
||
/** | ||
* Asserts that the details of person at specific index cannot be retrieved due to | ||
* invalid index. | ||
*/ | ||
private void assertViewErrorInvalidIndex(AddressBook addressBook, List<ReadOnlyPerson> list, int index) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this really need its own method? Yes, I know the alternative is repeating If the reason is because There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If inline this method and put Message into the test case, does it really make sense or help improve readability? eg. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Doesn't your test name already say
OK then. |
||
assertViewError(addressBook, list, index, Messages.MESSAGE_INVALID_PERSON_DISPLAYED_INDEX); | ||
} | ||
|
||
/** | ||
* Asserts that the details of person at specific index cannot be retrieved due to | ||
* person not existing in the addressbook. | ||
*/ | ||
private void assertViewErrorPersonNotInAddressBook(AddressBook addressBook, List<ReadOnlyPerson> list, int index) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this really need its own method? Same reasoning as |
||
assertViewError(addressBook, list, index, Messages.MESSAGE_PERSON_NOT_IN_ADDRESSBOOK); | ||
} | ||
|
||
/** | ||
* Asserts that both a ViewCommand and a ViewAllCommand can retrieve from | ||
* the {@code addressBook} details of the person at the given {@code index} in the given {@code list}. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Change Change |
||
* | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Trailing whitespace. |
||
* @param targetVisibleIndex one-indexed position of the target person in the list | ||
*/ | ||
private void assertViewSuccess(AddressBook addressBook, List<ReadOnlyPerson> list, int targetVisibleIndex) { | ||
ReadOnlyPerson personToBeViewed = list.get(targetVisibleIndex - 1); // -1 is because index is one-indexed | ||
|
||
String expectedMessage = String.format(ViewCommand.MESSAGE_VIEW_PERSON_DETAILS, | ||
personToBeViewed.getAsTextHidePrivate()); | ||
assertViewBehavior(new ViewCommand(targetVisibleIndex), addressBook, list, expectedMessage); | ||
|
||
expectedMessage = String.format(ViewAllCommand.MESSAGE_VIEW_PERSON_DETAILS, | ||
personToBeViewed.getAsTextShowAll()); | ||
assertViewBehavior(new ViewAllCommand(targetVisibleIndex), addressBook, list, expectedMessage); | ||
} | ||
|
||
/** | ||
* Asserts that the Viewcommand and ViewAllcommand reports the given error for the given input | ||
*/ | ||
private static void assertViewError(AddressBook addressBook, List<ReadOnlyPerson> displayList, | ||
int targetVisibleIndex, String expectedMessage) { | ||
assertViewBehavior(new ViewCommand(targetVisibleIndex), addressBook, displayList, expectedMessage); | ||
assertViewBehavior(new ViewAllCommand(targetVisibleIndex), addressBook, displayList, expectedMessage); | ||
} | ||
|
||
/** | ||
* Asserts that the ViewCommand and ViewAllCommand show given feedback information | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It does not just check for feedback information. You may wish to enumerate the list of things it checks for, e.g.
|
||
*/ | ||
private static void assertViewBehavior(Command viewCommand, AddressBook addressBook, | ||
List<ReadOnlyPerson> list, String expectedMessage) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indent this further to the right. |
||
AddressBook expectedAddressBook = TestUtil.clone(addressBook); | ||
|
||
viewCommand.setData(addressBook, list); | ||
CommandResult result = viewCommand.execute(); | ||
|
||
// Feedback message is as expected and there are no relevant persons returned | ||
assertEquals(expectedMessage, result.feedbackToUser); | ||
assertEquals(Optional.empty(), result.getRelevantPersons()); | ||
|
||
// Address book was not modified | ||
assertEquals(expectedAddressBook.getAllPersons(), addressBook.getAllPersons()); | ||
} | ||
|
||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this class is almost the same as the other test class, we can text both commands in one of the class and make the other class empty with just a comment to explain why it is empty. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,13 +24,16 @@ | |
public class TestUtil { | ||
/** | ||
* Creates an address book containing the given persons. | ||
* @throws DuplicatePersonException if two or more given persons are the same | ||
*/ | ||
public static AddressBook createAddressBook(Person... persons) throws DuplicatePersonException { | ||
public static AddressBook createAddressBook(Person... persons) { | ||
AddressBook addressBook = new AddressBook(); | ||
|
||
for (Person person : persons) { | ||
addressBook.addPerson(person); | ||
try { | ||
addressBook.addPerson(person); | ||
} catch (DuplicatePersonException e) { | ||
throw new AssertionError(e); | ||
} | ||
} | ||
|
||
return addressBook; | ||
|
@@ -75,4 +78,5 @@ public static void assertTextFilesEqual(Path path1, Path path2) throws IOExcepti | |
List<String> list2 = Files.readAllLines(path2, Charset.defaultCharset()); | ||
assertEquals(String.join("\n", list1), String.join("\n", list2)); | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unnecessary change. |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,23 +7,26 @@ | |
import seedu.addressbook.data.person.Name; | ||
import seedu.addressbook.data.person.Person; | ||
import seedu.addressbook.data.person.Phone; | ||
import seedu.addressbook.data.tag.Tag; | ||
import seedu.addressbook.data.tag.UniqueTagList; | ||
|
||
/** | ||
* Class to generate typical test persons | ||
*/ | ||
public class TypicalPersons { | ||
|
||
public Person amy, bill, candy; | ||
public Person amy, bill, candy, dan; | ||
|
||
public TypicalPersons() { | ||
try { | ||
amy = new Person(new Name("Amy Buck"), new Phone("91119111", false), new Email("[email protected]", false), | ||
new Address("1 Clementi Road", false), new UniqueTagList()); | ||
bill = new Person(new Name("Bill Clint"), new Phone("92229222", false), new Email("[email protected]", false), | ||
new Address("2 Clementi Road", false), new UniqueTagList()); | ||
candy = new Person(new Name("Candy Destiny"), new Phone("93339333", false), | ||
new Email("[email protected]", false), new Address("3 Clementi Road", false), new UniqueTagList()); | ||
new Address("2 Clementi Road", true), new UniqueTagList()); | ||
candy = new Person(new Name("Candy Destiny"), new Phone("93339333", true), | ||
new Email("[email protected]", false), new Address("3 Clementi Road", true), new UniqueTagList()); | ||
dan = new Person(new Name("Dan Smith"), new Phone("1234556", true), new Email("[email protected]", true), | ||
new Address("NUS", true), new UniqueTagList(new Tag("Test"))); | ||
} catch (IllegalValueException e) { | ||
e.printStackTrace(); | ||
assert false : "not possible"; | ||
|
@@ -41,7 +44,7 @@ private void loadAddressBookWithSampleData(AddressBook ab) { | |
} | ||
|
||
public Person[] getTypicalPersons() { | ||
return new Person[]{amy, bill, candy}; | ||
return new Person[]{amy, bill, candy, dan}; | ||
} | ||
|
||
public AddressBook getTypicalAddressBook() { | ||
|
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.
Trailing whitespace