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 40 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.
}
150 changes: 150 additions & 0 deletions test/java/seedu/addressbook/commands/ViewCommandTest.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,150 @@
package seedu.addressbook.commands;

import static org.junit.Assert.assertEquals;

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 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> emptyPersonList = Collections.emptyList();
private List<ReadOnlyPerson> listWithAllTypicalPersons = Arrays.asList(td.getTypicalPersons());
private List<ReadOnlyPerson> listWithSomeTypicalPersons = Arrays.asList(td.amy, td.candy, td.dan);

@Test
public void execute_invalidIndex_returnsInvalidIndexMessage() {
// empty addressbook
assertViewErrorInvalidIndex(emptyAddressBook, emptyPersonList, 1);

// non-empty addressbook
assertViewErrorInvalidIndex(typicalAddressBook, listWithAllTypicalPersons, -1);
assertViewErrorInvalidIndex(typicalAddressBook, listWithAllTypicalPersons, 0);
assertViewErrorInvalidIndex(typicalAddressBook, listWithAllTypicalPersons,
listWithAllTypicalPersons.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.

Minor nit: indent for better readability. This way, ( and ) that demarcate the parameter list appear on far left and far right of the parameters, respectively.

 assertViewErrorInvalidIndex(typicalAddressBook, listWithAllTypicalPersons,
                                       listWithAllTypicalPersons.size() + 1);

This applies to a few other places as well.

}

@Test
public void execute_personNotInAddressBook_returnsPersonNotInAddressBookMessage() throws Exception {
// generate list with person not in addressbook, add to list
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> listWithExtraPerson
= new ArrayList<ReadOnlyPerson>(listWithAllTypicalPersons);
listWithExtraPerson.add(someone);

// empty addressbook
assertViewErrorPersonNotInAddressBook(emptyAddressBook, listWithExtraPerson, 1);

// non-empty addressbook
assertViewErrorPersonNotInAddressBook(typicalAddressBook, listWithExtraPerson,
listWithExtraPerson.size());
}

@Test
public void execute_validIndex_returnsPersonDetails() {
// person with no private information
assertViewSuccess(typicalAddressBook, listWithAllTypicalPersons, 1);

// person with some private information
assertViewSuccess(typicalAddressBook, listWithAllTypicalPersons, 2);

// person with all private information
assertViewSuccess(typicalAddressBook, listWithAllTypicalPersons, 4);

// addressbook has more people than the list
// 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, listWithSomeTypicalPersons, 1);
}

/**
* Asserts that the details of person at specific index cannot be retrieved due to
* invalid index.
*/
private void assertViewErrorInvalidIndex(AddressBook addressBook, List<ReadOnlyPerson> relevantPersons,
int targetVisibleIndex) {
assertViewError(addressBook, relevantPersons, targetVisibleIndex, Messages.MESSAGE_INVALID_PERSON_DISPLAYED_INDEX);
Copy link
Contributor

Choose a reason for hiding this comment

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

This line is much longer than 110 chars.

}

/**
* 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> relevantPersons,
int targetVisibleIndex) {
assertViewError(addressBook, relevantPersons, targetVisibleIndex, 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?

*
* @param targetVisibleIndex one-indexed position of the target person in the list
*/
private void assertViewSuccess(AddressBook addressBook, List<ReadOnlyPerson> relevantPersons,
int targetVisibleIndex) {
ReadOnlyPerson personToBeViewed = relevantPersons.get(targetVisibleIndex - 1); // -1 is because index is one-indexed
Copy link
Contributor

Choose a reason for hiding this comment

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

This line is much longer than 110 chars.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could put it like that:

        // -1 is because index is one-indexed
        ReadOnlyPerson personToBeViewed = relevantPersons.get(targetVisibleIndex - 1);

but imo that looks weird because usually a comment at the code block describes what the code below overall does, but in this case all we are doing is just commenting on the use of -1.

So maybe we need to add a description on what the code below does, even though it may be obvious. So something like:

        // Get person to be viewed (targetVisibleIndex - 1 because index is one-indexed)
        ReadOnlyPerson personToBeViewed = relevantPersons.get(targetVisibleIndex - 1);


String expectedMessage = String.format(ViewCommand.MESSAGE_VIEW_PERSON_DETAILS,
personToBeViewed.getAsTextHidePrivate());
assertViewBehavior(new ViewCommand(targetVisibleIndex), addressBook, relevantPersons, expectedMessage);

expectedMessage = String.format(ViewAllCommand.MESSAGE_VIEW_PERSON_DETAILS,
personToBeViewed.getAsTextShowAll());
assertViewBehavior(new ViewAllCommand(targetVisibleIndex), addressBook, relevantPersons, expectedMessage);
}

/**
* Asserts that the Viewcommand and ViewAllcommand reports the given error for the given input.
*/
private static void assertViewError(AddressBook addressBook, List<ReadOnlyPerson> relevantPersons,
int targetVisibleIndex, String expectedMessage) {
assertViewBehavior(new ViewCommand(targetVisibleIndex), addressBook, relevantPersons, expectedMessage);
assertViewBehavior(new ViewAllCommand(targetVisibleIndex), addressBook, relevantPersons, expectedMessage);
}

/**
* Executes the test command for the given addressbook data.
* Checks that ViewCommand and ViewAllCommand exhibits the correct command behavior, namely:
* 1. The feedback message of the CommandResult it returns matches expectedMessage.
* 2. The CommandResult it returns has no relevant persons.
* 3. The original addressbook data is not modified after executing ViewCommand and ViewAllCommand.
*/
private static void assertViewBehavior(Command viewCommand, AddressBook addressBook,
List<ReadOnlyPerson> relevantPersons, String expectedMessage) {
AddressBook expectedAddressBook = TestUtil.clone(addressBook);

viewCommand.setData(addressBook, relevantPersons);
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.

9 changes: 6 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
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