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

Adding Integration Tests to Person #916

Open
wants to merge 19 commits into
base: develop
Choose a base branch
from

Conversation

sayed4900
Copy link
Contributor

Add tests for updating and deleting a person

image

sayed4900 and others added 7 commits March 24, 2024 15:27
…ntact' into integration-test-organization-contact

Merge remote-tracking branch 'origin/integration-test-organization-contact' into integration-test-organization-contact

This merge incorporates changes from the remote repository into the local branch to synchronize our work with the latest updates.
@sayed4900
Copy link
Contributor Author

@mozzy11 can you review this PR?

@sayed4900 sayed4900 changed the title Integration tests person Adding Integration Tests to Person Mar 27, 2024
Copy link
Contributor

@jona42-ui jona42-ui left a comment

Choose a reason for hiding this comment

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

You're using both Assert class and static assertion methods. for consistency, you need to stick to one style.
@mozzy11 can you see if this is something to take note of though it looks more readable

@jona42-ui
Copy link
Contributor

jona42-ui commented Mar 27, 2024

Also, link to a corresponding issue

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotEquals;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertNull;
Copy link
Contributor

@jona42-ui jona42-ui Mar 27, 2024

Choose a reason for hiding this comment

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

can we stick to one style for consistency? you are using both Assert class and static assertion methods

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 your review, I will edit it now.

deletedPerson = personService.get(personId);
} catch (ObjectNotFoundException e) {

}
Copy link
Contributor

@jona42-ui jona42-ui Mar 27, 2024

Choose a reason for hiding this comment

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

you're not doing anything with this catch,
catching ObjectNotFoundException

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you're not doing anything with this catch, catching ObjectNotFoundException

can you check my last commit and give me feedback and thanks for your review.

@sayed4900
Copy link
Contributor Author

Also, link to a corresponding issue

should I open issue?

@mozzy11
Copy link
Collaborator

mozzy11 commented Mar 27, 2024

can you configure your IDE with java formatter to properly format your code ??

@sayed4900
Copy link
Contributor Author

I am trying to test function getPersonById', I pass to it personId` but I am get this error
Parameter value [2] did not match expected type [java.lang.String (n/a)]

image

can the problem comes from getPersonById Implementation ?
@mozzy11

@sayed4900
Copy link
Contributor Author

sayed4900 commented Mar 29, 2024

@mozzy11 I adding more tests, can you review them?
image

@valens200
Copy link

Also, link to a corresponding issue

should I open issue?

Yes @sayed4900 but it's always a good practice to open an issue before start working on it.

@sayed4900
Copy link
Contributor Author

Also, link to a corresponding issue

should I open issue?

Yes @sayed4900 but it's always a good practice to open an issue before start working on it.

Thanks @valens200
I will keep that point in mind next time.

@sayed4900
Copy link
Contributor Author

sayed4900 commented Apr 17, 2024

Hello @mozzy11

When I test this function it's always failed
image

To resolve this issue, I modified the method to accept the personId parameter as a String directly, like so: query.setParameter("personId", personId).
image
image

@mozzy11 mozzy11 changed the base branch from develop_3x to develop May 13, 2024 07:42
@mozzy11
Copy link
Collaborator

mozzy11 commented May 13, 2024

Thanks @sayed4900 .
Can you fix the merge conflicts ??

@sayed4900
Copy link
Contributor Author

Thanks @sayed4900 . Can you fix the merge conflicts ??

How can I solve the conflicts?
Sorry for the delay in responding, excuse me.
@mozzy11

Copy link

👋 Hi, @sayed4900,
Conflicts have been detected against the base branch. Please rebase your branch against the base branch.


This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there.

@github-actions github-actions bot added the merge conflict Merge Conflicts label May 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge conflict Merge Conflicts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants