test: more meaningful AnimatedNumber component test #8309

rahimrahman · 2024-11-01T17:02:10Z

Summary

This was presented during Developer Meeting October 30th. The goal was to ask developers to re-consider the use of toMatchSnapshot which doesn't really give enough information what the component AnimatedNumber is doing and why it's rendering the way it did.

In order to get coverage % when running this test, you'd have to temporarily update the jest.config.js file:

-    coveragePathIgnorePatterns: ['/node_modules/', '/components/', '/screens/'],
+    coveragePathIgnorePatterns: ['/node_modules/'],

npm run test app/components/animated_number/index.test.tsx -- --collectCoverageFrom=app/components/animated_number/index.tsx --coverage --watch

 PASS  app/components/animated_number/index.test.tsx
  AnimatedNumber, using toMatchSnapshot
    ○ skipped should render correctly
    ○ skipped should rerender the correct number
  AnimatedNumber
    ✓ should render the non-animated number (107 ms)
    ✓ should removed the non-animation number after getting the correct height (7 ms)
    ✓ should switch to the animated number view (3 ms)
    ✓ KNOWN UI BUG: should show that there will be an issue if the text height changes, due to the non-animated number view has been removed (2 ms)
    should show the correct number of animated views based on the digits
      ✓ should display 1 view(s) for 1 (2 ms)
      ✓ should display 2 view(s) for 23 (2 ms)
      ✓ should display 3 view(s) for 579 (3 ms)
      ✓ should display 4 view(s) for -123 (3 ms)
      ✓ should display 4 view(s) for 6789 (3 ms)
      ✓ should display 5 view(s) for 23456 (4 ms)
    should show the correct number
      ✓ should display the number 123 (4 ms)
      ✓ should display the number 9982 (4 ms)
      ✓ should display the number 328 (3 ms)
      ✓ should display the number 1111 (3 ms)
      ✓ should display the number 2222 (4 ms)
      ✓ should display the number 3333 (3 ms)
    should rerender the correct number that it animates to
      ✓ should display the number 146 (5 ms)
      ✓ should display the number 144 (3 ms)
      ✓ should display the number 1 (2 ms)
      ✓ should display the number 1000000 (6 ms)
      ✓ should display the number -145 (4 ms)


=============================== Coverage summary ===============================
Statements   : 100% ( 37/37 )
Branches     : 100% ( 16/16 )
Functions    : 100% ( 13/13 )
Lines        : 100% ( 35/35 )
================================================================================

Ticket Link

Checklist

Added or updated unit tests (required for all new features)
Has UI changes
Includes text changes and localization file updates
Have tested against the 5 core themes to ensure consistency between them.
Have run E2E tests by adding label E2E iOS tests for PR.

Device Information

This PR was tested on:

Screenshots

Release Note

rahimrahman · 2024-11-01T17:17:32Z

app/components/animated_number/index.test.tsx

