Skip to content

[NO QA] Add an enhanced mock for ReportUtils to fix tests #68768

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.

Sign up for GitHub

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

Merged
tgolen merged 7 commits into from
Aug 19, 2025
Merged

[NO QA] Add an enhanced mock for ReportUtils to fix tests #68768

Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
89d7eb3
Add an enhanced mock for ReportUtils to fix tests
AndrewGable Aug 19, 2025
61e3368
Fix types
AndrewGable Aug 19, 2025
007f63b
Remove comment to trigger tests
AndrewGable Aug 19, 2025
8ab5571
Remove unrelated diff
AndrewGable Aug 19, 2025
5aa82c6
Add comment describing mock
AndrewGable Aug 19, 2025
0bc862d
Merge branch 'main' into andrew-perf-tests
AndrewGable Aug 19, 2025
8558ca3
Clean up code comments even more
AndrewGable Aug 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions tests/perf-test/OptimisticReportNames.perf-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,10 @@ import waitForBatchedUpdates from '../utils/waitForBatchedUpdates';

// Mock dependencies
jest.mock('@libs/ReportUtils', () => ({
// jest.requireActual is necessary to include multi-layered module imports (eg. Report.ts has processReportIDDeeplink() which uses parseReportRouteParams() imported from getReportIDFromUrl.ts)
// Without jest.requireActual, parseReportRouteParams would be undefined, causing the test to fail.
...jest.requireActual<typeof ReportUtils>('@libs/ReportUtils'),
// These methods are mocked below in the beforeAll function to return specific values
isExpenseReport: jest.fn(),

@tgolen tgolen Aug 19, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do these need to be mocked because they use the router, and the router is not available in the unit test environment?

I'd love it if all mocks actually included a thorough explanation of why they are necessary.

@chiragsalian chiragsalian Aug 19, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm i think comment should be a NAB and the proper solution can be discussed and applied in a follow up. I say this because we mocked it before and we're just fixing the mock to unblock other PRs.

@chiragsalian chiragsalian Aug 19, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

im not an expert on jest but i did a quick check. From my understanding the reason we're written it this way is cause we use the actual methods of ReportUtils like getTitleReportField. It doesn't use the router so its safe for the test to use.
Additionally we mock some of the methods like isExpenseReport to always true so that setup is perhaps easier in this test.

Also yeah i agree, having this in a comment would be helpful.

@tgolen tgolen Aug 19, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding a comment, but I don't think it helps me understand why the mock is necessary, or why only these two methods are turned into noops. Chirag's explanation comes close... but I still don't know that I get it.

Partial mock preserves parseReportRouteParams

Why is this important? In order to try and answer that, I'd need to hunt through a ton of code to try and figure it out. Also, it's not clear what parseReprotRouteParams has to do with these methods.

and other exports that are used by Report.ts's dependencies

I think that the comment should explain the mocked methods, specifically isExpenseReport and getTitleReportField, rather than just saying all the other exports are untouched (which is pretty obvious from just looking at the mock).

@AndrewGable AndrewGable Aug 19, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

getTitleReportField: jest.fn(),
}));
Expand Down
Loading