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.

Sign up for GitHub

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

Jump to bottom

[#12048] Migrate tests for DeleteAccountActionTest #13200

Merged
merged 4 commits into from
Feb 10, 2025
Merged

[#12048] Migrate tests for DeleteAccountActionTest #13200

merged 4 commits into from
Feb 10, 2025

Conversation

mingyang143
Copy link
Contributor

Part of #12048

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

@github-actions GitHub Actions
Copy link

Hi @mingyang143, thank you for your interest in contributing to TEAMMATES!
However, your PR does not appear to follow our contribution guidelines:

  • Title must start with the issue number the PR is fixing in square brackets, e.g. [#<issue-number>]

Please address the above before we proceed to review your PR.

@mingyang143 mingyang143 changed the title Migrate tests for DeleteAccountActionTest [#12048] Migrate tests for DeleteAccountActionTest Jan 22, 2025
dishenggg
dishenggg requested changes Feb 6, 2025
View reviewed changes
Copy link
Contributor

@dishenggg dishenggg left a comment

Choose a reason for hiding this comment

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

Sorry for the late review, nice work so far!

I think in all of your PRs for the tests migration, it includes your changes for #13195. You can try rebasing your branches, keeping only the commits for the migration.

src/test/java/teammates/sqlui/webapi/DeleteAccountActionTest.java Outdated
Comment on lines 41 to 52
Course stubCourse = new Course("course-id", "name", Const.DEFAULT_TIME_ZONE, "institute");
Account stubAccount = new Account(googleId, "name", "[email protected]");
Instructor instructor = new Instructor(stubCourse, "name", "[email protected]",
false, "", null, new InstructorPrivileges());
instructor.setAccount(stubAccount);
String[] params = {
Const.ParamsNames.INSTRUCTOR_ID, instructor.getGoogleId(),
};
DeleteAccountAction action = getAction(params);
MessageOutput actionOutput = (MessageOutput) getJsonResult(action).getOutput();
assertEquals("Account is successfully deleted.", actionOutput.getMessage());
assertNull(mockLogic.getInstructorByGoogleId(stubCourse.getId(), instructor.getGoogleId()));
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that because the actual implementation of the action is so simple(we dont need the logic layer to return us any value etc) and we only need to test for SQL related code we can ignore the datastore method call.

Verifying that sqlLogic.deleteAccountCascade(googleId); is called once with the correct argument and the json output is correct should be enough.

One idea is to verify(mockLogic, times(1)).deleteAccountCascade(googleId) provided by Mockito this helps to ensure that the method is called and the INSTRUCTOR_ID from the request params is passed properly as an argument to the method.

Copy link
Contributor

Choose a reason for hiding this comment

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

I just realized that since the treatment for both existing and non existing account is the same maybe we can just test this 2 cases:

  1. Null request params
  2. Non-Null request params

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm working on this now and will update soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated the tests. Let me know if further fixes are needed, thanks!

@dishenggg dishenggg added the s.Ongoing The PR is being worked on by the author(s) label Feb 6, 2025
@mingyang143
Copy link
Contributor Author

mingyang143 commented Feb 6, 2025 •
edited
Loading

Sorry for the late review, nice work so far!

I think in all of your PRs for the tests migration, it includes your changes for #13195. You can try rebasing your branches, keeping only the commits for the migration.

Hi I have rebased this branch already.

dishenggg
dishenggg approved these changes Feb 9, 2025
View reviewed changes
Copy link
Contributor

@dishenggg dishenggg left a comment

Choose a reason for hiding this comment

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

Nice work! LGTM

@dishenggg dishenggg added s.FinalReview The PR is ready for final review and removed s.Ongoing The PR is being worked on by the author(s) labels Feb 9, 2025
dishenggg
dishenggg reviewed Feb 9, 2025
View reviewed changes
src/test/java/teammates/sqlui/webapi/DeleteAccountActionTest.java Outdated
}

@Test
protected void textExecute_nullParams_failSilently() {
Copy link
Contributor

Choose a reason for hiding this comment

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

one small nit here, maybe we can update the name of this test since verifyHttpParameterFailure actually checks if an exception is thrown

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the PR review! I've made the updates as requested.

domoberzin
domoberzin approved these changes Feb 10, 2025
View reviewed changes
Copy link
Contributor

@domoberzin domoberzin left a comment

Choose a reason for hiding this comment

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

LGTM, good work!

@domoberzin domoberzin merged commit 8005a6a into TEAMMATES:master Feb 10, 2025
11 checks passed
Hunter104 added a commit to Hunter104/teammates that referenced this pull request Feb 11, 2025
@Hunter104
Merge pull request #1 from TEAMMATES/master
a47e572 
[TEAMMATES#12048] Migrate tests for DeleteAccountActionTest (TEAMMATES#13200)
mingyang143 added a commit to mingyang143/teammates that referenced this pull request Feb 15, 2025
@mingyang143
[TEAMMATES#12048] Migrate tests for DeleteAccountActionTest (TEAMMATE…
b80274b 
…S#13200)

* Migrate DeleteAccountActionTest

* Fix checkstyle

* Modify tests in DeleteAccountActionTest.java

* Update method name in DeleteAccountActionTest
domoberzin added a commit that referenced this pull request Mar 12, 2025
@mingyang143 @BunnyHoppp
@jasonqiu212 @domoberzin
[#12048] Migrate tests for GetCoursesActionTest (#13232)
4bbd103 
* Migrate tests for GetCoursesActionTest.java

* [#12048] Migrate tests for DeleteAccountActionTest (#13200)

* Migrate DeleteAccountActionTest

* Fix checkstyle

* Modify tests in DeleteAccountActionTest.java

* Update method name in DeleteAccountActionTest

* [#12048] Migrate tests for DeleteFeedbackQuestionActionTest (#13218)

* Migrate tests for GetCoursesActionTest

* Update PR for GetCoursesActionTest

* Update PR for GetCoursesActionTest

* Update PR for GetCoursesActionTest

---------

Co-authored-by: Wei Loon <[email protected]>
Co-authored-by: Jason Qiu <[email protected]>
Co-authored-by: domoberzin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dishenggg dishenggg dishenggg approved these changes

@domoberzin domoberzin domoberzin approved these changes

Assignees

@mingyang143 mingyang143

Labels
s.FinalReview The PR is ready for final review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mingyang143 @dishenggg @domoberzin