[#12048] Migrate tests for GetStudentsActionTest #13243
Conversation
There was a problem hiding this comment.
Appreciate your effort! This is a complex class to test.
Just keep in mind that all the Google IDs are null by default since Student and Instructor does not have an Account unless we set using setAccount. So better to store the IDs in variables so they can be reused for stubbing or logging in.
Also, consider standardizing the test/variable names (for instructor-related) to something like withCoursePermission, withSectionPermission, withoutCoursePermission, and withoutSectionPermission, rather than using "same" or "different" so it's clearer which conditional branch is being tested. For teams, we can still use "same" or "different."
mingyang143
added
s.Ongoing
| } | ||
|
|
||
| @Test | ||
| void testExecute_instructorWithoutPermissionWithInvalidTeamParams_emptyList() { |
There was a problem hiding this comment.
small nit here lets also rename to WithNullTeamParams since null is a valid value for this action
| void testExecute_instructorWithoutPermissionWithTeamParams_success() { | ||
| loginAsInstructor(stubInstructorWithoutPrivileges.getGoogleId()); | ||
| when(mockLogic.getInstructorByGoogleId(stubCourse.getId(), stubInstructorWithoutPrivileges.getGoogleId())) | ||
| .thenReturn(stubInstructorWithoutPrivileges); | ||
| when(mockLogic.getStudentsByTeamName(stubTeamOne.getName(), stubCourse.getId())).thenReturn(stubStudentListOne); | ||
|
|
||
| String[] params = { | ||
| Const.ParamsNames.COURSE_ID, stubCourse.getId(), | ||
| Const.ParamsNames.TEAM_NAME, stubTeamOne.getName(), | ||
| }; | ||
| GetStudentsAction action = getAction(params); | ||
| JsonResult jsonResult = getJsonResult(action); | ||
| StudentsData actualStudentsData = (StudentsData) jsonResult.getOutput(); | ||
|
|
||
| verifyStudentsData(stubStudentListOne, actualStudentsData, Type.STUDENT); | ||
| verify(mockLogic, times(1)).getStudentsByTeamName(stubTeamOne.getName(), stubCourse.getId()); | ||
| verify(mockLogic, never()).getStudentsForCourse(stubCourse.getId()); | ||
| } |
There was a problem hiding this comment.
I think since the test skips checkSpecificAccessControl, this test runs with no problems but it should fail the access control check.
Lets modify this to be a AccessControlTest instead
Same for #13243 (comment)
| @Test | ||
| void testExecute_instructorWithDifferentSectionPrivilegesAsStudents_emptyList() { | ||
| InstructorPrivileges wrongPrivileges = | ||
| new InstructorPrivileges(Const.InstructorPermissionRoleNames.INSTRUCTOR_PERMISSION_ROLE_CUSTOM); | ||
| wrongPrivileges.updatePrivilege("random-1", | ||
| Const.InstructorPermissions.CAN_VIEW_STUDENT_IN_SECTIONS, true); | ||
| Instructor stubInstructorWithOnlyViewPrivilegesForDifferentSection = | ||
| new Instructor(stubCourse, "instructor-3-name", "valid3@teammates.tmt", | ||
| false, Const.DEFAULT_DISPLAY_NAME_FOR_INSTRUCTOR, customRole, wrongPrivileges); | ||
| loginAsInstructor(stubInstructorWithOnlyViewPrivilegesForDifferentSection.getGoogleId()); | ||
| when(mockLogic.getInstructorByGoogleId(stubCourse.getId(), | ||
| stubInstructorWithOnlyViewPrivilegesForDifferentSection.getGoogleId())) | ||
| .thenReturn(stubInstructorWithOnlyViewPrivilegesForDifferentSection); | ||
| when(mockLogic.getStudentsForCourse(stubCourse.getId())).thenReturn(stubStudentListTwo); | ||
|
|
||
| String[] params = { | ||
| Const.ParamsNames.COURSE_ID, stubCourse.getId(), | ||
| }; | ||
| GetStudentsAction action = getAction(params); | ||
| JsonResult jsonResult = getJsonResult(action); | ||
| StudentsData actualStudentsData = (StudentsData) jsonResult.getOutput(); | ||
|
|
||
| assertEquals(0, actualStudentsData.getStudents().size()); | ||
| verify(mockLogic, never()).getStudentsByTeamName(null, stubCourse.getId()); | ||
| verify(mockLogic, times(1)).getStudentsForCourse(stubCourse.getId()); | ||
| } |
There was a problem hiding this comment.
Similarly I think this should fail at access control check as well
| void testExecute_instructorWithPermission_success() { | ||
| loginAsInstructor(stubInstructorWithAllPrivileges.getGoogleId()); | ||
| when(mockLogic.getInstructorByGoogleId(stubCourse.getId(), stubInstructorWithAllPrivileges.getGoogleId())) | ||
| .thenReturn(stubInstructorWithAllPrivileges); | ||
| when(mockLogic.getStudentsForCourse(stubCourse.getId())).thenReturn(stubStudentListTwo); | ||
|
|
There was a problem hiding this comment.
Lets create 1 more stubSection for this test case
Since the difference between this test case and testExecute_instructorWithSameSectionPrivilegesAsStudents_success is the filtering of students by section
We can check that for this test cases students in all sections are returned and for the other test only students in the section with permission
| @Test | ||
| void testExecute_adminWithTeamNameParams_success() { | ||
| loginAsAdmin(); | ||
| when(mockLogic.getStudentsByTeamName(stubStudentOne.getTeam().getName(), | ||
| stubStudentOne.getCourse().getId())).thenReturn(stubStudentListOne); | ||
|
|
||
| String[] params = { | ||
| Const.ParamsNames.COURSE_ID, stubStudentOne.getCourse().getId(), | ||
| Const.ParamsNames.TEAM_NAME, stubStudentOne.getTeam().getName(), | ||
| }; | ||
| GetStudentsAction action = getAction(params); | ||
| JsonResult jsonResult = getJsonResult(action); | ||
| StudentsData actualStudentsData = (StudentsData) jsonResult.getOutput(); | ||
|
|
||
| verifyStudentsData(stubStudentListOne, actualStudentsData, Type.STUDENT); | ||
| verify(mockLogic, times(1)) | ||
| .getStudentsByTeamName(stubStudentOne.getTeamName(), stubCourse.getId()); | ||
| verify(mockLogic, never()).getStudentsForCourse(stubCourse.getId()); | ||
| } | ||
|
|
||
| @Test | ||
| void testExecute_unregisteredLoggedInUser_success() { | ||
| loginAsUnregistered("id"); | ||
| when(mockLogic.getStudentsByTeamName(stubStudentOne.getTeam().getName(), | ||
| stubStudentOne.getCourse().getId())).thenReturn(stubStudentListOne); | ||
|
|
||
| String[] params = { | ||
| Const.ParamsNames.COURSE_ID, stubStudentOne.getCourse().getId(), | ||
| Const.ParamsNames.TEAM_NAME, stubStudentOne.getTeam().getName(), | ||
| }; | ||
| GetStudentsAction action = getAction(params); | ||
| JsonResult jsonResult = getJsonResult(action); | ||
| StudentsData actualStudentsData = (StudentsData) jsonResult.getOutput(); | ||
|
|
||
| verifyStudentsData(stubStudentListOne, actualStudentsData, Type.STUDENT); | ||
| verify(mockLogic, times(1)) | ||
| .getStudentsByTeamName(stubStudentOne.getTeamName(), stubCourse.getId()); | ||
| verify(mockLogic, never()).getStudentsForCourse(stubCourse.getId()); | ||
| } |
There was a problem hiding this comment.
Something I just noticed for testExecute_.... we dont have to do loginAs... as the permissions are not checked
This means we can convert these to be tested under testSpecificAccessControl instead
Oh, @dishenggg, so sorry I forgot to push the changes. Will puhs the updated changes in abit |
Part of #12048
Outline of Solution
Change GetStudentsActionTest.java to ensure compatibility with the PostgreSQL database following the database migration.