Skip to content

Commit

Permalink
chore/5743-Setupi18nInUnitTests (#9702)
Browse files Browse the repository at this point in the history
  • Loading branch information
alexandec authored Oct 2, 2024
1 parent 4ef3f0b commit c16a60a
Show file tree
Hide file tree
Showing 4 changed files with 104 additions and 84 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -17,33 +17,61 @@ All React components should have at least one unit test. The ideal quantity of t
Note that while a high coverage percentage is good, it doesn't ensure tests are complete and correct. It's important to think critically and implement tests that cover the key cases a user might encounter.

### More information

- Google's [Code Coverage Best Practices](https://testing.googleblog.com/2020/08/code-coverage-best-practices.html)
- [How to know what to test](https://kentcdodds.com/blog/how-to-know-what-to-test) (Kent C Dodds)

## Targeting by rendered text, label, or role

❌ Avoid targeting child props based on numeric order:

```tsx
expect(textView[5].props.children).toEqual('Rx #: 3636691')
expect(textView[5].props.children).toEqual(`${t('prescription.rxNumber')}: 3636691`)
```

✅ Instead, target rendered text, role, or accessibility label:
✅ Instead, target rendered text, accessibility label, or role:

```tsx
expect(screen.getByText('Rx #: 3636691')).toBeTruthy()
expect(screen.getByLabelText('Prescription number 3636691')).toBeTruthy()
expect(screen.getByRole('checkbox', { name: 'Prescription 1 of 3', checked: true })).toBeTruthy()
expect(screen.getByText(`${t('prescription.rxNumber')}: 3636691`)).toBeTruthy()
expect(screen.getByLabelText(`${t('prescription.rxNumber.a11yLabel')} 3636691`)).toBeTruthy()
expect(
screen.getByRole('checkbox', {
name: t('prescription.history.orderIdentifier', { idx: 1, total: 3 }),
checked: true,
}),
).toBeTruthy()
```

### Why?
This method reduces test fragility because moving an element into/out of a child component, changing props, or adding/removing sibling components does not break the test. Targeting accessibility label or role ensures screen readers read the correct label and/or role to the user, preventing a11y regressions. Finally, this type of test is simpler to read and write because it ignores implementation details, focusing instead on what the user expects to see in rendered output.

This method reduces test fragility because moving an element into/out of a child component, changing props, or adding/removing sibling components does not break the test. Targeting accessibility label or role ensures screen readers read the correct label and/or role to the user, preventing a11y regressions. Finally, this type of test is simpler to read and write because it ignores implementation details, focusing instead on what the user expects to see in rendered output.

### More information

- React Testing Library's [guiding principles](https://testing-library.com/docs/guiding-principles)
- [Some thoughts](https://www.boyney.io/blog/2019-05-21-my-experience-moving-from-enzyme-to-react-testing-library) on why this RTL approach is an improvement over Enzyme
- [The Dangers of Shallow Rendering](https://mskelton.medium.com/the-dangers-of-shallow-rendering-343e48fe5f28)

## Targeting by translated text

❌ Avoid using actual strings to target elements:

```tsx
fireEvent.press(screen.getByRole('tab', { name: "Make sure you're in the right health portal" }))
```

✅ Instead, call the translation function as you do in the component under test:

```tsx
fireEvent.press(screen.getByRole('tab', { name: t('cernerAlertSM.header') }))
```

### Why?

Using the translation function reduces test fragility. Minor wording changes won't break the test.

## Firing events

❌ Avoid calling a callback function in a prop to simulate user interaction:

```tsx
Expand All @@ -53,71 +81,82 @@ testInstance.findByType(Pressable).props.onPress()
✅ Instead, fire a press event:

```tsx
fireEvent.press(screen.getByText('Cancel'))
fireEvent.press(screen.getByText(t('cancel')))
```

✅ Fire a changeText event:

```tsx
fireEvent.changeText(screen.getByText('Phone'), '123-456-7890');
fireEvent.changeText(screen.getByText(t('phone')), '123-456-7890')
```

✅ Fire a scroll event:

```tsx
fireEvent.scroll(screen.getByText('scroll-view'), {
nativeEvent: { contentOffset: { y: 200 } }
nativeEvent: { contentOffset: { y: 200 } },
})
```

### Why?

Calling a callback function in a prop only checks that the function runs. It doesn’t test that the element is visible to the user and that it’s wired up correctly. It’s also fragile because refactoring the component might change the props and break the test. Firing an event resolves these concerns, which also apply to text fields and scrolling.

## Exercising key functionality
❌ Avoid tests that just check large quantities of static props:

❌ Avoid tests that just check large quantities of static text:

```tsx
expect(textView[6].props.children).toEqual('What’s next')
expect(textView[7].props.children).toEqual("We're reviewing your refill request. Once approved, the VA pharmacy will process your refill.")
expect(textView[8].props.children).toEqual('If you have questions about the status of your refill, contact your provider or local VA pharmacy.')
expect(textView[7].props.children).toEqual(
"We're reviewing your refill request. Once approved, the VA pharmacy will process your refill.",
)
expect(textView[8].props.children).toEqual(
'If you have questions about the status of your refill, contact your provider or local VA pharmacy.',
)
```

✅ Instead, focus on tests that check important functionality:

```tsx
describe('on click of the "Go to inbox" link', () => {
it('calls useRouteNavigation and updateSecureMessagingTab', () => {
fireEvent.press(screen.getByRole('link', { name: 'Go to inbox' }))
fireEvent.press(screen.getByRole('link', { name: t('secureMessaging.goToInbox') }))
expect(navigate).toHaveBeenCalled()
expect(updateSecureMessagingTab).toHaveBeenCalled()
})
})
```

### Why?

Each test should add value by serving as a focused warning that something important has failed. Testing that a sequence of TextViews renders certain text doesn't tell us much. It's also fragile because the smallest text change breaks the test. Testing important and/or complex logic is more beneficial because that’s where high-impact regressions typically occur. In addition, tests for complicated logic serve as a form of documentation, letting engineers know how the code is supposed to function.

### More information

- See #2 of [The 7 Sins of Unit Testing](https://www.testrail.com/blog/the-7-sins-of-unit-testing/) about why more assertions can be worse, not better

## Testing from the user’s perspective

Consider what the user expects to do and see, then write tests that simulate it. For example, let's say the user expects to press “Select all”, then see two checked checkboxes and relevant text.

✅ This test tells the user's story and checks it at the same time:

```tsx
it('toggles items when "Select all" is pressed', () => {
fireEvent.press(screen.getByText('Select all'))
expect(screen.getByRole('checkbox', { name: 'One', checked: true })).toBeTruthy()
expect(screen.getByRole('checkbox', { name: 'Two', checked: true })).toBeTruthy()
expect(screen.getByText('2/2 selected')).toBeTruthy()
fireEvent.press(screen.getByText(t('select.all')))
expect(screen.getByRole('checkbox', { name: t('one'), checked: true })).toBeTruthy()
expect(screen.getByRole('checkbox', { name: t('two'), checked: true })).toBeTruthy()
expect(screen.getByText(t('selectedOutOfTotal', { selected: 2, total: 2 }))).toBeTruthy()
})
```

### Why?

By taking the user's point of view, user-focused tests help prevent the most damaging regressions, ones which prevent users from completing their desired tasks. But because implementation details aren't baked into the test, engineers retain the flexibility to refactor as needed without causing test failures.

### More information

- Why it's important to focus on the [end user](https://kentcdodds.com/blog/avoid-the-test-user) and avoid the "test user"

## Test File Naming
Expand All @@ -127,6 +166,7 @@ The test file should live in the same location as its associated component with
`ClaimsScreen.tsx` will have a test file named `ClaimsScreen.test.tsx`

## Running Tests

- Run unit tests with `yarn test`
- Coverage can be found under `coverage/lcov-report/index.html`

Expand Down Expand Up @@ -174,12 +214,12 @@ This block of code will mock the entirety of the hooks util file using the origi
navigateToPaymentMissingSpy = jest.fn()

when(mockNavigationSpy)
.mockReturnValue(() => {})
.calledWith('PaymentMissing')
.mockReturnValue(navigateToPaymentMissingSpy)
.mockReturnValue(() => {})
.calledWith('PaymentMissing')
.mockReturnValue(navigateToPaymentMissingSpy)
```

This will create another object `navigateToPaymentMissingSpy` that will be returned if the hook is called with the parameters `'PaymentMissing'`
This will create another object `navigateToPaymentMissingSpy` that will be returned if the hook is called with the parameters `'PaymentMissing'`

```tsx
// Do something that will trigger a navigation to the PaymentMissing screen
Expand Down Expand Up @@ -212,4 +252,4 @@ const letterOptions = {
expect(downloadLetter).toBeCalledWith(LetterTypeConstants.benefitSummary, letterOptions)
```

This checks to see that the `downloadLetter` action was called with the expected parameters
This checks to see that the `downloadLetter` action was called with the expected parameters
49 changes: 22 additions & 27 deletions VAMobile/src/screens/BenefitsScreen/BenefitsScreen.test.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import React from 'react'

import { fireEvent, screen, waitFor } from '@testing-library/react-native'
import { t } from 'i18next'
import { DateTime } from 'luxon'

import { claimsAndAppealsKeys } from 'api/claimsAndAppeals'
Expand Down Expand Up @@ -45,19 +46,23 @@ context('BenefitsScreen', () => {
it('displays active claims count when veteran has active claims', () => {
const activeClaimsCount = 3
initializeTestInstance(activeClaimsCount)
expect(screen.getByRole('link', { name: 'Claims' })).toBeTruthy()
expect(screen.getByRole('link', { name: `${activeClaimsCount} active` })).toBeTruthy()
expect(screen.getByRole('link', { name: t('claims.title') })).toBeTruthy()
expect(
screen.getByRole('link', { name: t('claims.activityButton.subText', { count: activeClaimsCount }) }),
).toBeTruthy()
})

it('does not display active claims count when there are no active claims', () => {
const activeClaimsCount = 0
initializeTestInstance(activeClaimsCount)
expect(screen.queryByRole('link', { name: `${activeClaimsCount} active` })).toBeFalsy()
expect(
screen.queryByRole('link', { name: t('claims.activityButton.subText', { count: activeClaimsCount }) }),
).toBeFalsy()
})

it('navigates to Claims history screen when claims button is pressed', () => {
initializeTestInstance(3)
fireEvent.press(screen.getByRole('link', { name: 'Claims' }))
fireEvent.press(screen.getByRole('link', { name: t('claims.title') }))
expect(mockNavigationSpy).toHaveBeenCalledWith('ClaimsHistoryScreen')
})

Expand All @@ -71,7 +76,9 @@ context('BenefitsScreen', () => {
]

initializeTestInstance(activeClaimsCount, serviceErrors)
expect(screen.queryByRole('link', { name: `${activeClaimsCount} active` })).toBeFalsy()
expect(
screen.queryByRole('link', { name: t('claims.activityButton.subText', { count: activeClaimsCount }) }),
).toBeFalsy()
})

it('displays warning message when there is a service error', () => {
Expand All @@ -84,11 +91,7 @@ context('BenefitsScreen', () => {
]

initializeTestInstance(activeClaimsCount, serviceErrors)
expect(
screen.getByText(
"We can't get some of your information right now. Benefits activity may not be accurate. Check back later.",
),
).toBeTruthy()
expect(screen.getByText(t('benefits.activity.nonFatalError'))).toBeTruthy()
})

it('displays downtime message when claims or appeals is in downtime', () => {
Expand All @@ -107,9 +110,7 @@ context('BenefitsScreen', () => {
}

initializeTestInstance(activeClaimsCount, undefined, preloadedState)
expect(
screen.getByText("We're working on the mobile app. Benefits activity may not be accurate. Check back later."),
).toBeTruthy()
expect(screen.getByText(t('benefits.activity.warning.downtime'))).toBeTruthy()
})

it('does not display active claims when claims or appeals is in downtime', () => {
Expand All @@ -128,43 +129,37 @@ context('BenefitsScreen', () => {
}

initializeTestInstance(activeClaimsCount, undefined, preloadedState)
expect(screen.queryByRole('link', { name: `${activeClaimsCount} active` })).toBeFalsy()
expect(
screen.queryByRole('link', { name: t('claims.activityButton.subText', { count: activeClaimsCount }) }),
).toBeFalsy()
})

it('displays error message when the claims API call throws an error', async () => {
when(get as jest.Mock)
.calledWith('/v0/claims-and-appeals-overview', expect.anything())
.mockRejectedValue('fail')
initializeTestInstance()
await waitFor(() =>
expect(
screen.getByText("We can't get some of your information. Benefits activity may not be accurate."),
).toBeTruthy(),
)
await waitFor(() => expect(screen.getByText(t('benefits.activity.error'))).toBeTruthy())
})

it('does not display active claims count when the claims API call throws an error', async () => {
when(get as jest.Mock)
.calledWith('/v0/claims-and-appeals-overview', expect.anything())
.mockRejectedValue('fail')
initializeTestInstance()
await waitFor(() =>
expect(
screen.getByText("We can't get some of your information. Benefits activity may not be accurate."),
).toBeTruthy(),
)
await waitFor(() => expect(screen.queryByText('active')).toBeFalsy())
await waitFor(() => expect(screen.getByText(t('benefits.activity.error'))).toBeTruthy())
await waitFor(() => expect(screen.queryByText(t('active'))).toBeFalsy())
})

it("navigates to Letters screen when 'letters and document' button is pressed", () => {
initializeTestInstance()
fireEvent.press(screen.getByRole('link', { name: 'VA letters and documents' }))
fireEvent.press(screen.getByRole('link', { name: t('lettersAndDocs.title') }))
expect(mockNavigationSpy).toHaveBeenCalledWith('LettersOverview')
})

it('navigates to Disability rating screen when Disability rating button is pressed', () => {
initializeTestInstance()
fireEvent.press(screen.getByRole('link', { name: 'Disability rating' }))
fireEvent.press(screen.getByRole('link', { name: t('disabilityRating.title') }))
expect(mockNavigationSpy).toHaveBeenCalledWith('DisabilityRatings')
})
})
38 changes: 14 additions & 24 deletions VAMobile/src/screens/HealthScreen/CernerAlert/CernerAlert.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,10 @@ import React from 'react'
import { Alert } from 'react-native'

import { fireEvent, screen } from '@testing-library/react-native'
import { t } from 'i18next'

import { context, render } from 'testUtils'
import { a11yLabelVA } from 'utils/a11yLabel'

import CernerAlert from './CernerAlert'

Expand Down Expand Up @@ -68,37 +70,25 @@ context('CernerAlert', () => {
})

it('When only cerner facilities', () => {
fireEvent.press(screen.getByRole('tab', { name: 'Your care team uses My VA Health' }))
expect(screen.getByLabelText('Your care team uses My V-A Health')).toBeTruthy()
expect(
screen.getByLabelText("You'll need to use our My V-A Health portal to manage your care at these facilities:"),
).toBeTruthy()
expect(
screen.getByText("You'll need to use our My VA Health portal to manage your care at these facilities:"),
).toBeTruthy()
fireEvent.press(screen.getByRole('tab', { name: t('healthHelp.usesVAHealth') }))
expect(screen.getByLabelText(a11yLabelVA(t('healthHelp.usesVAHealth')))).toBeTruthy()
expect(screen.getByLabelText(a11yLabelVA(t('healthHelp.goToPortal')))).toBeTruthy()
expect(screen.getByText(t('healthHelp.goToPortal'))).toBeTruthy()
expect(screen.getByText('FacilityOne')).toBeTruthy()
expect(screen.getByText('FacilityTwo')).toBeTruthy()
fireEvent.press(screen.getByRole('link', { name: 'Go to My VA Health' }))
fireEvent.press(screen.getByRole('link', { name: t('goToMyVAHealth') }))
expect(Alert.alert).toBeCalled()
})

it('when some facilities are cerner and pressing the link', async () => {
fireEvent.press(screen.getByRole('tab', { name: 'Some of your care team uses My VA Health' }))
expect(screen.getByLabelText('Some of your care team uses My V-A Health')).toBeTruthy()
expect(
screen.getByLabelText("You'll need to use our My V-A Health portal to manage your care at these facilities:"),
).toBeTruthy()
expect(
screen.getByText("You'll need to use our My VA Health portal to manage your care at these facilities:"),
).toBeTruthy()
fireEvent.press(screen.getByRole('tab', { name: t('cernerAlert.header.some') }))
expect(screen.getByLabelText(a11yLabelVA(t('cernerAlert.header.some')))).toBeTruthy()
expect(screen.getByLabelText(a11yLabelVA(t('healthHelp.goToPortal')))).toBeTruthy()
expect(screen.getByText(t('healthHelp.goToPortal'))).toBeTruthy()
expect(screen.getByText('FacilityOne')).toBeTruthy()
expect(
screen.getByLabelText('You can still use this app to manage your care at other V-A health facilities.'),
).toBeTruthy()
expect(
screen.getByText('You can still use this app to manage your care at other VA health facilities.'),
).toBeTruthy()
fireEvent.press(screen.getByLabelText('Go to My V-A Health'))
expect(screen.getByLabelText(a11yLabelVA(t('healthHelp.canStillUse')))).toBeTruthy()
expect(screen.getByText(t('healthHelp.canStillUse'))).toBeTruthy()
fireEvent.press(screen.getByLabelText(a11yLabelVA(t('goToMyVAHealth'))))
expect(Alert.alert).toBeCalled()
})
})
Loading

0 comments on commit c16a60a

Please sign in to comment.