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
    ✓ should render the non-animated number (155 ms)
    ✓ should removed the non-animation number after getting the correct height (7 ms)
    ✓ should switch to the animated number view (4 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 (3 ms)
    should show the correct number of animated views based on the digits
      ✓ should display 1 view(s) for 1 (1 ms)
      ✓ should display 2 view(s) for 23 (3 ms)
      ✓ should display 3 view(s) for 579 (2 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 (3 ms)
      ✓ should display the number 9982 (4 ms)
      ✓ should display the number 12345 (4 ms)
      ✓ should display the number 901876 (6 ms)
      ✓ should display the number -157 (2 ms)
    should rerender the correct number that it animates to
      ✓ should display the number 146 (4 ms)
      ✓ should display the number 144 (4 ms)
      ✓ should display the number 1 (3 ms)
      ✓ should display the number 1000000 (6 ms)
      ✓ should display the number -145 (5 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

app/components/animated_number/index.test.tsx

streamer45

Great work on this @rahimrahman 💯

Left a couple of comments, but nothing blocking.

app/components/animated_number/index.test.tsx

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.

In this instance, the whole idea is to show how many views were created based on digits. We have 1, 2, 3, 4 (- is considered a "digit"), 4 and 5 digits in those examples.

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?

Because fireEvent() is not a promise and there's no rejection.

It still throws an error, because the View is no longer there (since it has been removed).

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

timing is taking a value from one to another at a certain duration, I don't care how long but in this example, I want animation.value to become b.toValue.

The implementation could be a bit more complicated like using easing, etc. So it might be unique in other test cases.

app/components/animated_number/index.test.tsx

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 · 2024-11-18T17:04:32Z

@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.

I thought I copied this test from another test somewhere, but for the life of me, I couldn't figure out where now.

Or, I wrote the original test to showcase how toMatchSnapshot() could be deceiving. With the thought of creating the newer test as educational purpose.

rahimrahman · 2024-11-20T18:37:03Z

@yasserfaraazkhan I didn't make any major changes to animated_number/index.tsx other than adding testID and using a better index for the views. I don't think it warrants even regression testing, but if you feel the need, do specify.

yasserfaraazkhan

Verified running the app locally on simulators. Checked for rections on a root post, threaded post. It worked as exptected.

* better unit test for animated_number * updated test * update test * clean up based on comments

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

app/components/animated_number/index.test.tsx Outdated Show resolved Hide resolved

rahimrahman commented Nov 1, 2024

View reviewed changes

app/components/animated_number/index.test.tsx Outdated Show resolved Hide resolved

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

clean up based on comments

ae9b58c

rahimrahman requested a review from yasserfaraazkhan November 20, 2024 18:37

rahimrahman added 3: QA Review Requires review by a QA tester and removed 2: Dev Review Requires review by a core commiter labels Nov 20, 2024

yasserfaraazkhan approved these changes Nov 21, 2024

View reviewed changes

larkox approved these changes Nov 21, 2024

View reviewed changes

rahimrahman merged commit 84d320a into main Nov 21, 2024
12 checks passed

rahimrahman deleted the test-animated-number-unit branch November 21, 2024 17:30

larkox pushed a commit that referenced this pull request Nov 26, 2024

test: more meaningful AnimatedNumber component test (#8309)

f290c2f

* better unit test for animated_number * updated test * update test * clean up based on comments

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

streamer45 left a comment

streamer45 Nov 4, 2024

rahimrahman Nov 18, 2024

streamer45 Nov 4, 2024

rahimrahman Nov 18, 2024

hmhealey left a comment

hmhealey Nov 4, 2024

rahimrahman Nov 18, 2024 •

edited

Loading

larkox commented Nov 5, 2024

rahimrahman commented Nov 18, 2024

rahimrahman commented Nov 20, 2024

yasserfaraazkhan left a comment

test: more meaningful AnimatedNumber component test #8309

test: more meaningful AnimatedNumber component test #8309

Conversation

rahimrahman commented Nov 1, 2024 • edited Loading

Summary

Ticket Link

Checklist

Device Information

Screenshots

Release Note

streamer45 left a comment

Choose a reason for hiding this comment

streamer45 Nov 4, 2024

Choose a reason for hiding this comment

rahimrahman Nov 18, 2024

Choose a reason for hiding this comment

streamer45 Nov 4, 2024

Choose a reason for hiding this comment

rahimrahman Nov 18, 2024

Choose a reason for hiding this comment

hmhealey left a comment

Choose a reason for hiding this comment

hmhealey Nov 4, 2024

Choose a reason for hiding this comment

rahimrahman Nov 18, 2024 • edited Loading

Choose a reason for hiding this comment

larkox commented Nov 5, 2024

rahimrahman commented Nov 18, 2024

rahimrahman commented Nov 20, 2024

yasserfaraazkhan left a comment

Choose a reason for hiding this comment

rahimrahman commented Nov 1, 2024 •

edited

Loading

rahimrahman Nov 18, 2024 •

edited

Loading