Skip to content
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

Merged
merged 44 commits into from
Jan 17, 2017
Merged
Show file tree
Hide file tree
Changes from 37 commits
Commits
Show all changes
44 commits
Select commit Hold shift + click to select a range
20e25f1
add unit test for view command
zzzzwen Dec 29, 2016
017b996
Add header comment
zzzzwen Dec 30, 2016
9d049e3
Add unit test for ViewAll command
zzzzwen Dec 30, 2016
f97c6b1
javadoc change
zzzzwen Dec 30, 2016
e942a7e
Move assertCommandResult method to TestUtil
zzzzwen Dec 30, 2016
42f0c7f
Extract TypicalPersons
zzzzwen Dec 30, 2016
b414b98
Conflict
zzzzwen Dec 30, 2016
7140ae2
Merge branch 'master' into 111-unit-test-view-viewall
zzzzwen Dec 30, 2016
5457976
Conflict resolving
zzzzwen Dec 30, 2016
ffd7b0f
Rework TypicalPersons
zzzzwen Dec 30, 2016
8b3d3a3
code quality
zzzzwen Dec 30, 2016
2486c44
Merge branch 'master' of https://github.com/se-edu/addressbook-level2
zzzzwen Jan 3, 2017
2d1260d
Update TypicalPersons
zzzzwen Jan 3, 2017
73147c3
Code style issues
zzzzwen Jan 3, 2017
915c4d9
Code quality
zzzzwen Jan 3, 2017
acfb6a3
Code Style and combine two test cases
zzzzwen Jan 4, 2017
ea54ab7
Grammar
zzzzwen Jan 4, 2017
3a068e6
Remove use of createList
zzzzwen Jan 4, 2017
2d4cf4f
Code Style
zzzzwen Jan 4, 2017
3048a59
rework getList
zzzzwen Jan 4, 2017
ea22dbf
extract method
zzzzwen Jan 4, 2017
5334baf
reorder message
zzzzwen Jan 4, 2017
defefe1
Code style
zzzzwen Jan 4, 2017
4d30dde
Code style
zzzzwen Jan 4, 2017
d64b9f4
Add one test case
zzzzwen Jan 4, 2017
261fcab
comment change
zzzzwen Jan 4, 2017
fe3fa09
extract local variable
zzzzwen Jan 4, 2017
b01b1e6
Remove list methods in TypicalPersons
zzzzwen Jan 4, 2017
39ad9a7
Remove unused import
zzzzwen Jan 4, 2017
d6a6987
javadoc and code style changes
zzzzwen Jan 4, 2017
d2e9c4d
code style change
zzzzwen Jan 5, 2017
449af76
code style changes
zzzzwen Jan 5, 2017
5fcc198
code style change
zzzzwen Jan 5, 2017
f565130
remove trailing whitespaces
zzzzwen Jan 5, 2017
f6c2d90
Code style
zzzzwen Jan 6, 2017
f584b82
code style
zzzzwen Jan 6, 2017
2a11bd5
Combine to generic methods
zzzzwen Jan 8, 2017
beef8ab
Code style changes
zzzzwen Jan 9, 2017
c3af8e8
code style changes
zzzzwen Jan 9, 2017
9c60233
Code style changes
zzzzwen Jan 9, 2017
b9db7e5
code style changes
zzzzwen Jan 13, 2017
5b654c4
Code style changes
zzzzwen Jan 15, 2017
b4aba03
code style changes
zzzzwen Jan 15, 2017
eb7f500
Code style changes
zzzzwen Jan 15, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions test/java/seedu/addressbook/commands/ViewAllCommandTest.java
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.
}
139 changes: 139 additions & 0 deletions test/java/seedu/addressbook/commands/ViewCommandTest.java
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();

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trailing whitespace

private AddressBook typicalAddressBook = td.getTypicalAddressBook();
private AddressBook emptyAddressBook = TestUtil.createAddressBook();
private List<ReadOnlyPerson> emptyDisplayList = Collections.emptyList();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe rename emptyDisplayList to emptyPersonList.

private List<ReadOnlyPerson> displayListWithAllTypicalPersons = Arrays.asList(td.getTypicalPersons());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe rename displayListWithAllTypicalPersons to allTypicalPersonsList or listWithAllTypicalPersons.

private List<ReadOnlyPerson> listWithSome = Arrays.asList(td.amy, td.candy, td.dan);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe rename listWithSome to listWithSomeTypicalPersons.


@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);
Copy link
Contributor

Choose a reason for hiding this comment

The 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More descriptive: generate list with person not in addressbook.

ReadOnlyPerson someone = new Person(new Name("me"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

someone --> stranger?

new Phone("123", true),
new Email("[email protected]", true),
new Address("nus", false),
new UniqueTagList(Collections.emptySet()));
List <ReadOnlyPerson> displayListWithExtraPerson = new ArrayList<ReadOnlyPerson>(displayListWithAllTypicalPersons);
Copy link
Contributor

Choose a reason for hiding this comment

The 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());
Copy link
Contributor

Choose a reason for hiding this comment

The 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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sentence is not very clear.
This can happen when a command causes the list to show only a sub-set of persons (e.g. FindCommand).

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) {
Copy link
Contributor

Choose a reason for hiding this comment

The 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 Messages.MESSAGE_INVALID_PERSON_DISPLAYED_INDEX in viewCommand_invalidIndex_returnsInvalidIndexMessage(), but then again with this method you still need to repeat assertViewErrorInvalidIndex anyway, which probably means this method doesn't add much value.

If the reason is because Messages.MESSAGE_INVALID_PERSON_DISPLAYED_INDEX is too long, then you can assign in to a variable with a shorter name.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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. assertViewErrorPersonNotInAddressBook(emptyAddressBook, listWithAll, 1) -> assertCommandError(emptyAddressBook, listWithAll, 1, Message.PERSON_NOT_IN_ADDRESSBOOK)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assertCommandError does not imply what expected error is and can we assume that reader can infer from the parameter about the expected error? In this case, assertViewErrorInvalidIndex is clearer for me

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zzzzwen

assertCommandError does not imply what expected error is and can we assume that reader can infer from the parameter about the expected error

Doesn't your test name already say _returnsInvalidIndexMessage?

In this case, assertViewErrorInvalidIndex is clearer for me

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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this really need its own method? Same reasoning as assertViewErrorInvalidIndex.

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}.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change {@code index} to {@code targetVisibleIndex}?

Change {@code list} to {@code relevantPersons} list?

*
Copy link
Contributor

Choose a reason for hiding this comment

The 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
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Checks that viewCommand exhibits the correct view command behavior, namely:

1. The feedback message of the CommandResult it returns matches expectedMessage
2. The CommandResult it returns has no relevant persons.
3. ...

*/
private static void assertViewBehavior(Command viewCommand, AddressBook addressBook,
List<ReadOnlyPerson> list, String expectedMessage) {
Copy link
Contributor

Choose a reason for hiding this comment

The 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());
}

}
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

10 changes: 7 additions & 3 deletions test/java/seedu/addressbook/util/TestUtil.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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));
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary change.

}
13 changes: 8 additions & 5 deletions test/java/seedu/addressbook/util/TypicalPersons.java
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand All @@ -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() {
Expand Down