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

Jump to bottom

[#9119] FeedbackResponseCommentsLogicTest: Fix verification for test #9280

Merged
merged 3 commits into from Jan 2, 2019
Merged

[#9119] FeedbackResponseCommentsLogicTest: Fix verification for test #9280

merged 3 commits into from Jan 2, 2019

Conversation

rrtheonlyone
Copy link
Contributor

Fixes #9119

The initial testing code did not verify the case where a duplicate comment is added.

When a duplicate comment is added, the expected behaviour is:

  1. Check database and retrieve the ID of the existing comment
  2. Set ID of the existing comment to the duplicate comment
  3. Update this comment if needed

As such, in the test, I do a verification of whether the ID has been set correctly. I also change the commentText and check if the update works.

@rrtheonlyone rrtheonlyone reopened this Dec 18, 2018
xpdavid
xpdavid reviewed Dec 19, 2018
View reviewed changes
src/test/java/teammates/test/cases/logic/FeedbackResponseCommentsLogicTest.java Outdated
@@ -85,19 +85,22 @@ public void testCreateFeedbackResponseComment() throws Exception {
verifyPresentInDatastore(frComment);

______TS("typical successful case: frComment already exists");
Copy link
Contributor

Choose a reason for hiding this comment

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

Describe the behaviour when frComment already exists.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay will do

src/test/java/teammates/test/cases/logic/FeedbackResponseCommentsLogicTest.java
@@ -85,19 +85,22 @@ public void testCreateFeedbackResponseComment() throws Exception {
verifyPresentInDatastore(frComment);

______TS("typical successful case: frComment already exists");

FeedbackResponseCommentAttributes actualComment =
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe you are trying to verify that the comment exist in the database before the test.

assertNotNull(frcLogic.getFeedbackResponseComment(frComment.getId()))

The above can already do the job?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, this is to fetch the existing comment from the database. I use this to verify that the duplicate comment has the same ID as the existing one.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then put it together with L97.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

L97? That would defeat the purpose, since after this I create a duplicate feedback response comment in the database. The variable frcomment will then be assigned the id.

If I fetch frcomment from the database following that, it would always pass. I need to be able to fetch the original comment and verify that its ID is the same as the one given to frcomment

Unless, there is a better way to do this.

Copy link
Contributor

@xpdavid xpdavid Dec 19, 2018 •
edited

Choose a reason for hiding this comment

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

since after this I create a duplicate feedback response comment in the database.

Wait, we are not creating duplicate feedback response right? If the id is the same, the creation will become update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the documentation! Do see if its more clear now

@xpdavid xpdavid self-assigned this Dec 19, 2018
@xpdavid xpdavid added the s.Ongoing The PR is being worked on by the author(s) label Dec 19, 2018
xpdavid
xpdavid reviewed Dec 19, 2018
View reviewed changes
src/test/java/teammates/test/cases/logic/FeedbackResponseCommentsLogicTest.java Outdated
break;
}
}
// check that it uses existing ID from database
Copy link
Contributor

@xpdavid xpdavid Dec 19, 2018 •
edited

Choose a reason for hiding this comment

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

I get what u mean finally. You are trying to verify that the ID has been set by the createFeedbackResponseComment. 👍 (You may want to comment it here.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay will update the comment 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

BTW, will the test fail if you remove the .setId() in createFeedbackResponseComment (i.e. make sure frComment has empty id before 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.

Yup it will fail: the .getId() method will return null and an exception will be thrown.

This is the current behaviour of the test even before the PR. The deleteFeedbackResponseCommentById method in the cleanup portion relies on the ID being set.

xpdavid
xpdavid approved these changes Dec 19, 2018
View reviewed changes
Copy link
Contributor

@xpdavid xpdavid left a comment

Choose a reason for hiding this comment

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

LGTM！

@xpdavid xpdavid added s.FinalReview The PR is ready for final review and removed s.Ongoing The PR is being worked on by the author(s) labels Dec 19, 2018
@xpdavid xpdavid requested a review from damithc December 19, 2018 09:40
@xpdavid xpdavid added this to the V6.16.0 milestone Dec 19, 2018
@xpdavid xpdavid added the c.Task Other non-user-facing works, e.g. refactoring, adding tests label Dec 19, 2018
@wkurniawan07 wkurniawan07 added s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging and removed s.FinalReview The PR is ready for final review labels Dec 19, 2018
@xpdavid xpdavid merged commit 1b65e67 into TEAMMATES:master Jan 2, 2019
jacoblipech pushed a commit to jacoblipech/teammates that referenced this pull request Jan 16, 2019
@rrtheonlyone
[TEAMMATES#9119] FeedbackResponseCommentsLogicTest: Fix verification …
91ef4bc 
…for test (TEAMMATES#9280)

[TEAMMATES#9119] FeedbackResponseCommentsLogicTest: Fix verification for test (TEAMMATES#9280)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@damithc damithc damithc approved these changes

@xpdavid xpdavid xpdavid approved these changes

Assignees

@xpdavid xpdavid

Labels
c.Task Other non-user-facing works, e.g. refactoring, adding tests s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging
Projects
None yet
Milestone
V6.16.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@rrtheonlyone @damithc @xpdavid @wkurniawan07