[HOLD for payment 2024-04-03] [PAY March 27][$250] [Reassure] Add reassure test for BaseOptionsList

[mountiny]

Problem

The components/OptionsList/BaseOptionsList is a complex component in the App and if its performance degrades, it can have considerable impact on the App performance.

Solution

Let's add a reassure test to cover various cases of this component use similarly as we have created such tests for SelectionList component for example.

---

Please refer to other tests in the repository, here is a readme for reassure tests which should help you get familiar with the tool.

All new tests should be written in TS.

Please provide a proposal stating, how you could write such test and scenarios for the component such that we test the slowest execution path as well.

cc @OlimpiaZurek

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~013308adf811c76d8b
• Upwork Job ID: 1767637401542344704
• Last Price Increase: 2024-03-12

Issue Owner

Current Issue Owner: @slafortune

[melvin-bot]

Triggered auto assignment to @slafortune (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~013308adf811c76d8b

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @jayeshmangwani (External)

[melvin-bot]

Upwork job price has been updated to $250

[ShridharGoel]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Add performance test for BaseOptionsList.

What is the root cause of that problem?

New tests to ensure stability of performance in BaseOptionsList.

What changes do you think we should make in order to solve the problem?

Example test:

function BaseOptionsListWrapper({
    sections,
}: {
    sections: OptionsListData<Section>[];
}) {
    return (
        <BaseOptionsList
            sections={sections}
            headerMessage="Options List Header"
            isLoading={false}
            onSelectRow={() => {}}
            focusedIndex={0}
        />
    );
}

test('[BaseOptionsList] should render 1 section and a thousand items', () => {
    const sections: OptionsListData<Section>[] = [
        {
            data: Array.from({length: 1000}, (_, index) => ({
                text: `Item ${index}`,
                keyForList: `item-${index}`,
            })),
            indexOffset: 0,
            isDisabled: false,
            shouldShow: true,
            title: 'Section 1',
        },
    ];

    measurePerformance(
        <BaseOptionsListWrapper sections={sections} />,
    );
});

[brunovjk]

Please re-state the problem that we are trying to solve in this issue.

Add reassure test for BaseOptionsList

What is the root cause of that problem?

The intricate nature of the BaseOptionsList component, when experiencing performance degradation, can have a notable impact on the application's responsiveness.

What changes do you think we should make in order to solve the problem?

To address this concern, we propose the addition of a reassurance test that comprehensively covers different use cases of the BaseOptionsList component. This reassurance test should be crafted in TypeScript (TS) to align with our project's language choices. The focus should be on evaluating the slowest execution path of the component, ensuring its robustness and optimal performance.

Additionally, we could follow the measurePerformance approach outlined in the App/tests/perf-test/SelectionList.perf-test.tsx file. This approach involves scenarios such as rendering lists with multiple items, testing scrolling performance, assessing onPress/onSelect actions, and evaluating text input interactions. By implementing this strategy, we can ensure that the reassurance test covers a broad range of potential performance bottlenecks.

What alternative solutions did you explore? (Optional)

[ghost]

Proposal

Please re-state the problem that we are trying to solve in this issue.

[Reassure] Add reassure test for BaseOptionsList]

What is the root cause of that problem?

** New Feature of Writing Test **

What changes do you think we should make in order to solve the problem?

This is the example Test for reference :

import {measureFunction} from 'reassure';  
import BaseOptionsList from '@src/components/OptionsList/BaseOptionsList';

describe('BaseOptionsList performance', () => {

  const mockSections = // array of 1000 sections
  const mockSelectedOptions = // array of 1000 selected options

  it('measures rendering with 1000 sections', async () => {

    const wrapper = shallow(
      <BaseOptionsList 
        sections={mockSections}
        selectedOptions={mockSelectedOptions}
      />
    );
    
    await measureFunction(() => {
      wrapper.render();
    });

  });

});

• This test creates a large amount of mock data - 1000 sections and 1000 selected options.

• It renders the BaseOptionsList component with this data and times how long the initial render takes.

• This allows assessing the performance rendering BaseOptionsList with a realistic amount of data it may handle.

• Additional tests could try different amounts of mock data to test scaling. Or measure rerender times by changing specific props like selectedOptions.

• The goal is to catch any regressions in rendering performance as the data grows over time. By testing with production-level data volumes, it reduces the risk of slowdowns when users have large amounts of options.

What alternative solutions did you explore? (Optional)

N/A

[jayeshmangwani]

Thanks for the proposals

From the above proposals I like the @ShridharGoel 's Proposal, they have mentioned a basic thousand items render scenario, in the PR we can add the few real-life other scenarios like,

1. scroll and press a option list item
2. render multiple options and select list items

🎀 👀 🎀 C+ reviewed

[melvin-bot]

Current assignee @mountiny is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[melvin-bot]

❌ There was an error making the offer to @jayeshmangwani for the Reviewer role. The BZ member will need to manually hire the contributor.

[melvin-bot]

❌ There was an error making the offer to @ShridharGoel for the Contributor role. The BZ member will need to manually hire the contributor.

[mountiny]

@ShridharGoel please create a Pr and can you please add the scenarios @jayeshmangwani mentioned?

[mountiny]

PR is up

[slafortune]

Merged on 3/20 - so without a regression, payment would be 3/27

[melvin-bot]

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

[melvin-bot]

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

• [NoQA] Add performance tests for BaseOptionsList #38435

If no regressions arise, payment will be issued on 2024-04-03. 🎊

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

• @jayeshmangwani requires payment (Needs manual offer from BZ)
• @ShridharGoel requires payment (Needs manual offer from BZ)

[melvin-bot]

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

• [@jayeshmangwani] Please propose regression test steps to ensure the new feature will work correctly on production in further releases.
• [@slafortune] Link the GH issue for creating/updating the regression test once above steps have been agreed upon.

[slafortune]

@mountiny @jayeshmangwani will we need regression tests for this?

[slafortune]

@jayeshmangwani offer sent here
@ShridharGoel offer sent here

Holding until 4/3

[jayeshmangwani]

@slafortune No regression tests are needed, update was in the automated tests

[mountiny]

No tests required here

[slafortune]

@jayeshmangwani @ShridharGoel please accept the offers sent.

[jayeshmangwani]

@slafortune Please send a new offer; Getting an error when accepting the old offer.

[slafortune]

Can you try to search for it another way - Upworks is wonky sometimes -

[jayeshmangwani]

@slafortune Declined the offer, and I have applied manually to the job Upwork job https://www.upwork.com/jobs/~013308adf811c76d8b

[slafortune]

Alright - I accepted the proposal and sent you another offer - https://www.upwork.com/nx/wm/offer/101721107

[slafortune]

@jayeshmangwani is paid
Waiting on @ShridharGoel to accept offer sent here

[ShridharGoel]

@slafortune I'm getting the same issue, unable to accept it.

[slafortune]

@ShridharGoel are you able to do the same thing that @jayeshmangwani just did? Since it worked, seems like the best option.

[ShridharGoel]

@slafortune It says "This job is no longer available.".

[slafortune]

I've sent you another offer here - https://www.upwork.com/nx/wm/offer/101722838

[ShridharGoel]

Accepted.

…

On Thu, 4 Apr 2024 at 03:03, Stevie LaFortune ***@***.***> wrote: I've sent you another offer here - https://www.upwork.com/nx/wm/offer/101722838 — Reply to this email directly, view it on GitHub <#38168 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AIPLJHHJDGT62ZMNJYP3533Y3RYUBAVCNFSM6AAAAABES33KPCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMZVGYZTKMBSHA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[slafortune]

All paid