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 DeleteStudentActionTest #13204

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

Conversation

DhiraPT
Copy link

@DhiraPT DhiraPT commented Jan 26, 2025

Part of #12048

Outline of Solution
Migrated DeleteStudentActionTest to use SQL-based logic instead of the previous Datastore model.

Copy link

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

  • Description must reference the issue number the PR is fixing, e.g. Fixes #<issue-number> (or Part of #<issue-number> if the PR does not address the issue fully)

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

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! Sorry for late review just a few small changes

DeleteStudentAction action = getAction(params);
MessageOutput actionOutput = (MessageOutput) getJsonResult(action).getOutput();

verify(mockLogic, times(0)).deleteStudentCascade(course.getId(), student.getEmail());
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 expanding the arguments that this verify checks for would be good, since in this case the deleteStudentCascade method should not be called at all, for example using

verify(mockLogic, never()).deleteStudentCascade(any(), any());

would ensure that the method is never called at all

DeleteStudentAction action = getAction(params);
MessageOutput actionOutput = (MessageOutput) getJsonResult(action).getOutput();

verify(mockLogic, times(0)).deleteStudentCascade(course.getId(), student.getEmail());
Copy link
Contributor

Choose a reason for hiding this comment

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

For nonExistentStudentId and courseDoesNotExist, since deleteStudentCascade is actually called with some input.

I think that it might be better to verify that the method is called only once with "RANDOM_STUDENT" and verify that the method is never called again or with no other arguments

small nit: while we are here lets choose one of nonExistent... or ...DoesNotExist

@dishenggg dishenggg added the s.Ongoing The PR is being worked on by the author(s) label Feb 13, 2025
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.

Seems like I was a bit confused with when the logic is called, but you got the idea!

Just one more minor change


verify(mockLogic, never()).getStudentByGoogleId(any(), any());
verify(mockLogic, times(1)).deleteStudentCascade(course.getId(), "RANDOM_EMAIL");
verify(mockLogic, never()).deleteStudentCascade(course.getId(), student.getEmail());
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 setting this to

verify(mockLogic, times(1)).deleteStudentCascade(any(), any());

might be better here to ensure that the action doesnt delete some random student by accident because chances are if something does go wrong it might call the method with something else than the email you defined in BeforeMethod

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.

2 participants