Unit tests in AddressBookTest needs improvement #182 #298
Conversation
| @@ -75,10 +75,13 @@ public void setUp() throws Exception { | |||
|
|
|||
| @Rule | |||
| public ExpectedException thrown = ExpectedException.none(); | |||
|
|
|||
There was a problem hiding this comment.
Avoid auto-formatting unrelated parts of the code. Doing so can result in unrelated changes being included in the commit. You can use sourcetree to look through changes before committing, and discard any unrelated changes. If you wish to commit formatting changes to unrelated parts of the code, do so in a separate commit.
|
|
||
| @Test | ||
| public void addPerson_emptyAddressBook() throws Exception { | ||
| emptyAddressBook.addPerson(bobChaplin); | ||
| emptyAddressBook.addPerson(bobChaplin); |
There was a problem hiding this comment.
Avoid auto-formatting unrelated parts of the code. Doing so can result in unrelated changes being included in the commit. You can use sourcetree to look through changes before committing, and discard any unrelated changes. If you wish to commit formatting changes to unrelated parts of the code, do so in a separate commit.
|
|
||
| @Test | ||
| public void addPerson_emptyAddressBook() throws Exception { | ||
| emptyAddressBook.addPerson(bobChaplin); | ||
| emptyAddressBook.addPerson(bobChaplin); | ||
| Person bobChaplin_confirmPerson = emptyAddressBook.getAllPersons().iterator().next(); |
There was a problem hiding this comment.
This naming style (using _ in variables), is not consistent with the rest of the code base.
| emptyAddressBook.addPerson(bobChaplin); | ||
| emptyAddressBook.addPerson(bobChaplin); | ||
| Person bobChaplin_confirmPerson = emptyAddressBook.getAllPersons().iterator().next(); | ||
| assertTrue(bobChaplin.equals(bobChaplin_confirmPerson)); |
There was a problem hiding this comment.
Why not make use of existing methods in the AddressBook class?
(Specifically AddressBook#containsPerson(ReadOnlyPerson):boolean)
|
@PierceAndy Can you please review the PR for issue #182