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.

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

[#12048] Migrate tests for GetAccountActionTest #13212

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mingyang143
Copy link
Contributor

@mingyang143 mingyang143 commented Feb 3, 2025

Part of #12048

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

@mingyang143 mingyang143 force-pushed the branch-migrate-tests-week4 branch from e68354c to 8a00cea Compare February 8, 2025 04:56
Copy link

@DhiraPT DhiraPT left a comment

Choose a reason for hiding this comment

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

Overall, the changes look good! The implementation is clear, and the tests cover the main functionality well. Consider adding a few additional tests to improve coverage and refining the test method name for better clarity. Great work so far!

Copy link

Choose a reason for hiding this comment

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

  1. Adding a Test for Missing Parameters
    The execute() method in GetAccountAction requires non-null Instructor ID parameter. Would it be helpful to add a test case to ensure the method handles missing parameters correctly? For example:

    @Test
    void testExecute_noParameters_throwsInvalidHttpParameterException() {
        verifyHttpParameterFailure();
    }
    
  2. Access Control Test Cases
    Since GetAccountAction extends AdminOnlyAction, do you think it would be beneficial to include tests that verify access control? This can ensure only admins have access to the action. Some possible test cases could be:

    @Test
    void testSpecificAccessControl_admin_canAccess() { ... }
    
    @Test
    void testSpecificAccessControl_instructor_cannotAccess() { ... }
    
    @Test
    void testSpecificAccessControl_student_cannotAccess() { ... }
    
    @Test
    void testSpecificAccessControl_loggedOut_cannotAccess() { ... }
    

}

@Test
void testExecute_accountDoesNotExist_failSilently() {
Copy link

Choose a reason for hiding this comment

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

The current method name testExecute_accountDoesNotExist_failSilently() might not fully reflect the behavior, as the method throws an exception rather than failing silently. Would something like testExecute_accountDoesNotExist_throwsEntityNotFoundException() be clearer?

Copy link
Contributor

Choose a reason for hiding this comment

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

good catch!

@dishenggg dishenggg added the s.Ongoing The PR is being worked on by the author(s) label Feb 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s.Ongoing The PR is being worked on by the author(s)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants