-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
test: migrate tests to React Testing Library #2516
test: migrate tests to React Testing Library #2516
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/hospitalrun/hospitalrun-frontend/lyv8m3nfi |
@marcosvega91 Just pinging you for reference. |
src/__tests__/page-header/button-toolbar/ButtonToolBar.test.tsx
Outdated
Show resolved
Hide resolved
Refactor to remove querySelector calls where possible
…rsonmodal-integration Pull AddRelatedPersonModal tests out into integration tests
expect(descriptionInput.prop('isInvalid')).toBeTruthy() | ||
expect(descriptionInput.prop('feedback')).toEqual(error.description) | ||
}) | ||
// ! Remove test? Save button is always rendered regardless of input values |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am guessing the jury is still out on this one in terms of whether or not this is still needed? Any reason we might no longer want it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I left that comment (I use Better Comments and that bang at the start is something I do lol)
@nobrayner thoughts on this test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was me - I'll make an issue for it
wrapper.update() | ||
return { wrapper: wrapper as ReactWrapper, history } | ||
), | ||
} | ||
} | ||
|
||
it('should render a list of allergies', async () => { | ||
const expectedAllergies = [{ id: '456', name: 'some name' }] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a question on recommendation in terms of naming conventions. In some tests we use variables named as mockSomething
, in other cases expectedSomething
while other times we dont have a prefix so we just have something
as the variable name.
Do you have recommendations/best practice advice when it comes to naming variables in tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My suggestion is whatever makes sense. I generally stick with "mock" if I really want to differentiate if the variable or value is fake, however, I don't generally use expected mainly because the expect()
tells you what is being asserted/expected
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... I was probably the one putting out most of the expected*
.
The convention I follow is:
- If I am mocking something out, rename it to
mock*
- If it is data I am using for assertions, label it as
expected*
I also use expected*
even if it is the data being returned from a mock.
I've find this helps me reason with the code a lot easier, as it helps give expectation hints to me when I revisit later.
const patient = { | ||
id: 'patientId', | ||
allergies: [{ id: '123', name: 'some name' }], | ||
allergies: [{ id: '123', name: 'cats' }], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh how can someone be allergic to cute cats 😺😺
expect(descriptionInput.prop('label')).toEqual('patient.careGoal.description') | ||
expect(descriptionInput.prop('isRequired')).toBeTruthy() | ||
expect(descriptionInput.prop('value')).toBe(careGoal.description) | ||
// TODO: not using built-in form accessibility features yet: required attribute |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a comment here so that we don't forget this TODO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nobrayner @codyarose this might be a good start for issues
expect(dueDatePicker).toBeInTheDocument() | ||
expect(noteInput).toBeInTheDocument() | ||
|
||
// TODO: not using built-in form accessibility features yet: HTMLInputElement.setCustomValidity() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just adding this here so we dont forget the TODO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nobrayner @codyarose this might be a good start for issues
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome! Thanks everyone for their Diligent 'n Dedicated (DnD 😉) work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some commented out code and a todo which we can create an issue for. But otherwise looking good. Thank you so much for all the effort.
expect(intentInput).toHaveClass('is-invalid') | ||
// expect(intentInput.nextSibling).toHaveTextContent( | ||
// expectedError.intent, | ||
// ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May we remove commented out code
expect(visitSelectorLabel.title).not.toBe('This is a required input') | ||
}) | ||
|
||
// it.only('should call the on change handler when visit changes', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this one still a work in progress or to be removed?
@@ -25,16 +23,16 @@ const expectedLabs = [ | |||
rev: '1', | |||
patient: '1234', | |||
requestedOn: new Date(2020, 1, 1, 9, 0, 0, 0).toISOString(), | |||
requestedBy: 'someone', | |||
type: 'lab type', | |||
requestedBy: 'Dr Strange', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mmm i don't know if i want my labs being requested by Dr Strange LOL
@jackcmeyer @blestab I signed the CLA on behalf of the Raid and I can get a few more people to sign it however there is no way I can get all 25+ people to sign it. |
We will keep this PR open and ongoing for maintainers to track the progress in realtime as the branches get merged into the forked master.