[HOLD for payment 2024-03-26] [$250] [Reassure] Add measureFunction test for getMemberAccountIDsForWorkspace

[mountiny]

Problem

The getMemberAccountIDsForWorkspaceis a method that is used often in the App and if its performance degrades, it can have considerable impact on the App performance.

Solution

Let's add a measure function reassure test to cover various cases of this method call similarly as we have created such tests for ReportActionUtils methods 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 method 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/~012f1d655a8cb04a5b
• Upwork Job ID: 1767636294734249984
• Last Price Increase: 2024-03-12

Issue Owner

Current Issue Owner: @kevinksullivan

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~012f1d655a8cb04a5b

[melvin-bot]

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

[melvin-bot]

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

[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 measureFunction test for getMemberAccountIDsForWorkspace.

What is the root cause of that problem?

New tests to ensure stability of performance in getMemberAccountIDsForWorkspace.

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

Mock policy members and personal data.

Then, we can have scenarios like:

const scenarios = [
        {
            description: 'With multiple members without errors and login details',
            policyMembers: Array.from({length: 100}, (_, i) => ({errors: {}})),
            personalDetails: Array.from({length: 100}, (_, i) => ({login: `user${i}@example.com`})),
        },
        {
            description: 'With multiple members with errors and without login details',
            policyMembers: Array.from({length: 100}, (_, i) => ({errors: {someError: true}})),
            personalDetails: Array.from({length: 100}, (_, i) => ({})),
        },
        // More examples like these
    ];

We can run for all scenarios like this:

scenarios.forEach((scenario, index) => {
        test(`Scenario ${index + 1}: ${scenario.description}`, async () => {
            await measureFunction(() => getMemberAccountIDsForWorkspace(scenario.policyMembers, scenario.personalDetails));
        });
    });

[ghost]

Proposal

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

[Reassure] Add measureFunction test for getMemberAccountIDsForWorkspace

What is the root cause of that problem?

New Feature of writing Tests

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

This is the example test :

import {measureFunction} from 'perf-test-utils'; 
import {createMockPolicyMembers, createMockPersonalDetails} from 'test-data-utils';

describe('getMemberAccountIDsForWorkspace perf', () => {

  let policyMembers;
  let personalDetails;

  beforeEach(() => {
    // Create 10,000 mock policyMembers
    policyMembers = createMockPolicyMembers(10000);

    // Create 10,000 mock personalDetails
    personalDetails = createMockPersonalDetails(10000);
  });

  it('should handle 10,000 members', async () => {
    const result = await measureFunction(() => {
      getMemberAccountIDsForWorkspace(policyMembers, personalDetails);
    });
    
    // Assert performance is within expectations
    expect(result.duration).toBeLessThan(500); 
  });

  it('should handle 100,000 members', async () => {
    // Create 100,000 mock data
    policyMembers = createMockPolicyMembers(100000);
    personalDetails = createMockPersonalDetails(100000);

    const result = await measureFunction(() => {
      getMemberAccountIDsForWorkspace(policyMembers, personalDetails);
    });
    
    // Assert performance is still acceptable with 100K rows
    expect(result.duration).toBeLessThan(5000);
  });

});

The test creates mock policyMember and personalDetails data in different volumes. It times how long the getMemberAccountIDsForWorkspace function takes to process the data. It asserts the performance is within expected thresholds for different data sizes. This allows catching regressions as the data scales up.

What alternative solutions did you explore? (Optional)

N/A

[paultsimura]

The proposal of @ShridharGoel looks good to me.
Just please note that the policyMembers is not an array, but an object.

@godofoutcasts94 I appreciate your intention to assert the execution time, but the measureFunction works a bit differently. Please refer to the Callstack's documentation for more details.

🎀👀🎀 C+ reviewed

[melvin-bot]

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

[mountiny]

@ShridharGoel Can you please raise a PR? thanks!

[melvin-bot]

❌ There was an error making the offer to @paultsimura 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.

[ShridharGoel]

PR: #38271

[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.54-4 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 getMemberAccountIDsForWorkspace #38271

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

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

• @paultsimura 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:

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

[paultsimura]

• Determine if we should create a regression test for this bug: No – this was just an addition of a performance test.

[kevinksullivan]

updated job here

https://www.upwork.com/jobs/~018339f69bac8f5267

@ShridharGoel lmk when you accept!

[ShridharGoel]

@kevinksullivan Accepted, thanks.

[mountiny]

@kevinksullivan @ShridharGoel @paultsimura can this be closed?

[paultsimura]

From my side - all settled

[mountiny]

Ok thanks!

[ShridharGoel]

@kevinksullivan I think my payment is pending, can you check it?

[ShridharGoel]

@kevinksullivan Can you check this?

[kevinksullivan]

All set