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

[#11572] Notification Feature - Notification Logic Tests #11696

Merged
merged 3 commits into from
Apr 4, 2022
Merged

[#11572] Notification Feature - Notification Logic Tests #11696

merged 3 commits into from
Apr 4, 2022

Conversation

ziqing26
Copy link
Contributor

@ziqing26 ziqing26 commented Apr 1, 2022

Part of #11572

Outline of Solution

  • Add tests for NotificationLogic, especially for functions related to GET, PUT and DELETE.

@ziqing26 ziqing26 added the s.ToReview The PR is waiting for review(s) label Apr 1, 2022
NicolasCwy
NicolasCwy reviewed Apr 1, 2022
View reviewed changes
Copy link
Contributor

@NicolasCwy NicolasCwy left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Some comments which we can discuss on!

src/test/java/teammates/logic/core/NotificationsLogicTest.java Outdated Show resolved Hide resolved
src/test/java/teammates/logic/core/NotificationsLogicTest.java Outdated Show resolved Hide resolved
fsgmhoward
fsgmhoward reviewed Apr 2, 2022
View reviewed changes
Copy link
Member

@fsgmhoward fsgmhoward left a comment

Choose a reason for hiding this comment

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

Just a few comments. Overall great effort!

src/test/java/teammates/logic/core/NotificationsLogicTest.java Outdated Show resolved Hide resolved
src/test/java/teammates/logic/core/NotificationsLogicTest.java Outdated Show resolved Hide resolved
zhaojj2209
zhaojj2209 reviewed Apr 2, 2022
View reviewed changes
Copy link
Contributor

@zhaojj2209 zhaojj2209 left a comment

Choose a reason for hiding this comment

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

Good coverage overall!
Other than what has already been pointed out, just to add some cosmetic changes:

src/test/java/teammates/logic/core/NotificationsLogicTest.java Outdated Show resolved Hide resolved
src/test/java/teammates/logic/core/NotificationsLogicTest.java Outdated Show resolved Hide resolved
src/test/java/teammates/logic/core/NotificationsLogicTest.java Outdated Show resolved Hide resolved
@ziqing26
Refactor notification logic test
ca2348c 
* Increase usage of typical databundle and reduce entity creation during
  test
* Improve tests for getActiveNotificationsForTargetUser
fsgmhoward
fsgmhoward approved these changes Apr 3, 2022
View reviewed changes
Copy link
Member

@fsgmhoward fsgmhoward left a comment

Choose a reason for hiding this comment

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

LGTM.

src/test/java/teammates/logic/core/NotificationsLogicTest.java Outdated Show resolved Hide resolved
NicolasCwy
NicolasCwy approved these changes Apr 4, 2022
View reviewed changes
Copy link
Contributor

@NicolasCwy NicolasCwy left a comment

Choose a reason for hiding this comment

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

LGTM, nice work!

zhaojj2209
zhaojj2209 approved these changes Apr 4, 2022
View reviewed changes
Copy link
Contributor

@zhaojj2209 zhaojj2209 left a comment

Choose a reason for hiding this comment

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

LGTM!

@NicolasCwy NicolasCwy added s.FinalReview The PR is ready for final review and removed s.ToReview The PR is waiting for review(s) labels Apr 4, 2022
t-cheepeng
t-cheepeng approved these changes Apr 4, 2022
View reviewed changes
Copy link
Contributor

@t-cheepeng t-cheepeng left a comment

Choose a reason for hiding this comment

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

LGTM

jianhandev
jianhandev reviewed Apr 4, 2022
View reviewed changes
Copy link
Contributor

@jianhandev jianhandev left a comment

Choose a reason for hiding this comment

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

Apart from the comment by @fsgmhoward, LGTM.

@ziqing26 ziqing26 added the s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging label Apr 4, 2022
@jianhandev jianhandev removed the s.FinalReview The PR is ready for final review label Apr 4, 2022
@jianhandev jianhandev merged commit 0e3286b into TEAMMATES:notification-feature Apr 4, 2022
jianhandev pushed a commit that referenced this pull request Apr 5, 2022
@ziqing26
[#11572] Notification Feature - Notification Logic Tests (#11696)
3e1c8c0 
* Add tests to NotificationsLogic

* Refactor notification logic test

* Increase usage of typical databundle and reduce entity creation during
  test

* Improve tests for getActiveNotificationsForTargetUser

* Remove check for presence in database
jianhandev pushed a commit that referenced this pull request Apr 5, 2022
@ziqing26
[#11572] Notification Feature - Notification Logic Tests (#11696)
9d161e2 
* Add tests to NotificationsLogic

* Refactor notification logic test

* Increase usage of typical databundle and reduce entity creation during
  test

* Improve tests for getActiveNotificationsForTargetUser

* Remove check for presence in database
jianhandev pushed a commit that referenced this pull request Apr 6, 2022
@ziqing26
[#11572] Notification Feature - Notification Logic Tests (#11696)
c51d2a4 
* Add tests to NotificationsLogic

* Refactor notification logic test

* Increase usage of typical databundle and reduce entity creation during
  test

* Improve tests for getActiveNotificationsForTargetUser

* Remove check for presence in database
jianhandev pushed a commit that referenced this pull request Apr 6, 2022
@ziqing26 @jianhandev
[#11572] Notification Feature - Notification Logic Tests (#11696)
262b722 
* Add tests to NotificationsLogic

* Refactor notification logic test

* Increase usage of typical databundle and reduce entity creation during
  test

* Improve tests for getActiveNotificationsForTargetUser

* Remove check for presence in database
@wkurniawan07 wkurniawan07 added the c.Task Other non-user-facing works, e.g. refactoring, adding tests label Apr 6, 2022
@wkurniawan07 wkurniawan07 added this to the V8.13.0 milestone Apr 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jianhandev jianhandev jianhandev left review comments

@zhaojj2209 zhaojj2209 zhaojj2209 approved these changes

@t-cheepeng t-cheepeng t-cheepeng approved these changes

@NicolasCwy NicolasCwy NicolasCwy approved these changes

@fsgmhoward fsgmhoward fsgmhoward approved these changes

Assignees
No one assigned
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
V8.13.0
Development

Successfully merging this pull request may close these issues.
7 participants
@ziqing26 @fsgmhoward @NicolasCwy @t-cheepeng @zhaojj2209 @jianhandev @wkurniawan07