Skip to content

(test) O3-3939: Add test to repeat component#385

Draft
WodPachua wants to merge 6 commits intoopenmrs:mainfrom
WodPachua:repeat-tests
Draft

(test) O3-3939: Add test to repeat component#385
WodPachua wants to merge 6 commits intoopenmrs:mainfrom
WodPachua:repeat-tests

Conversation

@WodPachua
Copy link
Contributor

@WodPachua WodPachua commented Sep 10, 2024

Requirements

  • This PR has a title that briefly describes the work done including the ticket number. If there is a ticket, make sure your PR title includes a conventional commit label. See existing PR titles for inspiration.
  • My work conforms to the OpenMRS 3.0 Styleguide and design documentation.
  • My work includes tests or is validated by existing tests.

Summary

Added test to the repeat component in the form engine

Screenshots

Related Issue

O3-3939

Other

@WodPachua WodPachua marked this pull request as draft September 10, 2024 06:46
@WodPachua WodPachua changed the title (test) add test to repeat component (test) Add test to repeat component Sep 10, 2024
@WodPachua WodPachua changed the title (test) Add test to repeat component (test) O3-3939: Add test to repeat component Sep 10, 2024
@samuelmale
Copy link
Member

@WodPachua are you still working on this?

@WodPachua
Copy link
Contributor Author

Was trying to add another/more tests... had made the draft for your review if am on the right track

@WodPachua WodPachua marked this pull request as ready for review September 18, 2024 12:22
@WodPachua WodPachua marked this pull request as draft September 18, 2024 13:21
@WodPachua WodPachua marked this pull request as ready for review September 24, 2024 19:24
Comment on lines +30 to +36
it('Should update field ID in expressions when adding repeated fields', () => {
const expression = "infantStatus !== 'someValue'";
const fieldIds = ['birthDate', 'infantStatus', 'deathDate'];
const updatedExpression = updateFieldIdInExpression(expression, 2, fieldIds);

expect(updatedExpression).toEqual("infantStatus_2 !== 'someValue'");
});
Copy link
Member

Choose a reason for hiding this comment

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

@WodPachua I like the test but the scope of work begs for testing the repeat component itself and not just the helpers. You want to render a form with a "repeatable" field, and then test out things like:

  1. Asserting that clicking the "Add" button clones the field at origin
  2. Test out submission (Assert that both the origin and its instances' values are submitted successfully)
  3. Edit mode (Assert that a repeat field with instances is properly initialised (hydrated) as expected)

@samuelmale
Copy link
Member

@WodPachua are you still working on this?

@WodPachua WodPachua marked this pull request as draft October 14, 2024 22:52
@WodPachua
Copy link
Contributor Author

@WodPachua are you still working on this?
Yes, any comments on the tests added so far? Is this the right approach? @samuelmale

@WodPachua
Copy link
Contributor Author

Have already noted the changes proposed on slack

@samuelmale
Copy link
Member

@WodPachua you probably want to do something like this.

Copy link
Member

@samuelmale samuelmale left a comment

Choose a reason for hiding this comment

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

@WodPachua sorry for the delayed review.

import React, { act } from 'react';

describe('RepeatingFieldComponent - handleExpressionFieldIdUpdate', () => {
it('Should handle update of expression with ids in repeat group', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
it('Should handle update of expression with ids in repeat group', () => {
it('Should update field IDs when repeat index is greater than 1', () => {

);
});

it('Should handle update of expression with ids not in repeat group', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
it('Should handle update of expression with ids not in repeat group', () => {
it('Should not update field IDs when repeat index equals 1', () => {

const mockUsePatient = jest.mocked(usePatient);
const mockUseSession = jest.mocked(useSession);

global.ResizeObserver = require('resize-observer-polyfill');
Copy link
Member

Choose a reason for hiding this comment

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

This is currently configured here.

};

beforeEach(() => {
Object.defineProperty(window, 'i18next', {
Copy link
Member

Choose a reason for hiding this comment

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

This is currently configured here.

formJson={repeatingComponentTestForm as FormSchema}
patientUUID="8673ee4f-e2ab-4077-ba55-4980f408773e"
mode={mode}
encounterUUID={mode === 'edit' ? 'a8817ad2-ef92-46c8-bbf7-db336505027c' : null}
Copy link
Member

Choose a reason for hiding this comment

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

@WodPachua, Do you have any test cases covering the "edit mode"? If not, can we define some?

Comment on lines +127 to +128
const clonedField = screen.getByLabelText(/Contact relationship/i);
expect(clonedField).toBeInTheDocument();
Copy link
Member

Choose a reason for hiding this comment

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

Can you confirm that there are two fields with the label "Contact relationship" that never existed on the initial render?

Comment on lines +145 to +149
await waitFor(() => {
expect(mockContext.methods.getValues).toHaveBeenCalledWith(expect.objectContaining({
patientContactRelationship: 'Child',
phoneNumber: '123456789',
}));
Copy link
Member

Choose a reason for hiding this comment

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

Can you also provide values for the instances so that we end up with a submission like:

     {
        patientContactRelationship: 'Child',
        patientContactRelationship_2: 'foo',
        phoneNumber: '123456789',
        phoneNumber_2: 'bar'
      }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants