Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
mountiny opened this issue Jun 12, 2024 · 12 comments
Closed
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Weekly KSv2

Comments

@mountiny
Copy link
Contributor

mountiny commented Jun 12, 2024

After updating Onyx to a later version here: #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
@mountiny mountiny added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jun 12, 2024
@mountiny mountiny self-assigned this Jun 12, 2024
Copy link

melvin-bot bot commented Jun 12, 2024

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
Copy link
Contributor

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

@mountiny
Copy link
Contributor Author

Thanks

@OlimpiaZurek
Copy link
Contributor

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.

@melvin-bot melvin-bot bot added the Overdue label Jun 17, 2024
@laurenreidexpensify
Copy link
Contributor

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

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Jun 17, 2024
@OlimpiaZurek
Copy link
Contributor

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 melvin-bot bot removed the Overdue label Jun 17, 2024
@laurenreidexpensify laurenreidexpensify removed the Bug Something is broken. Auto assigns a BugZero manager. label Jun 19, 2024
@melvin-bot melvin-bot bot added the Overdue label Jun 19, 2024
@laurenreidexpensify laurenreidexpensify removed their assignment Jun 19, 2024
@melvin-bot melvin-bot bot removed the Overdue label Jun 19, 2024
@laurenreidexpensify laurenreidexpensify added Overdue Bug Something is broken. Auto assigns a BugZero manager. labels Jun 19, 2024
Copy link

melvin-bot bot commented Jun 19, 2024

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.

@melvin-bot melvin-bot bot removed the Overdue label Jun 19, 2024
@laurenreidexpensify
Copy link
Contributor

@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 melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Jun 19, 2024
@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Jun 24, 2024
@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Jul 3, 2024
@melvin-bot melvin-bot bot changed the title [LOW] [Performance] Fix onyx mocking for reassure [HOLD for payment 2024-07-10] [LOW] [Performance] Fix onyx mocking for reassure Jul 3, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Jul 3, 2024
Copy link

melvin-bot bot commented Jul 3, 2024

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

Copy link

melvin-bot bot commented Jul 3, 2024

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

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

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

Copy link

melvin-bot bot commented Jul 3, 2024

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] The PR that introduced the bug has been identified. Link to the PR:
  • [@mountiny] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
  • [@mountiny] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
  • [@OlimpiaZurek] Determine if we should create a regression test for this bug.
  • [@OlimpiaZurek] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
  • [@kadiealexander] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

@mountiny
Copy link
Contributor Author

mountiny commented Jul 5, 2024

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

Thank you @OlimpiaZurek

@mountiny mountiny closed this as completed Jul 5, 2024
@github-project-automation github-project-automation bot moved this from LOW to Done in [#whatsnext] #quality Jul 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Weekly KSv2
Projects
Development

No branches or pull requests

4 participants