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.

Sign up for GitHub

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
+175 −8
Merged

Add unit tests for ViewAllCommand and ViewCommand classes #111 #131

Show file tree
Hide file tree
Changes from 2 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
53 changes: 14 additions & 39 deletions test/java/seedu/addressbook/commands/ViewAllCommandTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
package seedu.addressbook.commands;

import static org.junit.Assert.assertEquals;

import java.util.Collections;
import java.util.List;

Expand All @@ -16,9 +14,11 @@
import seedu.addressbook.data.person.Person;
import seedu.addressbook.data.person.Phone;
import seedu.addressbook.data.person.ReadOnlyPerson;
import seedu.addressbook.data.tag.Tag;
import seedu.addressbook.data.tag.UniqueTagList;
import seedu.addressbook.util.TestUtil;
import seedu.addressbook.util.TypicalPersons;

import static seedu.addressbook.util.TestUtil.assertCommandResult;

public class ViewAllCommandTest {
private AddressBook addressBook;
Expand All @@ -29,23 +29,13 @@ public class ViewAllCommandTest {

@Before
public void setUp() throws Exception {
Person johnDoe = new Person(new Name("John Doe"),
new Phone("98765432", false),
new Email("[email protected]", false),
new Address("John street, block 123, #01-01", false),
new UniqueTagList(Collections.emptySet()));
Person betsyCrowe = new Person(new Name("Betsy Crowe"),
new Phone("1234567", true),
new Email("[email protected]", false),
new Address("Newgate Prison", true),
new UniqueTagList(new Tag("friend"), new Tag("criminal")));

emptyAddressBook = TestUtil.createAddressBook();
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be initialized at the same place it's declared. Junit creates a new instance for each test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@damithc This TestUtil.createAddressBook() is throwing an excpetion in TestUtil Class so cannot put outside right?

Copy link
Contributor

Choose a reason for hiding this comment

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

In that case can keep.

addressBook = TestUtil.createAddressBook(johnDoe, betsyCrowe);
addressBook = TestUtil.createAddressBook();
TypicalPersons.loadAddressBookWithSampleData(addressBook);

emptyDisplayList = TestUtil.createList();
listWithAll = TestUtil.createList(johnDoe, betsyCrowe);
listWithSome = TestUtil.createList(betsyCrowe);
listWithAll = TypicalPersons.getListWithAllPersons();
listWithSome = TypicalPersons.getListWithSomePersons();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Extract into a TypicalPersons utility class?


@Test
Expand All @@ -56,7 +46,7 @@ public void viewAllCommand_invalidIndex_returnsInvalidIndexMessage() {
// non-empty addressbook
assertViewAllErrorInvalidIndex(addressBook, listWithAll, -1);
assertViewAllErrorInvalidIndex(addressBook, listWithAll, 0);
assertViewAllErrorInvalidIndex(addressBook, listWithAll, 3);
assertViewAllErrorInvalidIndex(addressBook, listWithAll, listWithAll.size() + 1);
}

@Test
Expand All @@ -73,7 +63,7 @@ public void viewAllCommand_personNotInAddressBook_returnsPersonNotInAddressBookM
assertViewAllErrorPersonNotInAddressBook(emptyAddressBook, listWithAll, 1);

// non-empty addressbook
assertViewAllErrorPersonNotInAddressBook(addressBook, listWithAll, 3);
assertViewAllErrorPersonNotInAddressBook(addressBook, listWithAll, listWithAll.size());
}

@Test
Expand Down Expand Up @@ -104,10 +94,11 @@ private Command generateViewAllCommand(AddressBook addressBook, List<ReadOnlyPer
* and displayed.
*/
private void assertViewAllSuccess(AddressBook addressBook, List<ReadOnlyPerson> list, int index) {
String expectedMessage = String.format(ViewAllCommand.MESSAGE_VIEW_PERSON_DETAILS, list.get(index - 1).getAsTextShowAll());
String expectedMessage = String.format(ViewAllCommand.MESSAGE_VIEW_PERSON_DETAILS,
list.get(index - 1).getAsTextShowAll());

Command command = generateViewAllCommand(addressBook, list, index);
assertCommandResult(command, expectedMessage, addressBook);
assertCommandResult(command, expectedMessage, addressBook, TestUtil.clone(addressBook));
}

/**
Expand All @@ -118,7 +109,7 @@ private void assertViewAllErrorInvalidIndex(AddressBook addressBook, List<ReadOn
String expectedMessage = Messages.MESSAGE_INVALID_PERSON_DISPLAYED_INDEX;

Command command = generateViewAllCommand(addressBook, list, index);
assertCommandResult(command, expectedMessage, addressBook);
assertCommandResult(command, expectedMessage, addressBook, TestUtil.clone(addressBook));
}

/**
Expand All @@ -129,22 +120,6 @@ private void assertViewAllErrorPersonNotInAddressBook(AddressBook addressBook, L
String expectedMessage = Messages.MESSAGE_PERSON_NOT_IN_ADDRESSBOOK;

Command command = generateViewAllCommand(addressBook, list, index);
assertCommandResult(command, expectedMessage, addressBook);
}

/**
* Executes the command, and asserts the result message is as expected.
*/
private void assertCommandResult(Command command, String expectedMessage,
AddressBook addressBook) {
AddressBook backUpAddressBook = TestUtil.clone(addressBook);
CommandResult result = command.execute();

// asserts the result message is correct as expected
assertEquals(expectedMessage, result.feedbackToUser);

// asserts the ViewAll command does not mutate the data
// TODO: overwrite equals method in AddressBook and replace with equals method below
assertEquals(addressBook.getAllPersons(), backUpAddressBook.getAllPersons());
assertCommandResult(command, expectedMessage, addressBook, TestUtil.clone(addressBook));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can the common part of the above two methods can be extracted as a method?

}
53 changes: 14 additions & 39 deletions test/java/seedu/addressbook/commands/ViewCommandTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
package seedu.addressbook.commands;

import static org.junit.Assert.assertEquals;

import java.util.Collections;
import java.util.List;

Expand All @@ -16,9 +14,11 @@
import seedu.addressbook.data.person.Person;
import seedu.addressbook.data.person.Phone;
import seedu.addressbook.data.person.ReadOnlyPerson;
import seedu.addressbook.data.tag.Tag;
import seedu.addressbook.data.tag.UniqueTagList;
import seedu.addressbook.util.TestUtil;
import seedu.addressbook.util.TypicalPersons;

import static seedu.addressbook.util.TestUtil.assertCommandResult;

public class ViewCommandTest {
private AddressBook addressBook;
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmph, we can't use TestUtil.createAddressBook() here because it throws a checked exception. However, if we step back a little, we see that TestUtil.createAddressBook() should not be passing on the DuplicatePersonException because it's only used for test data. (i.e. the same reason why TypicalPersons just asserts false if it encounters an IllegalValueException.)

So, I think you should just add this in to your PR:

diff --git a/test/java/seedu/addressbook/util/TestUtil.java b/test/java/seedu/addressbook/util/TestUtil.java
index 8f4eaf8..a1f0836 100644
--- a/test/java/seedu/addressbook/util/TestUtil.java
+++ b/test/java/seedu/addressbook/util/TestUtil.java
@@ -26,13 +26,16 @@ import seedu.addressbook.data.tag.UniqueTagList;
 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;

No point letting the quality of your PR suffer just because the code base just happens to be like this already.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, maybe name this typicalAddressBook to signal that it is filled with all the TypicalPersons.

Expand All @@ -29,23 +29,13 @@ public class ViewCommandTest {

@Before
public void setUp() throws Exception {
Person johnDoe = new Person(new Name("John Doe"),
new Phone("98765432", false),
new Email("[email protected]", false),
new Address("John street, block 123, #01-01", false),
new UniqueTagList(Collections.emptySet()));
Person betsyCrowe = new Person(new Name("Betsy Crowe"),
new Phone("1234567", true),
new Email("[email protected]", false),
new Address("Newgate Prison", true),
new UniqueTagList(new Tag("friend"), new Tag("criminal")));

emptyAddressBook = TestUtil.createAddressBook();
addressBook = TestUtil.createAddressBook(johnDoe, betsyCrowe);
addressBook = TestUtil.createAddressBook();
TypicalPersons.loadAddressBookWithSampleData(addressBook);

emptyDisplayList = TestUtil.createList();
listWithAll = TestUtil.createList(johnDoe, betsyCrowe);
listWithSome = TestUtil.createList(betsyCrowe);
listWithAll = TypicalPersons.getListWithAllPersons();
listWithSome = TypicalPersons.getListWithSomePersons();
}

@Test
Expand All @@ -56,7 +46,7 @@ public void viewCommand_invalidIndex_returnsInvalidIndexMessage() {
// non-empty addressbook
assertViewErrorInvalidIndex(addressBook, listWithAll, -1);
assertViewErrorInvalidIndex(addressBook, listWithAll, 0);
assertViewErrorInvalidIndex(addressBook, listWithAll, 3);
assertViewErrorInvalidIndex(addressBook, listWithAll, listWithAll.size() + 1);
}

@Test
Expand All @@ -73,7 +63,7 @@ public void viewCommand_personNotInAddressBook_returnsPersonNotInAddressBookMess
assertViewErrorPersonNotInAddressBook(emptyAddressBook, listWithAll, 1);

// non-empty addressbook
assertViewErrorPersonNotInAddressBook(addressBook, listWithAll, 3);
assertViewErrorPersonNotInAddressBook(addressBook, listWithAll, listWithAll.size());
}

@Test
Expand Down Expand Up @@ -104,10 +94,11 @@ private Command generateViewCommand(AddressBook addressBook, List<ReadOnlyPerson
* and displayed.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Also add
@param index one-indexed position of the target person in the list

*/
private void assertViewSuccess(AddressBook addressBook, List<ReadOnlyPerson> list, int index) {
String expectedMessage = String.format(ViewCommand.MESSAGE_VIEW_PERSON_DETAILS, list.get(index - 1).getAsTextHidePrivate());
String expectedMessage = String.format(ViewCommand.MESSAGE_VIEW_PERSON_DETAILS,
list.get(index - 1).getAsTextHidePrivate());

Command command = generateViewCommand(addressBook, list, index);
assertCommandResult(command, expectedMessage, addressBook);
assertCommandResult(command, expectedMessage, addressBook, TestUtil.clone(addressBook));
}

/**
Expand All @@ -118,7 +109,7 @@ private void assertViewErrorInvalidIndex(AddressBook addressBook, List<ReadOnlyP
String expectedMessage = Messages.MESSAGE_INVALID_PERSON_DISPLAYED_INDEX;

Command command = generateViewCommand(addressBook, list, index);
assertCommandResult(command, expectedMessage, addressBook);
assertCommandResult(command, expectedMessage, addressBook, TestUtil.clone(addressBook));
}

/**
Expand All @@ -129,22 +120,6 @@ private void assertViewErrorPersonNotInAddressBook(AddressBook addressBook, List
String expectedMessage = Messages.MESSAGE_PERSON_NOT_IN_ADDRESSBOOK;

Command command = generateViewCommand(addressBook, list, index);
assertCommandResult(command, expectedMessage, addressBook);
}

/**
* Executes the command, and asserts the result message is as expected.
*/
private void assertCommandResult(Command command, String expectedMessage,
AddressBook addressBook) {
AddressBook backUpAddressBook = TestUtil.clone(addressBook);
CommandResult result = command.execute();

// asserts the result message is correct as expected
assertEquals(expectedMessage, result.feedbackToUser);

// asserts the view command does not mutate the data
// TODO: overwrite equals method in AddressBook and replace with equals method below
assertEquals(addressBook.getAllPersons(), backUpAddressBook.getAllPersons());
assertCommandResult(command, expectedMessage, addressBook, TestUtil.clone(addressBook));
}
}
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.

17 changes: 17 additions & 0 deletions test/java/seedu/addressbook/util/TestUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
package seedu.addressbook.util;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.fail;

import java.util.ArrayList;
import java.util.List;

import seedu.addressbook.commands.Command;
import seedu.addressbook.commands.CommandResult;
import seedu.addressbook.data.AddressBook;
import seedu.addressbook.data.exception.IllegalValueException;
import seedu.addressbook.data.person.Address;
Expand Down Expand Up @@ -60,4 +63,18 @@ public static Person generateTestPerson() {
return null;
}
}

/**
* Executes the command, and asserts the result message is as expected.
*/
public static void assertCommandResult(Command command, String expectedMessage, AddressBook addressBook,
AddressBook expectedAddressBook) {
Copy link
Contributor

Choose a reason for hiding this comment

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

More readable indentation:

public static void assertCommandResult(Command command, String expectedMessage, AddressBook addressBook, 
                                                                         AddressBook expectedAddressBook) {

CommandResult result = command.execute();

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.

// asserts the result message is correct as expected
assertEquals(expectedMessage, result.feedbackToUser);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we be comparing the CommandResult instead, rather than just one field of it?

For example, we may wish to check that CommandResult.getRelevantPersons() returns Optional.empty() or a list that we expect.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we be comparing the CommandResult instead, rather than just one field of it?

Yes, that's actually better. I don't mind leaving it as a TODO though.


// TODO: overwrite equals method in AddressBook and replace with equals method below
assertEquals(addressBook.getAllPersons(), expectedAddressBook.getAllPersons());
}
}
82 changes: 82 additions & 0 deletions test/java/seedu/addressbook/util/TypicalPersons.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
package seedu.addressbook.util;

import java.util.ArrayList;
import java.util.Collections;
import java.util.List;

import seedu.addressbook.data.AddressBook;
import seedu.addressbook.data.exception.IllegalValueException;
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.person.UniquePersonList;
import seedu.addressbook.data.tag.Tag;
import seedu.addressbook.data.tag.UniqueTagList;

/**
*
*/
public class TypicalPersons {

public static Person johnDoe, selbyWhite, betsyCrowe, steveSam;

private static void initialisePersons() {
try {
johnDoe = new Person(new Name("John Doe"),
new Phone("98765432", false),
new Email("[email protected]", false),
new Address("John street, block 123, #01-01", false),
new UniqueTagList(Collections.emptySet()));
selbyWhite = new Person(new Name("Selby White"),
new Phone("111344", false),
new Email("[email protected]", true),
new Address("UK", false),
new UniqueTagList(Collections.emptySet()));
betsyCrowe = new Person(new Name("Betsy Crowe"),
new Phone("1234567", true),
new Email("[email protected]", false),
new Address("Newgate Prison", true),
new UniqueTagList(new Tag("friend"), new Tag("criminal")));
steveSam = new Person(new Name("Steve Sam"),
new Phone("1231", true),
new Email("[email protected]", true),
new Address("Silicon Vally", true),
new UniqueTagList(new Tag("boss")));
} catch (IllegalValueException e) {
e.printStackTrace();
assert false : "not possible";
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

There is another similar class in #120 that might be merged before this. May be you can follow the same? Note how the person names follow a pattern. e.g. First person has initials A B, second one B C, and so on. Even the address follows a pattern and not random. Such patterns help us detect when things go out of place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@damithc So I wait for the other pr to merge first then use and edit his TypicalPersons for the test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we both have same class created

Copy link
Contributor

Choose a reason for hiding this comment

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

That class is mostly stable now, so you can copy-paste-modify that code into your and continue with your PR. If that PR is merged first, you'll have to update your branch and de-conflict your class a bit at that point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok


public static void loadAddressBookWithSampleData(AddressBook ab) {
try {
initialisePersons();
ab.addPerson(johnDoe);
ab.addPerson(selbyWhite);
ab.addPerson(betsyCrowe);
ab.addPerson(steveSam);
} catch (UniquePersonList.DuplicatePersonException e) {
assert false : "not possible";
}
}

public static List<ReadOnlyPerson> getListWithAllPersons() {
List<ReadOnlyPerson> list = new ArrayList<ReadOnlyPerson>();
list.add(johnDoe);
list.add(selbyWhite);
list.add(betsyCrowe);
list.add(steveSam);
return list;
}

public static List<ReadOnlyPerson> getListWithSomePersons() {
List<ReadOnlyPerson> list = new ArrayList<ReadOnlyPerson>();
list.add(johnDoe);
list.add(selbyWhite);
list.add(steveSam);
return list;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This method is very specific to your test class and therefore not a good abstraction. Try something like this instead:
public List<ReadOnlyPerson> getList(ReadOnlyPerson... personsToInclude);

Can be used like this:
getList(td.amy, td.candy);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

noted

}