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

(test) O3-2216: Add tests for id-field.component.tsx #748

Merged
merged 6 commits into from
Sep 24, 2023
Merged

(test) O3-2216: Add tests for id-field.component.tsx #748

merged 6 commits into from
Sep 24, 2023

Conversation

ayush-AI
Copy link
Contributor

Requirements

Summary

In this PR i have added test for id-field component.

Screenshots

Screenshot 2023-06-30 at 11 51 16 PM

Related Issue

https://issues.openmrs.org/browse/O3-2216

Other

None.

@ayush-AI
Copy link
Contributor Author

cc/ @jayasanka-sack @anjula-sack @Piumal1999

Piumal1999
Piumal1999 requested changes Jul 3, 2023
View reviewed changes
Copy link
Collaborator

@Piumal1999 Piumal1999 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Good job @ayush-AI. I requested some minor changes.

Btw, there's a formatting pattern called Arrange-Act-Assert, which is used to improve the readability of tests. You can follow it when arranging the steps of the tests (if it suits our test cases).

packages/esm-patient-registration-app/src/patient-registration/field/id/id-field.test.tsx Outdated Show resolved Hide resolved
packages/esm-patient-registration-app/src/patient-registration/field/id/id-field.test.tsx Outdated Show resolved Hide resolved
packages/esm-patient-registration-app/src/patient-registration/field/id/id-field.test.tsx Outdated
Comment on lines 162 to 163
expect(screen.getByText('Configure')).toBeInTheDocument();
expect(screen.getByText('Configure')).toBeEnabled();
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this is related to a Button, we can use getByRole('button', { name: 'Configure' }). We need to make sure that we are querying the correct element.

(Use getByRole() whenever it is possible.)

packages/esm-patient-registration-app/src/patient-registration/field/id/id-field.test.tsx Outdated Show resolved Hide resolved
@ayush-AI ayush-AI requested a review from Piumal1999 July 5, 2023 07:56
@ayush-AI ayush-AI requested a review from Piumal1999 July 13, 2023 13:32
Piumal1999
Piumal1999 previously requested changes Aug 18, 2023
View reviewed changes
...m-patient-registration-app/src/patient-registration/field/__mocks__/identifier-types.mock.ts Outdated Show resolved Hide resolved
packages/esm-patient-registration-app/src/patient-registration/field/id/id-field.test.tsx
initialFormValues: { identifiers: { openmrsID } },
setInitialFormValues: jest.fn(),
values: {
identifiers: { openmrsID },
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
identifiers: { openmrsID },
identifiers: { mockedIdentifierTypes[0] },

I meant some thing like this. Check my comment on identifier-types.mock.ts as well. If it is not working, let's keep this as it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated it as per your suggestions @Piumal1999. Please check

Copy link
Collaborator

Choose a reason for hiding this comment

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

The requested change is not added. Could you please recheck? @ayush-AI

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not working like the way you are suggesting. The test is failing because of syntax error

@jayasanka-sack
Copy link
Member

@ayush-AI could you please check @Piumal1999 's comments?

@ayush-AI
Copy link
Contributor Author

ayush-AI commented Sep 18, 2023 •
edited
Loading

@ayush-AI could you please check @Piumal1999 's comments?

I checked it and it is not working as piumal suggested to try. That is why I have to keep that file.

cc:/ @Piumal1999

@denniskigen
Copy link
Member

Can we move this along @ayush-AI @Piumal1999?

@ayush-AI
Copy link
Contributor Author

ayush-AI commented Sep 21, 2023 •
edited
Loading

yup we can @denniskigen. If other things seems okay to you.

@denniskigen denniskigen merged commit 1304925 into openmrs:main Sep 24, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Piumal1999 Piumal1999 Piumal1999 left review comments

@denniskigen denniskigen denniskigen approved these changes

@jayasanka-sack jayasanka-sack Awaiting requested review from jayasanka-sack

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ayush-AI @jayasanka-sack @denniskigen @Piumal1999