[#13304] Refactoring for Unit Test Access Controls #13385
Conversation
…CourseJoinEmailWorkerActionTest, and add report md
remove this duplicate record
- Remove trailing whitespaces from PublishFeedbackSessionActionTest.java (lines 72, 82, 91) - Remove trailing whitespaces from QueryLogsActionTest.java (line 165) - Fixes Checkstyle RegexpSinglelineJava violations
Add missing mock for getFeedbackSession to fix EntityNotFoundException. The convenience method requires this mock to be set up for access control verification.
|
Hi, @InfinityTwo could you kindly review this pr? I checked previous pr #13359 and it seems the E2E test error isn't relevant with refactoring. |
|
@Ederich013 Thanks for your contribution. However, the error was indeed caused by a refactoring, not by the one you mentioned, but another PR that has been merged (#13357) At the moment, even if I were to review your PR, I can't merge it since it will still failing checks. However, I'll keep your PR in mind and review it onces the other refactor backlogs have been merged successfully in case of any cascading errors from other merges. |
|
@Ederich013 Alternatively, you can consider creating a fix based on your report that you made with another PR on the failing checks. If you can successfully fix the issue, I can review it and merge all your PRs |
InfinityTwo
left a comment
InfinityTwo
left a comment
There was a problem hiding this comment.
Most of the tests LGTM! I have a few comments, but once those are resolved and the E2E issue fixes are merged, I think this is good to go
src/test/java/teammates/sqlui/webapi/PublishFeedbackSessionActionTest.java
Show resolved
Hide resolved
| errorLogTimestamp2, errorLogTextPayload2, errorLogJsonPayLoad2); | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
The other test files don't have the large chunk of comments here (also ditto on your other files with these large chunks of comments). I see your view in making the code easier to understand, and it's great, but perhaps, would it be better if we could keep the standardisation? If you think comments would help, what do you think of adding it inside BaseActionTest.java instead?
There was a problem hiding this comment.
I think you might have accidentally added this file. In the case where I'm wrong, what do you think of documenting it elsewhere instead?
There was a problem hiding this comment.
Yeah it shoud be removed. Will fix those issue after the final exams, likely after 10th Nov.
2 participants
Part of #13304
Refactored test cases GetUsageStatisticsActionTest.java to PublishFeedbackSessionActionTest.java (71-78) and StudentCourseJoinEmailWorkerActionTest.java(94).
Details recorded here:
Refactoring_Report.md