[#12048] Migrate tests for GetSessionResultsActionTest #13216

mingyang143 · 2025-02-04T16:17:31Z

Part of #12048

Outline of Solution
Change GetSessionResultsActionTest.java to ensure compatibility with the PostgreSQL database following the database migration.

InfinityTwo · 2025-02-10T04:45:44Z

src/test/java/teammates/sqlui/webapi/GetSessionResultsActionTest.java

+                FeedbackParticipantType.INSTRUCTORS, FeedbackParticipantType.OWN_TEAM, 0,
+                new ArrayList<>(), new ArrayList<>(), new ArrayList<>(), new FeedbackMcqQuestionDetails());
+        questionsStub.add(questionStub);
+        SqlSessionResultsBundle resultsStub = new SqlSessionResultsBundle(questionsStub,


Great work, but perhaps you could abstract the instantiation of resultsStub to a different function as you are using it more than once?

InfinityTwo · 2025-02-10T04:48:28Z

src/test/java/teammates/sqlui/webapi/GetSessionResultsActionTest.java

+
+    @Test
+    void testCheckSpecificAccessControl_studentResultIntentUnpublishedSession_cannotAccess() {
+        loginAsStudent(googleId);


Great coverage for test cases but perhaps you could abstract out the first few lines that are similar throughout each method into before each?

dishenggg

Thanks for the PR!, some small changes and most of them are just changing to getTypicalXXX which should help to simply the code since we need a lot of different objects for this test and

I think we should add 2 more access control cases:

Logged in as instructor with PREVIEWAS, can access
Logged in as student with PREVIEWAS, cannot access

dishenggg · 2025-02-11T06:44:59Z

src/test/java/teammates/sqlui/webapi/GetSessionResultsActionTest.java

+    String googleId = "user-googleId";
+    Course course;
+
+    FeedbackSession session;


Lets private these for consistency

dishenggg · 2025-02-19T16:28:18Z

src/test/java/teammates/sqlui/webapi/GetSessionResultsActionTest.java

+        when(mockLogic.getSessionResultsForCourse(any(FeedbackSession.class), any(String.class), any(String.class),
+                any(UUID.class), any(String.class), any(FeedbackResultFetchType.class))).thenReturn(resultsStub);


I think it would be better if we try to narrow down the allowed args instead of using any to ensure that the params are parsed and passed to the method properly or, we can add a verify statement as well

Lets try and do this everywhere possible eg in prepareMocksBasicParams too

dishenggg · 2025-02-19T16:59:15Z

src/test/java/teammates/sqlui/webapi/GetSessionResultsActionTest.java

+        List<FeedbackQuestion> questionsStub = new ArrayList<>();
+        questionsStub.add(new FeedbackMcqQuestion(session, 1, "description",
+                FeedbackParticipantType.INSTRUCTORS, FeedbackParticipantType.OWN_TEAM, 0,
+                new ArrayList<>(), new ArrayList<>(), new ArrayList<>(), new FeedbackMcqQuestionDetails()));
+        SqlSessionResultsBundle resultsStub = new SqlSessionResultsBundle(questionsStub,
+                new HashSet<>(), new HashSet<>(), new ArrayList<>(),
+                new ArrayList<>(), new HashMap<>(), new HashMap<>(),
+                new HashMap<>(), new HashMap<>(), new SqlCourseRoster(new ArrayList<>(), new ArrayList<>()));


Lets use the getTypicalXXX methods for this so our mock data is more realistic

dishenggg · 2025-02-19T17:02:12Z

src/test/java/teammates/sqlui/webapi/GetSessionResultsActionTest.java

+        FeedbackSession unpublishedSession = new FeedbackSession(
+                "session-name",
+                course,
+                "[email protected]",
+                null,
+                Instant.parse("2020-01-01T00:00:00.000Z"),
+                Instant.parse("2020-10-01T00:00:00.000Z"),
+                Instant.parse("2020-01-01T00:00:00.000Z"),
+                Instant.MAX,
+                null,
+                false,
+                false,
+                false);


Lets use the getTypicalFeedbackSessionForCourse method for this and change the publish time with the setter after

dishenggg · 2025-02-19T17:02:39Z

src/test/java/teammates/sqlui/webapi/GetSessionResultsActionTest.java

+        FeedbackSession unpublishedSession = new FeedbackSession(
+                "session-name",
+                course,
+                "[email protected]",
+                null,
+                Instant.parse("2020-01-01T00:00:00.000Z"),
+                Instant.parse("2020-10-01T00:00:00.000Z"),
+                Instant.parse("2020-01-01T00:00:00.000Z"),
+                Instant.MAX,
+                null,
+                false,
+                false,
+                false);


Lets use the getTypicalFeedbackSessionForCourse method here too

dishenggg · 2025-02-19T17:12:12Z

src/test/java/teammates/sqlui/webapi/GetSessionResultsActionTest.java

+                Const.ParamsNames.FEEDBACK_SESSION_NAME, session.getName(),
+                Const.ParamsNames.COURSE_ID, session.getCourseId(),
+                Const.ParamsNames.INTENT, Intent.STUDENT_RESULT.name(),
+                Const.ParamsNames.PREVIEWAS, student.getEmail(),


Lets remove the PREVIEWAS param from here and test it separately

dishenggg · 2025-02-19T17:12:16Z

src/test/java/teammates/sqlui/webapi/GetSessionResultsActionTest.java

+                Const.ParamsNames.FEEDBACK_SESSION_NAME, session.getName(),
+                Const.ParamsNames.COURSE_ID, session.getCourseId(),
+                Const.ParamsNames.INTENT, FULL_DETAIL.name(),
+                Const.ParamsNames.PREVIEWAS, instructor.getEmail(),


Lets remove the PREVIEWAS param from here and test it separately

dishenggg · 2025-02-19T17:12:19Z

src/test/java/teammates/sqlui/webapi/GetSessionResultsActionTest.java

+                Const.ParamsNames.FEEDBACK_SESSION_NAME, session.getName(),
+                Const.ParamsNames.COURSE_ID, session.getCourseId(),
+                Const.ParamsNames.INTENT, Intent.INSTRUCTOR_RESULT.name(),
+                Const.ParamsNames.PREVIEWAS, instructor.getEmail(),


Lets remove the PREVIEWAS param from here and test it separately

dishenggg · 2025-02-19T17:16:04Z

src/test/java/teammates/sqlui/webapi/GetSessionResultsActionTest.java

+    void testCheckSpecificAccessControl_nullInstructor_cannotAccess() {
+        loginAsInstructor(googleId);
+        String[] params = {
+                Const.ParamsNames.FEEDBACK_SESSION_NAME, session.getName(),
+                Const.ParamsNames.COURSE_ID, session.getCourseId(),
+                Const.ParamsNames.INTENT, Intent.INSTRUCTOR_RESULT.name(),
+        };
+        when(mockLogic.getFeedbackSession(session.getName(), session.getCourseId())).thenReturn(session);
+        when(mockLogic.getInstructorByGoogleId(session.getCourseId(), googleId)).thenReturn(null);
+        verifyCannotAccess(params);
+    }


Maybe I am missing something but this test case feels a bit off when we are logged in as an Instructor and yet getInstructorByGoogleId returns null, maybe we can replace this with Instructor from another course? or/and Instructor without permissions

dishenggg · 2025-02-19T17:24:29Z

src/test/java/teammates/sqlui/webapi/GetSessionResultsActionTest.java

+        assertEquals(resultsStub.getQuestions().size(), output.getQuestions().size());
+        assertEquals(resultsStub.getQuestions().get(0).getDescription(),
+                output.getQuestions().get(0).getFeedbackQuestion().getQuestionDescription());


Hmmm, cant quite figure out if this 2 asserts are enough maybe you or anyone else can share some insights on this?

lets follow the old test case and go and do something similar to checking if 2 objects are equal better to be safe then sorry

mingyang143 added 3 commits February 8, 2025 13:04

Migrate tests for GetSessionResultsActionTest.java

6f7bacf

Migrate tests for GetSessionResultsActionTest.java

1d0d62f

Migrate tests for GetSessionResultsActionTest.java

199f09c

mingyang143 force-pushed the branch-migrate-tests-week-4-2 branch from 47b048c to 199f09c Compare February 8, 2025 05:06

domoberzin requested review from dishenggg and yuanxi1 February 10, 2025 04:43

InfinityTwo reviewed Feb 10, 2025

View reviewed changes

Merge branch 'master' into branch-migrate-tests-week-4-2

e27d97b

dishenggg assigned mingyang143 Feb 19, 2025

dishenggg requested changes Feb 19, 2025

View reviewed changes

dishenggg added the s.Ongoing The PR is being worked on by the author(s) label Feb 19, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#12048] Migrate tests for GetSessionResultsActionTest #13216

[#12048] Migrate tests for GetSessionResultsActionTest #13216

mingyang143 commented Feb 4, 2025

InfinityTwo Feb 10, 2025

InfinityTwo Feb 10, 2025

dishenggg left a comment

dishenggg Feb 11, 2025

dishenggg Feb 19, 2025

dishenggg Feb 19, 2025

dishenggg Feb 19, 2025

dishenggg Feb 19, 2025

dishenggg Feb 19, 2025

dishenggg Feb 19, 2025

dishenggg Feb 19, 2025

dishenggg Feb 19, 2025

dishenggg Feb 19, 2025 •

edited

Loading

dishenggg Feb 24, 2025

		when(mockLogic.getSessionResultsForCourse(any(FeedbackSession.class), any(String.class), any(String.class),
		any(UUID.class), any(String.class), any(FeedbackResultFetchType.class))).thenReturn(resultsStub);

[#12048] Migrate tests for GetSessionResultsActionTest #13216

Are you sure you want to change the base?

[#12048] Migrate tests for GetSessionResultsActionTest #13216

Conversation

mingyang143 commented Feb 4, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dishenggg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dishenggg Feb 19, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dishenggg Feb 19, 2025 •

edited

Loading