+        fireEvent(text, 'onLayout', {nativeEvent: {layout: {height: NUMBER_HEIGHT}}});
+
+        try {
+            fireEvent(text, 'onLayout', {nativeEvent: {layout: {height: NUMBER_HEIGHT}}});


The onLayout on non-animation-number view is not being triggered since it's no longer there. Even though we're using the same NUMBER_HEIGHT, the point is, it will not update to any number.

I'm surprised that the RN version of Testing Library doesn't automatically fire onLayout events

rahimrahman · 2024-11-01T17:18:49Z

app/components/animated_number/index.test.tsx

+                let translateY = -1;
+
+                // every digit will have a row of 10 numbers, so the translateY should be the height of the number * the number * -1 (since the animation is going up)
+                await waitFor(() => {


I don't think I need a waitFor() anymore since I'm using a Promise.all() afterwards to make sure all the numbers are correct.

streamer45

Great work on this @rahimrahman 💯

Left a couple of comments, but nothing blocking.

streamer45 · 2024-11-04T15:06:09Z

app/components/animated_number/index.test.tsx

+// jest.mock('../../../node_modules/react-native/Libraries/Text/Text', () => {
+//     const Text = jest.requireActual('../../../node_modules/react-native/Libraries/Text/Text');


Is this meant to stay commented out? If so, could we leave a comment explaining its purpose?

streamer45 · 2024-11-04T15:10:20Z

app/components/animated_number/index.test.tsx

+        expect(animatedView).toBeTruthy();
+    });
+
+    describe.each([1, 23, 579, -123, 6789, 23456])('should show the correct number of animated views based on the digits', (animateToNumber: number) => {


This is more of a general question, but ideally, I'd like to find consistency in how we identify test cases, and in this specific instance, it seems like there's some pattern here (testing numbers with increasing digits + negative case).

I wonder whether some fuzz testing could be valuable in such cases rather than arbitrarily choosing some values. I don't have an answer here; I'm just curious to hear more thoughts.

streamer45 · 2024-11-04T15:13:57Z

app/components/animated_number/index.test.tsx

+        } catch (e) {
+            expect(e).toBeTruthy();


Any reason for not using the .rejects.toThrow('error'); pattern in here?

hmhealey

I have a few questions and one request for a helper method, but I definitely like the way this looks compared to the older version even for a more complicated component like this one

hmhealey · 2024-11-04T21:52:11Z

app/components/animated_number/index.test.tsx

+describe('AnimatedNumber', () => {
+    // running on jest, since Animated is a native module, Animated.timing.start needs to be mocked in order to update to the final Animated.Value.
+    // Ex: 1 => 2, the Animated.Value should be -20 (from -10) after the animation is done
+    jest.spyOn(Animated, 'timing').mockImplementation((a, b) => ({


Are we likely to need this when testing other components? I wonder if we might want to mock this globally be adding it to test/setup.ts

hmhealey · 2024-11-04T21:55:43Z

app/components/animated_number/index.test.tsx

+    it('should render the non-animated number', () => {
+        const {getByTestId} = render(<AnimatedNumber animateToNumber={123}/>);
+
+        const text = getByTestId('no-animation-number');


On web, I've been recommending people to get elements with methods like screen.getByLabelText instead of getByTestId since it uses the accessible text. Is there an equivalent on mobile?

From what I understand, our accessibility support on mobile is pretty bad so I wouldn't be surprised if there's an option but not one that we can use

hmhealey · 2024-11-04T22:06:29Z

app/components/animated_number/index.test.tsx

+    });
+
+    describe.each([1, 23, 579, -123, 6789, 23456])('should show the correct number of animated views based on the digits', (animateToNumber: number) => {
+        it(`should display ${animateToNumber.toString().length} view(s) for ${animateToNumber}`, async () => {


We might want to replace any occurrences of X.toString().length with a helper method like countDigits(X) since it took me a minute to remember what that did. Giving it a name would help make it clearer what we're trying to do with it.

On my first read of this, I thought this meant that it made animateToNumber views, and I was going to comment on how that would cause problems when there 😅

hmhealey · 2024-11-04T22:08:27Z

app/components/animated_number/index.test.tsx

+        fireEvent(text, 'onLayout', {nativeEvent: {layout: {height: NUMBER_HEIGHT}}});
+
+        try {
+            fireEvent(text, 'onLayout', {nativeEvent: {layout: {height: NUMBER_HEIGHT}}});


I'm surprised that the RN version of Testing Library doesn't automatically fire onLayout events

larkox · 2024-11-05T09:39:21Z

@rahimrahman Maybe I am missing something, but aren't we missing the snapshots on the PR? I am surprised the CI did not yell about that. Maybe something we should look into.

rahimrahman added 4 commits October 19, 2024 07:43

better unit test for animated_number

35cc12a

updated test

3c52745

Merge branch 'main' into test/animated-number-unit-test

c1d1de8

update test

14d313d

rahimrahman requested a review from Rajat-Dabade November 1, 2024 17:02

mm-cloud-bot added the release-note label Nov 1, 2024

rahimrahman requested review from streamer45, larkox and hmhealey November 1, 2024 17:05

rahimrahman commented Nov 1, 2024

View reviewed changes

streamer45 approved these changes Nov 4, 2024

View reviewed changes

hmhealey added the 2: Dev Review Requires review by a core commiter label Nov 4, 2024

hmhealey approved these changes Nov 4, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: more meaningful AnimatedNumber component test #8309

test: more meaningful AnimatedNumber component test #8309

rahimrahman commented Nov 1, 2024 •

edited

Loading

rahimrahman Nov 1, 2024

hmhealey Nov 4, 2024

rahimrahman Nov 1, 2024

streamer45 left a comment

streamer45 Nov 4, 2024

streamer45 Nov 4, 2024

streamer45 Nov 4, 2024

hmhealey left a comment

hmhealey Nov 4, 2024

hmhealey Nov 4, 2024

hmhealey Nov 4, 2024

hmhealey Nov 4, 2024

larkox commented Nov 5, 2024

		// jest.mock('../../../node_modules/react-native/Libraries/Text/Text', () => {
		// const Text = jest.requireActual('../../../node_modules/react-native/Libraries/Text/Text');

test: more meaningful AnimatedNumber component test #8309

Are you sure you want to change the base?

test: more meaningful AnimatedNumber component test #8309

Conversation

rahimrahman commented Nov 1, 2024 • edited Loading

Summary

Ticket Link

Checklist

Device Information

Screenshots

Release Note

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

streamer45 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hmhealey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

larkox commented Nov 5, 2024

rahimrahman commented Nov 1, 2024 •

edited

Loading