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 deadline test for PersonTest.java #218

Merged
merged 4 commits into from
Apr 6, 2021

Conversation

lue97
Copy link
Collaborator

@lue97 lue97 commented Apr 6, 2021

No description provided.

@lue97 lue97 requested review from ivantjh and Assyarul April 6, 2021 09:28
@lue97 lue97 added the type.Chore Something that needs to be done, but not a story, bug, or an epic. e.g. Move testing code into a new label Apr 6, 2021
Copy link
Collaborator

@ivantjh ivantjh left a comment

Choose a reason for hiding this comment

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

lgtm!


editedAlice = new PersonBuilder(aliceCopy).build()
.withGoal(new Goal(Goal.Frequency.WEEKLY))
.withMeetings(Arrays.asList(TypicalMeetings.getTypicalMeetings().clone()));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually don't have to clone right?

Person aliceCopy = new PersonBuilder(ALICE).build();
LocalDate date = LocalDate.of(2021, 3, 30);

Person editedAlice = new PersonBuilder(aliceCopy).build()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just rename all aliceCopy to ALICE, shouldn't need to copy here

// no prior meetings
editedAlice = new PersonBuilder(aliceCopy).build()
.withGoal(new Goal(Goal.Frequency.WEEKLY))
.withMeetings(new ArrayList<>());
Copy link
Collaborator

Choose a reason for hiding this comment

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

.withMeetings(Collections.emptyList()) would be better

@@ -21,4 +21,13 @@
.withDate(LocalDate.now())
.withTime(LocalTime.now())
.build();

public static Meeting[] getTypicalMeetings() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can shift this to PersonTest because getTypicalMeetings() feels like I am retrieving all the meetings from TypicalMeetings but that is not the case here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I merely did this as TypicalPersons has a getTypicalPersons method. I suppose we could have a test equivalent of SampleDataUtil in main

@lue97 lue97 merged commit 41922a9 into AY2021S2-CS2103T-W14-1:master Apr 6, 2021
@lue97 lue97 mentioned this pull request Apr 7, 2021
@ivantjh ivantjh added this to the v1.4 milestone Apr 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type.Chore Something that needs to be done, but not a story, bug, or an epic. e.g. Move testing code into a new
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants