[#12048] Migrate tests for GetNotificationsActionTest #13219
Conversation
src/test/java/teammates/sqlui/webapi/GetNotificationsActionTest.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| @Test | ||
| protected void testAccessControl_instructorAccessStudentNotification_shouldFail() { |
There was a problem hiding this comment.
What's the reason to use protected for most of the test methods? If we don't need to limit visibility, let's change all to public
There was a problem hiding this comment.
After checking some files in the same directory, I think there's no standardisation on this. Perhaps you can just stick to one, public protected or or no modifier at all.
| List<Notification> notificationToCheck = | ||
| mockLogic.getActiveNotificationsByTargetUser(testNotification.getTargetUser()); | ||
| notificationToCheck.forEach(n -> assertFalse(n.isShown())); |
There was a problem hiding this comment.
I think you did not set up a return value for when getActiveNotificationsByTargetUser is called, so notificationToCheck is null by default and the assertion on line 169 won't be run.
| List<Notification> notificationAttributes = | ||
| mockLogic.getActiveNotificationsByTargetUser(NotificationTargetUser.INSTRUCTOR); | ||
| notificationAttributes = notificationAttributes.stream() | ||
| .filter(n -> !readNotificationsId.contains(n.getId().toString())) | ||
| .collect(Collectors.toList()); | ||
| notificationAttributes.forEach(n -> assertTrue(n.isShown())); |
There was a problem hiding this comment.
Let's rename notificationAttributes to some other names such as activeNotifications becuase 1) XXXAttribute is the naming convention for datastore entities 2) it is clearer what kind these notifications are of
Part of #12048
Outline of Solution
Added the unit tests for GetNotificationsActionTest for PostgreSQL.
PS: This is not a duplicate of #13210 if anyone gets confused by the similar name