search

Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.11k stars 2.61k forks source link

[HOLD for payment 2024-07-10] [LOW] [Performance] Fix onyx mocking for reassure #43549

Closed mountiny closed 5 hours ago

mountiny commented 3 weeks ago

After updating Onyx to a later version here: https://github.com/Expensify/App/pull/42772, the mocking for Onyx has to be updated in the reassure tests, as otherwise, they will fail.

We have disabled the tests after confirming the regressions are not manifesting in the product.

Discussing in this slack thread. Let's figure out how to fix the onyx mocking and update the tests with it cc @chrispader @OlimpiaZurek

Issue OwnerCurrent Issue Owner: @kadiealexander
melvin-bot[bot] commented 3 weeks ago

Triggered auto assignment to @laurenreidexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

OlimpiaZurek commented 3 weeks ago

Hi. I'm Olimpia from Callstack. I would like to work on this issue.

mountiny commented 3 weeks ago

Thanks

OlimpiaZurek commented 3 weeks ago

Daily Update: It looks like the issue with increasing render count after removing null values from the cache in onyx is likely due to more frequent state updates and re-renders. The Jest environment makes things worse by handling mocks, asynchronous operations, and state updates differently than in a real application, leading to excessive rendering.

I have found that simplifying the tests by breaking the scenarios into smaller parts and testing the performance of smaller pieces of the component could solve this problem. This will ensure that each part of the scenario addresses specific state updates, making it easier to manage and understand the impact of each update on component render counts.

So the next step is to change these tests according to the above conclusions. I should have my PR ready at the beginning of next week.

laurenreidexpensify commented 2 weeks ago

@OlimpiaZurek still on track to get a PR up in next day?

OlimpiaZurek commented 2 weeks ago

I've been working on a different task recently. I'll come back to this tomorrow. The PR should be ready by Wednesday.

melvin-bot[bot] commented 2 weeks ago

Triggered auto assignment to @kadiealexander (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

laurenreidexpensify commented 2 weeks ago

@kadiealexander handing over in prep for parental leave 🙏 this one is still waiting on getting a PR up, but Olimpia should have this done today 👍

melvin-bot[bot] commented 1 day ago

Reviewing label has been removed, please complete the "BugZero Checklist".

melvin-bot[bot] commented 1 day ago

The solution for this issue has been :rocket: deployed to production :rocket: in version 9.0.3-7 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2024-07-10. :confetti_ball:

For reference, here are some details about the assignees on this issue:

melvin-bot[bot] commented 1 day ago

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

mountiny commented 5 hours ago

I believe this is now completed and we do not have anything else actionable.

Thank you @OlimpiaZurek