Add unit tests for FindCommand class #110 #120

awhdesmond · 2016-12-09T17:20:44Z

Fixes #110
Ready for review.

This change is

damithc

Some prelim comments added. Can modify based on those comments first. Also watchout for other similar PRs going on in parallel as we want all test classes to be written in a similar style.

damithc · 2016-12-10T08:03:11Z

src/seedu/addressbook/data/AddressBook.java

+        return other == this // short circuit if same object
+                || (other instanceof AddressBook // instanceof handles nulls
+                && this.allPersons.equals(((AddressBook) other).allPersons)
+                && this.allTags.equals(((AddressBook) other).allTags));


These two lines should be indented further because it is misleading to align || with && (they are not at the same level in the condition tree)

Okay fixed!

damithc · 2016-12-10T08:13:01Z

test/java/seedu/addressbook/commands/FindCommandTest.java

+public class FindCommandTest extends CommandTest {
+
+    @Test
+    public void execute_find_isCaseSensitive() throws IllegalValueException {


This name does not follow the convention. For example, the 3rd part of the name should be the expected result.

Okay changed to execute_find_matchesOnlyIfCaseSensitive

damithc · 2016-12-10T08:15:22Z

test/java/seedu/addressbook/testutil/TestMain.java

+/**
+ * Test class for Main
+ */
+public class TestMain {


This PR is for testing the FindCommand class, not the 'find command' as typed by the user. Hence, I don't see the need for this class. Compare with #118

Okay removed!

damithc · 2016-12-10T08:21:03Z

test/java/seedu/addressbook/testutil/TypicalTestPersons.java

+
+            hopper = new PersonBuilder().withName("Grace Hopper").withPhone("83434567", true)
+                    .withEmail("gh@gmail.com", true).withAddress("5 Clementi Road", true).build();
+


When defining test data, try to follow a pattern that is easy to remember. For example, the first person's name has initials A. B. (e.g. Alice Betsy), second person's name has initials B. C. (Ben Charles) and so on. When things always follow a pattern, anything going out of that pattern (e.g. Ben suddenly appears at the start of the list) gets noticed immediately and sometimes can lead to discover bugs.

damithc · 2016-12-10T08:22:14Z

test/java/seedu/addressbook/testutil/PersonBuilder.java

+        return this;
+    }
+
+    public PersonBuilder withPhone(String phone, boolean isPrivate) throws IllegalValueException {


While we are at it, we can also have a withPrivatePhone method.

Okay. Implemented two methods for each field.
For example withPublicPhone and withPrivatePhone.

damithc · 2016-12-11T03:08:46Z

Seems like you have unrelated commits in this branch.

awhdesmond · 2016-12-11T04:03:33Z

Sorry, I rebased the commits wrongly! Have rectified.

damithc

A few more comments added.

damithc · 2016-12-11T08:22:54Z

test/java/seedu/addressbook/commands/FindCommandTest.java

+    }
+
+    @Test
+    public void execute_find_matchesOnlyIfCaseSensitive() throws IllegalValueException {


As we are testing FindCommand, execute implies find.
The method name can be execute_sameWordDifferentCase_notMatched(). In that case, this method needs to be split into two. Alternatively, you can name it execute_caseSensitivity() and test all cases of case sensitivity within the same method. In that case, use comments inside the method to describe each case.

Okay! I have split the original method into 2.

damithc · 2016-12-11T08:23:54Z

test/java/seedu/addressbook/commands/FindCommandTest.java

+    @Test
+    public void execute_find_matchesOnlyIfCaseSensitive() throws IllegalValueException {
+        List<ReadOnlyPerson> expectedOne = TestUtil.createList(new Person(TypicalTestPersons.amy));
+        List<ReadOnlyPerson> expectedZero = Collections.emptyList();


Define the variable closer to where it will be used.

You can use the same variable expected instead of declaring two variables with somewhat 'odd' names. :-)

damithc · 2016-12-11T08:26:59Z

test/java/seedu/addressbook/commands/FindCommandTest.java

+        assertCommandBehavior(command, Command.getMessageForPersonListShownSummary(expectedZero),
+                unmutatedAddressBook, addressBook);
+
+


Why two blank lines?

damithc · 2016-12-11T08:28:41Z

test/java/seedu/addressbook/commands/FindCommandTest.java

+        List<ReadOnlyPerson> expectedOne = TestUtil.createList(new Person(TypicalTestPersons.amy));
+        List<ReadOnlyPerson> expectedZero = Collections.emptyList();
+
+        String[] caseSensitiveKeywords = {"Amy"};


The name doesn't do a very good job of describing what the variable is. A word by itself is not 'case sensitive'. keyWordsWithMatchingCase?

damithc · 2016-12-11T08:34:02Z

test/java/seedu/addressbook/util/TestPerson.java

+import seedu.addressbook.data.person.ReadOnlyPerson;
+import seedu.addressbook.data.tag.UniqueTagList;
+
+public class TestPerson implements ReadOnlyPerson {


Why do we need this class? In any case, there should be a header comment to explain what it is.

Okay I removed this class. PersonBuilder now builds an instance of the Person class.

damithc · 2016-12-11T08:37:58Z

test/java/seedu/addressbook/util/TypicalTestPersons.java

+    public TypicalTestPersons() {
+        try {
+            amy = new PersonBuilder().withName("Amy Buck").withPublicPhone("91234567")
+                    .withPublicEmail("cs@gmail.com").withPublicAddress("1 Clementi Road").build();


telephone 91119111 and email ab@gmail.com instead of random values? (for the same reasons as before; if we follow patterns, it's easy to detect when things go wrong)

awhdesmond · 2016-12-11T10:44:09Z

Implemented the requested changes!

damithc

On the right track. A further set of revisions suggested.

damithc · 2016-12-11T13:45:52Z

test/java/seedu/addressbook/commands/FindCommandTest.java

+public class FindCommandTest {
+
+    @Rule
+    public ExpectedException thrown = ExpectedException.none();


Is this used anywhere?

damithc · 2016-12-11T14:07:38Z

src/seedu/addressbook/data/person/UniquePersonList.java

@@ -124,11 +124,12 @@ public void clear() {
    public Iterator<Person> iterator() {
        return internalList.iterator();
    }
-
+    


Blank lines should not contain spaces.

damithc · 2016-12-11T14:08:47Z

test/java/seedu/addressbook/commands/FindCommandTest.java

+    }
+
+
+


Why 3 blank lines here?

damithc · 2016-12-11T14:09:38Z

test/java/seedu/addressbook/commands/FindCommandTest.java

+
+
+    @Test
+    public void execute_find_matchesOnlyFullWordsInNames() throws IllegalValueException {


Renames this method and subsequent methods to to be consistent with the previous two methods.

damithc · 2016-12-11T14:12:46Z

test/java/seedu/addressbook/commands/FindCommandTest.java

+
+        FindCommand command = createFindCommand(keywords);
+        assertCommandBehavior(command, Command.getMessageForPersonListShownSummary(expected),
+                unmutatedAddressBook, addressBook);


Increasing indentation like this can improve readability (the opening and closing brackets appear on extreme left and extreme right respectively of the parameter text)

assertCommandBehavior(command, Command.getMessageForPersonListShownSummary(expected), unmutatedAddressBook, addressBook);

damithc · 2016-12-11T14:18:16Z

test/java/seedu/addressbook/commands/FindCommandTest.java

+
+
+    /**
+     * Creates a new find command.


FindCommand

Anyway, can be omitted as it doesn't add any value. It's the same as the method name.

damithc · 2016-12-11T14:21:46Z

test/java/seedu/addressbook/util/TypicalPersons.java

+        }
+    }
+
+    public void loadAddressBookWithSampleData(AddressBook ab) {


does this need to be public?

damithc · 2016-12-17T06:34:56Z

@KnewYouWereTrouble what's the status of this PR?

awhdesmond · 2016-12-17T08:22:13Z

Ready for review!

damithc

Nearly there. Some cosmetic changes suggested, mainly to reduce verbosity of the code.

damithc · 2016-12-17T09:13:22Z

test/java/seedu/addressbook/commands/FindCommandTest.java

+
+        FindCommand command = createFindCommand(keywordsWithMatchingCase);
+        assertCommandBehavior(command, Command.getMessageForPersonListShownSummary(expected),
+                                                                    unmutatedAddressBook, addressBook);


Command.getMessageForPersonListShownSummary seems to be repeated every time. May be you can put that part inside the assertCommandBehavior method itself?

Same thing for unmutatedAddressBook. It can be inside the method itself instead of passing as a parameter every time?

Okay! I moved them into the assertCommandBehavior method!

damithc · 2016-12-17T09:15:01Z

test/java/seedu/addressbook/commands/FindCommandTest.java

+
+    @Test
+    public void execute_sameWordDifferentCase_notMatched() throws IllegalValueException {
+        List<ReadOnlyPerson> expected = Collections.emptyList();


Declare this as a constant EMPTY_LIST and use it everywhere?

Alternatively, you can create this utility method:

public void assertNoResults(...){ assertCommandBehavior(..., Collections.emptyList(), ...); }

In that case you may want to rename assertCommandBehavior as assertResults (for symmetry)

damithc · 2016-12-17T09:29:09Z

test/java/seedu/addressbook/commands/FindCommandTest.java

+        List<ReadOnlyPerson> expected = TestUtil.createList(new Person(TypicalPersons.amy),
+                                                            new Person(TypicalPersons.bill));
+
+        String[] keywords = {"Amy", "Bill"};


You seem to be testing the first name only. What if there is a bug that limits the string matching to the first word of the name only?

Okay! added another keyword that is the last name of the person

damithc · 2016-12-17T09:30:30Z

test/java/seedu/addressbook/util/TypicalPersons.java

+
+    private void loadAddressBookWithSampleData(AddressBook ab) {
+        try {
+            for (Person t : getTypicalPersons()) {


Why t and not p?

awhdesmond · 2016-12-18T05:14:56Z

Ready for review!

damithc · 2016-12-18T05:51:03Z

test/java/seedu/addressbook/util/TypicalPersons.java

+ */
+public class TypicalPersons {
+
+    public static Person amy, bill, candy, donald, grace;


May be use static methods instead e.g. amy(), bill() etc. to create new objects every time? That way, we ensure that tests cannot influence each other via shared test objects.

@damithc Addressbook-Level4 has the same problem too. I'll open up an issue there.

Shouldn't we just copy over the code from AddressBook-Level4 in order to share as much code as possible?

In AddressBook-Level4 we have alice, benson, carl etc. but here we have amy, bill, candy etc. Seems like it will cause trouble when we want to share tests across different levels in the future.

@pyokagan I agree. But I don't mind doing it as a separate issue. The extra work caused by duplication of code is a good source of 'low hanging fruit' issues for new contributors to do.

On a somewhat related note, at the moment I'm not too eager to unify similarities between the levels. At our final steady state it may be desirable to have different levels doing the same thing in different ways, so that students get to see more ways of doing the same thing. As we are not yet certain what our final steady state is going to be, I don't mind if levels evolved somewhat independently for now.

In hindsight, I'm not sure if we should use the Builder pattern in level 2. It would be good for one level (level 2?) to not use the Builder pattern so that it can be contrasted with the next level (level 3?) that improves the situation by applying the Builder pattern. Anyway, that's something we can consider on a future day. We can leave it be for now. :-p

@damithc says:

The extra work caused by duplication of code is a good source of 'low hanging fruit' issues for new contributors to do.

It's more work for reviewers through, and will slow down the project. Reviewers may not be happy that they have to review the same things over and over again, but subtly different each time due to differences in code bases.

at the moment I'm not too eager to unify similarities between the levels.

I understand. However I hope that until there is a clear direction on how the different levels are going to differ from each other, we try to prevent them from diverging unnecessarily, simply because it is much easier to make code bases diverge rather than converge.

For an example of "diverging unnecessarily", for this PR I hope that the test data is kept exactly the same as the other levels, because future tests will be written for the test data. Having test data that is different will just cause unnecessary friction in the future should we ever try to bring tests from one level to another. Just try to cover our bases here :-)

damithc

A step in the right direction. I think we can push harder to reduce verbosity of the code.

damithc · 2016-12-18T05:51:56Z

src/seedu/addressbook/data/person/UniquePersonList.java

@@ -131,4 +131,5 @@ public boolean equals(Object other) {
                || (other instanceof UniquePersonList // instanceof handles nulls
                        && this.internalList.equals(((UniquePersonList) other).internalList));
    }
+


Unrelated change?

Need to remove this

damithc · 2016-12-18T05:55:21Z

test/java/seedu/addressbook/commands/FindCommandTest.java

+        String[] keywordsWithMatchingCase = {"Amy"};
+
+        FindCommand command = createFindCommand(keywordsWithMatchingCase);
+        assertCommandBehavior(command, expected, addressBook);


Still too verbose. Out of this 4 lines, the only things that are specific to this test case is Amy and amy. Everything else is just boilerplate and can be pushed into the utility method.

damithc · 2016-12-18T05:57:15Z

test/java/seedu/addressbook/commands/FindCommandTest.java

+
+    private final List<ReadOnlyPerson> EMPTY_LIST = Collections.emptyList();
+
+    @Before public void setUp() {


Annotation should be in a separate line.

damithc · 2016-12-24T02:15:21Z

@KnewYouWereTrouble reminder

awhdesmond · 2016-12-25T13:20:32Z

Ready for review!

damithc

Better now. Some more changes requested.

damithc · 2016-12-25T13:55:21Z

src/seedu/addressbook/data/person/UniquePersonList.java

@@ -131,4 +131,5 @@ public boolean equals(Object other) {
                || (other instanceof UniquePersonList // instanceof handles nulls
                        && this.internalList.equals(((UniquePersonList) other).internalList));
    }
+


Need to remove this

damithc · 2016-12-25T13:59:47Z

test/java/seedu/addressbook/util/TypicalPersons.java

+/**
+ * Class to generate typical test persons
+ */
+public class TypicalPersons {


We did some tweaks to Typical persons in level 4 (see this)
Let's follow the same here.

damithc · 2016-12-25T14:02:20Z

test/java/seedu/addressbook/util/PersonBuilder.java

+/**
+ * Factory class for building TestPerson
+ */
+public class PersonBuilder {


I think we can do without a PersonBuilder. Level 2 a bit too early to do this kind of thing. Sorry I didn't think about it earlier.

damithc · 2016-12-25T14:13:17Z

test/java/seedu/addressbook/commands/FindCommandTest.java

+        String[] keywordsWithMatchingCase = {"Amy"};
+
+        assertFindCommandBehavior(keywordsWithMatchingCase, expected, addressBook);
+    }


Can be made even more concise:

public void execute() throws exception { //same word, same case: matched assertFindCommandBehavior( {"Amy"}, TypicalPersons.amy()); //same word, different case: not matched assertFindCommandBehavior( {"amy"}); ... } private void assertFindCommandBehavior(String[] keywords, Person... expectedPersons){ ... }

Or even this (slightly better):

public void execute() throws exception { //same word, same case: matched assertFindCommandBehavior( {"Amy"}, {TypicalPersons.amy()}); //same word, different case: not matched assertFindCommandBehavior( {"amy"}, {}); ... } private void assertFindCommandBehavior(String[] keywords, Person[] expectedPersons){ ... }

awhdesmond · 2016-12-27T04:41:23Z

Ready for review

damithc

A few more minor comments added. Nearly there 👍

damithc · 2016-12-27T05:54:08Z

test/java/seedu/addressbook/commands/FindCommandTest.java

+        assertFindCommandBehavior(new String[]{"my"}, EMPTY_LIST);
+
+        //multiple words: matched
+        assertFindCommandBehavior(new String[]{"Amy", "Bill", "Destiny"}, TestUtil.createList(td.amy, td.bill, td.candy));


You have a case of matching first name only, and second name only. How about a case of matching both names? i.e. "Amy", "Bill", "Candy", "Destiny"

damithc · 2016-12-27T05:55:22Z

test/java/seedu/addressbook/commands/FindCommandTest.java

+        assertFindCommandBehavior(new String[]{"Amy", "Bill", "Destiny"}, TestUtil.createList(td.amy, td.bill, td.candy));
+    }
+
+


Extra blank line

damithc · 2016-12-27T05:56:41Z

test/java/seedu/addressbook/commands/FindCommandTest.java

+
+
+    /**
+     * Executes the find command, and checks that the execution was what we had expected.


Can be more specific. e.g. Executes the find command for the given keywords and verifies the result matches the persons in the expectedPersonList exactly

damithc · 2016-12-27T06:00:59Z

test/java/seedu/addressbook/commands/FindCommandTest.java

+
+        //multiple words: matched
+        assertFindCommandBehavior(new String[]{"Amy", "Bill", "Destiny"}, TestUtil.createList(td.amy, td.bill, td.candy));
+    }


Other interesting cases to test:

Repeated keyword

Keyword matching a word in address

damithc · 2016-12-27T06:01:46Z

test/java/seedu/addressbook/commands/FindCommandTest.java

+
+/**
+ * Test class for the Find Command
+ */


This header comment doesn't add any value. Can be removed.

damithc · 2016-12-29T03:34:27Z

Reviewed 1 of 15 files at r1, 4 of 14 files at r2, 1 of 5 files at r4, 3 of 4 files at r9.
Review status: all files reviewed at latest revision, 6 unresolved discussions.

Comments from Reviewable

… FindCommandTest to asserCommandBehavior method

awhdesmond · 2016-12-29T10:45:20Z

Ready for review!

damithc · 2016-12-29T14:26:07Z

Reviewed 2 of 2 files at r11.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

src/seedu/addressbook/data/tag/UniqueTagList.java, line 169 at r2 (raw file):

                        && this.internalList.equals(((UniqueTagList) other).internalList));
    }

Why was this line deleted? Usually we leave a blank line (or two) between last method and the closing brace of the class.

Comments from Reviewable

damithc · 2016-12-30T06:49:14Z

@pyokagan any further inputs?

pyokagan

Please fix these style errors as well:

[ant:checkstyle] [ERROR] /home/pyokagan/pyk/addressbook-level2/src/seedu/addressbook/data/AddressBook.java:146: Line matches the illegal pattern 'Trailing whitespace'. [Regexp]
[ant:checkstyle] [ERROR] /home/pyokagan/pyk/addressbook-level2/src/seedu/addressbook/data/tag/UniqueTagList.java:162: Line matches the illegal pattern 'Trailing whitespace'. [Regexp]
[ant:checkstyle] [ERROR] /home/pyokagan/pyk/addressbook-level2/test/java/seedu/addressbook/commands/FindCommandTest.java:24: Line matches the illegal pattern 'Trailing whitespace'. [Regexp]
[ant:checkstyle] [WARN] /home/pyokagan/pyk/addressbook-level2/test/java/seedu/addressbook/commands/FindCommandTest.java:25:40: Name 'EMPTY_LIST' must match pattern '^[a-z][a-zA-Z0-9]*$'. [MemberName]
[ant:checkstyle] [ERROR] /home/pyokagan/pyk/addressbook-level2/test/java/seedu/addressbook/commands/FindCommandTest.java:27: Line matches the illegal pattern 'Trailing whitespace'. [Regexp]
[ant:checkstyle] [ERROR] /home/pyokagan/pyk/addressbook-level2/test/java/seedu/addressbook/commands/FindCommandTest.java:37: Line matches the illegal pattern 'Trailing whitespace'. [Regexp]
[ant:checkstyle] [ERROR] /home/pyokagan/pyk/addressbook-level2/test/java/seedu/addressbook/commands/FindCommandTest.java:40: Line matches the illegal pattern 'Trailing whitespace'. [Regexp]
[ant:checkstyle] [ERROR] /home/pyokagan/pyk/addressbook-level2/test/java/seedu/addressbook/commands/FindCommandTest.java:43: Line matches the illegal pattern 'Trailing whitespace'. [Regexp]
[ant:checkstyle] [ERROR] /home/pyokagan/pyk/addressbook-level2/test/java/seedu/addressbook/commands/FindCommandTest.java:45: Line is longer than 120 characters (found 131). [LineLength]
[ant:checkstyle] [ERROR] /home/pyokagan/pyk/addressbook-level2/test/java/seedu/addressbook/commands/FindCommandTest.java:46: Line matches the illegal pattern 'Trailing whitespace'. [Regexp]
[ant:checkstyle] [ERROR] /home/pyokagan/pyk/addressbook-level2/test/java/seedu/addressbook/commands/FindCommandTest.java:49: Line matches the illegal pattern 'Trailing whitespace'. [Regexp]
[ant:checkstyle] [ERROR] /home/pyokagan/pyk/addressbook-level2/test/java/seedu/addressbook/commands/FindCommandTest.java:55: Line is longer than 120 characters (found 134). [LineLength]
[ant:checkstyle] [ERROR] /home/pyokagan/pyk/addressbook-level2/test/java/seedu/addressbook/commands/FindCommandTest.java:60: Line matches the illegal pattern 'Trailing whitespace'. [Regexp]
[ant:checkstyle] [ERROR] /home/pyokagan/pyk/addressbook-level2/test/java/seedu/addressbook/util/TypicalPersons.java:18: Line matches the illegal pattern 'Trailing whitespace'. [Regexp]
[ant:checkstyle] [ERROR] /home/pyokagan/pyk/addressbook-level2/test/java/seedu/addressbook/util/TypicalPersons.java:21: Line matches the illegal pattern 'Trailing whitespace'. [Regexp]
[ant:checkstyle] [ERROR] /home/pyokagan/pyk/addressbook-level2/test/java/seedu/addressbook/util/TypicalPersons.java:23: Line matches the illegal pattern 'Trailing whitespace'. [Regexp]
[ant:checkstyle] [ERROR] /home/pyokagan/pyk/addressbook-level2/test/java/seedu/addressbook/util/TypicalPersons.java:25: Line is longer than 120 characters (found 123). [LineLength]
[ant:checkstyle] [ERROR] /home/pyokagan/pyk/addressbook-level2/test/java/seedu/addressbook/util/TypicalPersons.java:25: Line matches the illegal pattern 'Trailing whitespace'. [Regexp]
[ant:checkstyle] [ERROR] /home/pyokagan/pyk/addressbook-level2/test/java/seedu/addressbook/util/TypicalPersons.java:31: Line matches the illegal pattern 'Trailing whitespace'. [Regexp]
[ant:checkstyle] [ERROR] /home/pyokagan/pyk/addressbook-level2/test/java/seedu/addressbook/util/TypicalPersons.java:32: Line matches the illegal pattern 'Trailing whitespace'. [Regexp]
[ant:checkstyle] [ERROR] /home/pyokagan/pyk/addressbook-level2/test/java/seedu/addressbook/util/TypicalPersons.java:34: Line matches the illegal pattern 'Trailing whitespace'. [Regexp]
[ant:checkstyle] [ERROR] /home/pyokagan/pyk/addressbook-level2/test/java/seedu/addressbook/util/TypicalPersons.java:35: Line matches the illegal pattern 'Trailing whitespace'. [Regexp]
[ant:checkstyle] [ERROR] /home/pyokagan/pyk/addressbook-level2/test/java/seedu/addressbook/util/TypicalPersons.java:42: Line matches the illegal pattern 'Trailing whitespace'. [Regexp]
[ant:checkstyle] [ERROR] /home/pyokagan/pyk/addressbook-level2/test/java/seedu/addressbook/util/TypicalPersons.java:44: Line matches the illegal pattern 'Trailing whitespace'. [Regexp]
[ant:checkstyle] [ERROR] /home/pyokagan/pyk/addressbook-level2/test/java/seedu/addressbook/util/TypicalPersons.java:48: Line matches the illegal pattern 'Trailing whitespace'. [Regexp]

pyokagan · 2016-12-30T09:30:22Z

test/java/seedu/addressbook/commands/FindCommandTest.java

+    private AddressBook addressBook;
+    private TypicalPersons td;
+
+    private final List<ReadOnlyPerson> EMPTY_LIST = Collections.emptyList();


Naming doesn't add a lot of value. I think you can just inline Collections.emptyList() directly.

pyokagan · 2016-12-30T09:30:30Z

test/java/seedu/addressbook/commands/FindCommandTest.java

+    @Test
+    public void execute() throws IllegalValueException {
+        //same word, same case: matched
+        assertFindCommandBehavior(new String[]{"Amy"}, TestUtil.createList(td.amy));


Use Arrays.asList() instead? I'm not sure why TestUtil.createList() exists, actually. @yamgent ?

createList() is redundant, my bad.

(I was under the impression that Arrays.asList() was returning List<Person> instead of List<ReadOnlyPerson>, because my parameters were all Persons. Looks like I was wrong.)

@yamgent create an issue to get rid of the unused method? We can leave it for new contributors.

@damithc ok, created #134.

pyokagan · 2016-12-30T09:33:13Z

test/java/seedu/addressbook/util/TypicalPersons.java

+            assert false : "not possible";
+        }
+
+


Remove these empty lines.

pyokagan · 2016-12-30T09:33:38Z

test/java/seedu/addressbook/util/TypicalPersons.java

+
+    public TypicalPersons() {
+        try {
+            amy =  new Person(new Name("Amy Buck"), new Phone("91119111", false), new Email("ab@gmail.com", false), 


One space between the = only.

pyokagan · 2016-12-30T09:36:58Z

test/java/seedu/addressbook/commands/FindCommandTest.java

+public class FindCommandTest {
+
+    private AddressBook addressBook;
+    private TypicalPersons td;


Can make these final as well.

Actually, I think you can just assign the fields here and do without the setUp() method (JUnit constructs a new instance for every test).

awhdesmond · 2016-12-30T11:45:23Z

Ready for review!

pyokagan · 2017-01-01T17:33:26Z

@KnewYouWereTrouble Just fix these problems below. Configure your IDE to automatically trim trailing whitespace so you won't have this problem any more. Everything else looks good.

[ant:checkstyle] [ERROR] /home/pyokagan/pyk/addressbook-level2/src/seedu/addressbook/data/tag/UniqueTagList.java:169: Line matches the illegal pattern 'Trailing whitespace'. [Regexp]
[ant:checkstyle] [ERROR] /home/pyokagan/pyk/addressbook-level2/test/java/seedu/addressbook/commands/FindCommandTest.java:35: Line matches the illegal pattern 'Trailing whitespace'. [Regexp]
[ant:checkstyle] [ERROR] /home/pyokagan/pyk/addressbook-level2/test/java/seedu/addressbook/commands/FindCommandTest.java:46: Line matches the illegal pattern 'Trailing whitespace'. [Regexp]

awhdesmond · 2017-01-02T05:01:30Z

Ready for review!

damithc requested changes Dec 10, 2016

View reviewed changes

damithc self-assigned this Dec 10, 2016

Neurrone mentioned this pull request Dec 10, 2016

Add unit tests for AddCommand class #108 #118

Merged

damithc requested changes Dec 11, 2016

View reviewed changes

damithc added the s.Ongoing label Dec 11, 2016

damithc mentioned this pull request Dec 11, 2016

Add unit tests for AddressBook class #105 #121

Merged

damithc requested changes Dec 11, 2016

View reviewed changes

damithc requested changes Dec 17, 2016

View reviewed changes

damithc reviewed Dec 18, 2016

View reviewed changes

damithc requested changes Dec 18, 2016

View reviewed changes

This was referenced Dec 20, 2016

Rework how TypicalTestPersons exposes its persons se-edu/addressbook-level4#221

Closed

Rework how TypicalTestPersons exposes its persons #221 se-edu/addressbook-level4#224

Merged

damithc requested changes Dec 25, 2016

View reviewed changes

damithc requested changes Dec 27, 2016

View reviewed changes

theapeeeee added 6 commits December 29, 2016 18:32

Add testutil classes to facilitate testing

809a9ff

Add TestMain class to facilitate testing of commands

cb04f84

Add equals() methond in AddressBook, UniquePersonList and UniqueTagList

228696b

Add FindCommandTest and CommandTest

d664932

Fix code style

d40448b

Add indentation for equals() method

1680011

theapeeeee added 14 commits December 29, 2016 18:32

Reimplement FindCommandTest without use of TestMain

ba2d41d

Remove duplicate equals() method in UniquePersonList

4fe6e4d

Remove TestPerson class

cb7db69

Refactor FindCommandTest

3d247be

Refactor FindCommandTest

4ab7163

Refactor FindCommandTest

acc5927

Remove empty line at top of assertCommandBehavior

6793926

Refactor TypicalPersons.java and shift instantiating a FindCommand in…

a90bdb7

… FindCommandTest to asserCommandBehavior method

Rename assertCommandBehavior to assertFindCommandBehavior

f7e7607

Remove PersonBuilder and make TypicalPersons fields non-static

fbc8f42

Refactor FindCommandTest by inlining test methods

71d0d84

Remove unused imports

349fb15

Add new test cases

94f4cbd

Remove trailing empty lines

0f539cc

damithc assigned pyokagan Dec 29, 2016

damithc approved these changes Dec 30, 2016

View reviewed changes

damithc mentioned this pull request Dec 30, 2016

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

Merged

pyokagan requested changes Dec 30, 2016

View reviewed changes

theapeeeee added 2 commits December 30, 2016 19:21

Fix coding style

642a760

Remove unused imports

1dd21bb

Remove trailing whitespaces

ce4feb5

pyokagan approved these changes Jan 3, 2017

View reviewed changes

pyokagan merged commit a909e09 into se-edu:master Jan 3, 2017


		hopper = new PersonBuilder().withName("Grace Hopper").withPhone("83434567", true)
		.withEmail("gh@gmail.com", true).withAddress("5 Clementi Road", true).build();

		assertCommandBehavior(command, Command.getMessageForPersonListShownSummary(expectedZero),
		unmutatedAddressBook, addressBook);



		@Test
		public void execute_find_matchesOnlyFullWordsInNames() throws IllegalValueException {


		private final List<ReadOnlyPerson> EMPTY_LIST = Collections.emptyList();

		@Before public void setUp() {

		assertFindCommandBehavior(new String[]{"Amy", "Bill", "Destiny"}, TestUtil.createList(td.amy, td.bill, td.candy));
		}



		/**
		* Executes the find command, and checks that the execution was what we had expected.

Add unit tests for FindCommand class #110 #120

Add unit tests for FindCommand class #110 #120

Conversation

awhdesmond commented Dec 9, 2016 • edited by damithc Loading

damithc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awhdesmond Dec 10, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damithc commented Dec 11, 2016

awhdesmond commented Dec 11, 2016

damithc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awhdesmond commented Dec 11, 2016

damithc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damithc commented Dec 17, 2016

awhdesmond commented Dec 17, 2016

damithc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awhdesmond commented Dec 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damithc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damithc commented Dec 24, 2016

awhdesmond commented Dec 25, 2016

damithc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damithc Dec 25, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awhdesmond commented Dec 27, 2016

damithc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awhdesmond commented Dec 9, 2016 •

edited by damithc

Loading

awhdesmond Dec 10, 2016 •

edited

Loading

damithc Dec 25, 2016 •

edited

Loading