[No QA] Clearing logs in unit tests by elirangoshen · Pull Request #70095 · Expensify/App

elirangoshen · 2025-09-08T14:05:21Z

Explanation of Change

Clearing logs in unit tests

Fixed Issues

$ #70676
PROPOSAL: Clearing console logs from unit tests

Background:
Unit tests often include many logs that can provide insight on what's happening in that test. In JavaScript, console logs are generally divided into four levels:
debug
info (this is generally the default used by console.log)
warn
error

If you are running a full test suite, you're likely to be most interested in error logs, because they will be the ones that show you which tests are failing and why. If you're running or debugging an individual test, you're more likely to be interested in all logs.

Problem
When running our jest tests, the large volume of logs across all tests makes it difficult to quickly see which tests failed and why. This prevents us from quickly resolving issues when tests fail on a PR, which in turn can delay the merge and deploy of the PR.

Solution
Silence debug, info, and warn logs by default in our jest tests. This will greatly reduce the volume of logs and make it easier to quickly identify which tests failed and why. Enable developers to see all log levels by passing the --verbose flag to jest; that way we can still dive into more detail in an individual test for debugging purposes.

Tests

Verify that no errors appear in the JS console

Offline tests

QA Steps

// TODO: These must be filled out, or the issue title must include "[No QA]."

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Android: Native

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

MacOS: Chrome / Safari

MacOS: Desktop

github-actions · 2025-09-08T14:05:36Z

All contributors have signed the CLA ✍️ ✅
_{Posted by the CLA Assistant Lite bot.}

github-actions · 2025-09-10T10:57:09Z

⚠️ This PR is possibly changing native code and/or updating libraries, it may cause problems with HybridApp. Please check if any patch updates are required in the HybridApp repo and run an AdHoc build to verify that HybridApp will not break. Ask Contributor Plus for help if you are not sure how to handle this. ⚠️

elirangoshen · 2025-09-15T08:43:34Z

I have read the CLA Document and I hereby sign the CLA

melvin-bot · 2025-09-17T08:44:14Z

@mananjadhav Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

roryabraham · 2025-09-17T17:11:53Z

+const isVerbose = process.argv.includes('verbose') || process.env.JEST_VERBOSE === 'true';
+
+if (!isVerbose) {
+    jest.spyOn(console, 'log').mockImplementation(() => {});


Do we need this if we also have it in setupAfterEnv?

yes, I checked and if its only in either of them there is still debug logs.. only when I use it in both of them its clearing all of those.

That shouldn't happen ideally. Have we tried removing this and only keeping setupAfterEnv?

I changed it, as I move the logic to setup.ts

mananjadhav

Left a few comments.

mananjadhav · 2025-09-18T16:35:03Z

+    jest.spyOn(console, 'log').mockImplementation(() => {});
+    jest.spyOn(console, 'info').mockImplementation(() => {});
+    jest.spyOn(console, 'debug').mockImplementation(() => {});
+    jest.spyOn(console, 'warn').mockImplementation(() => {});


I understand hiding others but do we really want to hide warn as well?

cc - @roryabraham

No strong opinion. I think it would make sense to hide log, info, debug, but not warn or error

Then again, I think if you want to see more detail, you run with --verbose. In CI we really only want to see error I guess. I think this is fine as-is

Okay thanks for confirming.

mananjadhav · 2025-09-18T16:37:05Z

-    // eslint-disable-next-line no-console
-    console.log('DEBUG', ...params);
-});
+const isVerbose = process.argv.includes('verbose') || process.env.JEST_VERBOSE === 'true';


Does it by any chance conflict with Jest verbose option? I don't have a specific use case in my head but do you think it would cause issue?

I removed process.argv.includes('verbose') anyway. I didn't come up with use case as well.
@roryabraham what do you think ?

mananjadhav · 2025-09-18T16:43:11Z

-    // eslint-disable-next-line no-console
-    console.log('DEBUG', ...params);
-});
+const isVerbose = process.argv.includes('verbose') || process.env.JEST_VERBOSE === 'true';


Other places we're using --verbose and here only verbose?

I double checked and now it should be ok

mananjadhav · 2025-09-18T16:49:10Z

+
+const isVerbose = process.argv.includes('--verbose') || process.env.JEST_VERBOSE === 'true';
+
+beforeEach(() => {


I have 2 concerns with using beforeEach.

Essentially this will be executing before every test and it's kinda like an overhead for something that is to be done global? We generally use beforeEach for isolation so that each test gets its own fresh setup

If you globally mock in beforeEach, then restoring for just one test requires extra juggling. This might be required when someone is testing it locally may be a bunch of tests?

Im now using only setup.ts, the code from this file has been removed.

mananjadhav · 2025-09-19T17:08:40Z

@@ -40,16 +41,24 @@ jest.mock('react-native/Libraries/LogBox/LogBox', () => ({
    },
 }));

-// Turn off the console logs for timing events. They are not relevant for unit tests and create a lot of noise
-jest.spyOn(console, 'debug').mockImplementation((...params: string[]) => {


@roryabraham These logs were always hidden earlier. Now when verbose is enabled, these logs will be printed. I am assuming we're fine with that? Confirming because of the comment.

They are not relevant for unit tests and create a lot of noise

mananjadhav

Changes look good and much cleaner from before. Just left with one clarifying comment and starting to test now.

mananjadhav · 2025-09-19T17:26:38Z

@elirangoshen I was verifying the result of the test runs but if you check the Jest GH action you'll see core.info is still being logged.

mananjadhav · 2025-09-19T17:45:10Z

@elirangoshen what do you think of mocking instead of spyOn. The reason being jest.resetAllMocks(); is causing the spyOn changes to be reverted. I think it's best we if we mock the core utils. I didn't spend a lot of time but following worked, it gives lint errors though.

if (!isVerbose) {
    jest.mock('@actions/core', () => {
        const originalCore = jest.requireActual('@actions/core'); // this is giving lint error
       
        const createMockFn = () => {
            const mockFn = jest.fn(() => {});
            return mockFn;
        };
        
        // this is also throwing lint error, may be we disable.
        return {
            ...originalCore,
            startGroup: createMockFn(),
            endGroup: createMockFn(),
            group: jest.fn((_title: unknown, fn: unknown) => fn),
            info: createMockFn(),
            setOutput: createMockFn(),
        };
    });
}

elirangoshen · 2025-09-22T09:58:19Z

@elirangoshen what do you think of mocking instead of spyOn. The reason being jest.resetAllMocks(); is causing the spyOn changes to be reverted. I think it's best we if we mock the core utils. I didn't spend a lot of time but following worked, it gives lint errors though.

if (!isVerbose) {
    jest.mock('@actions/core', () => {
        const originalCore = jest.requireActual('@actions/core'); // this is giving lint error
       
        const createMockFn = () => {
            const mockFn = jest.fn(() => {});
            return mockFn;
        };
        
        // this is also throwing lint error, may be we disable.
        return {
            ...originalCore,
            startGroup: createMockFn(),
            endGroup: createMockFn(),
            group: jest.fn((_title: unknown, fn: unknown) => fn),
            info: createMockFn(),
            setOutput: createMockFn(),
        };
    });
}

Great idea. I implemented it it and added ts-ignore as I noticed we do it in other similar places in the tests code. I also double checked that we not getting any other logs except warn or errors.

Could you check it now ?

mananjadhav · 2025-09-22T19:52:40Z

The code is fine, but we've got failing tests because of the changes. @elirangoshen Can you please check those?

elirangoshen · 2025-09-24T09:29:07Z

The code is fine, but we've got failing tests because of the changes. @elirangoshen Can you please check those?

Im currently working on a task with more priority but Ill take a look as soon as I could, I've notice it thanks

roryabraham · 2025-09-29T18:43:16Z

@elirangoshen any updates here? Should we close this and reopen when you're ready?

elirangoshen · 2025-10-01T14:43:22Z

@elirangoshen any updates here? Should we close this and reopen when you're ready?

Ill take a look now.

@elirangoshen any updates here? Should we close this and reopen when you're ready?

Im still working on other issue but today I could work on this one as well.

There's more failing tests and I still couldn't resolve those it seems that each test mocking getInput from core doesn't work.

usually those tests using this command : asMutable(core).getInput = mockGetInput;
If Im trying to comment it in the tests it still not ok as some of have different implementation.

Ill check what else I can do @mananjadhav
@roryabraham since i will be on leave in rest of this week I can check if after I return I will have time to continue it. and if still not than we can close it and reopen later, ok ?

melvin-bot · 2025-10-02T15:12:48Z

npm has a package.json file and a package-lock.json file. It seems you updated one without the other, which is usually a sign of a mistake. If you are updating a package make sure that you update the version in package.json then run npm install

elirangoshen · 2025-10-08T13:56:30Z

@elirangoshen any updates here? Should we close this and reopen when you're ready?

Ill take a look now.

@elirangoshen any updates here? Should we close this and reopen when you're ready?

Im still working on other issue but today I could work on this one as well.

There's more failing tests and I still couldn't resolve those it seems that each test mocking getInput from core doesn't work.

usually those tests using this command : asMutable(core).getInput = mockGetInput; If Im trying to comment it in the tests it still not ok as some of have different implementation.

Ill check what else I can do @mananjadhav @roryabraham since i will be on leave in rest of this week I can check if after I return I will have time to continue it. and if still not than we can close it and reopen later, ok ?

So there's multiple failing tests, mostly one who trying to mock core functions, eg getInput.

Sample of such logs is:

 tests/unit/markPullRequestsAsDeployedTest.ts
  markPullRequestsAsDeployed
    ✕ comments on pull requests correctly for a standard staging deploy (9 ms)
    ✕ comments on pull requests correctly for a standard production deploy (4 ms)
    ✕ comments on pull requests correctly for a cherry pick
    ✕ comments on pull requests correctly when one platform fails

  ● markPullRequestsAsDeployed › comments on pull requests correctly for a standard staging deploy

    TypeError: core.getInput is not a function

      101 |     const mobileExpensifyPRList = Array.isArray(mobileExpensifyPRListInput) ? mobileExpensifyPRListInput.map((num: string) => Number.parseInt(num, 10)) : [];
      102 |     const isProd = ActionUtils.getJSONInput('IS_PRODUCTION_DEPLOY', {required: true}) as boolean;
    > 103 |     const version = core.getInput('DEPLOY_VERSION', {required: true});
          |                          ^

While i'm able to fix it in some of the failing tests files, I'm not able to fix it in al of them, and even if I will I don't think its good idea, as I think its better to modifying only general jest files like setup.ts.

Do you think I can do different implementation , or maybe revert to what I did before as there was not failing tests?
@mananjadhav

When I revert to use .spyOn I notice only extra 10-20 logs which you mentioned earlier :

::group::Fetching paginated commits:
📄 Fetching page 1 of commits...
📊 Total commits: unknown
✅ compareCommits API returned 0 commits for page 1
🎉 Successfully fetched 0 total commits
::endgroup::
::group::Fetching paginated commits:
📄 Fetching page 1 of commits...
📊 Total commits: unknown
✅ compareCommits API returned 1 commits for page 1
🎉 Successfully fetched 1 total commits
::endgroup::
::group::Fetching paginated commits:
📄 Fetching page 1 of commits...
📊 Total commits: unknown
✅ compareCommits API returned 2 commits for page 1
🎉 Successfully fetched 2 total commits
::endgroup::
::group::Fetching paginated commits:
📄 Fetching page 1 of commits...
::endgroup::
::group::Fetching paginated commits:
📄 Fetching page 1 of commits...
::endgroup::

Maybe its minor extra logs and its still better to use spyOn as the rest seems ok ?

@roryabraham maybe we can remove it from WIP ?

roryabraham · 2025-10-10T00:05:47Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified that the composer does not automatically focus or open the keyboard on mobile unless explicitly intended. This includes checking that returning the app from the background does not unexpectedly open the keyboard.
I verified tests pass on all platforms & I tested again on:
- Android: HybridApp
- Android: mWeb Chrome
- iOS: HybridApp
- iOS: mWeb Safari
- MacOS: Chrome / Safari
- MacOS: Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified there are no new alerts related to the canBeMissing param for useOnyx
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.ts or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
- I verified that all the inputs inside a form are aligned with each other.
- I added Design label and/or tagged @Expensify/design so the design team can review the changes.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
For any bug fix or new feature in this PR, I verified that sufficient unit tests are included to prevent regressions in this flow.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Android: HybridApp

Android: mWeb Chrome

iOS: HybridApp

iOS: mWeb Safari

MacOS: Chrome / Safari

MacOS: Desktop

OSBotify · 2025-10-10T00:17:28Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2025-10-10T11:11:46Z

🚀 Deployed to staging by https://github.com/roryabraham in version: 9.2.29-0 🚀

platform	result
🖥 desktop 🖥	success ✅
🕸 web 🕸	success ✅
🤖 android 🤖	success ✅
🍎 iOS 🍎	success ✅

OSBotify · 2025-10-13T20:58:44Z

🚀 Deployed to production by https://github.com/mountiny in version: 9.2.29-5 🚀

platform	result
🖥 desktop 🖥	success ✅
🕸 web 🕸	success ✅
🤖 android 🤖	success ✅
🍎 iOS 🍎	success ✅

melvin-bot Bot assigned elirangoshen Sep 8, 2025

elirangoshen force-pushed the eliran/perf/disable-logs-CIGitLogic branch from 9187c7a to 4add60f Compare September 10, 2025 08:35

CLABotify added a commit to Expensify/CLA that referenced this pull request Sep 15, 2025

@elirangoshen has signed the CLA in Expensify/App#70095

f931860

elirangoshen changed the title ~~Clearing logs in unit tests~~ [NO QA] Clearing logs in unit tests Sep 15, 2025

elirangoshen changed the title ~~[NO QA] Clearing logs in unit tests~~ [No Qa] Clearing logs in unit tests Sep 15, 2025

elirangoshen changed the title ~~[No Qa] Clearing logs in unit tests~~ [No QA] Clearing logs in unit tests Sep 15, 2025

elirangoshen force-pushed the eliran/perf/disable-logs-CIGitLogic branch from 50804d2 to 438ba69 Compare September 15, 2025 09:48

elirangoshen marked this pull request as ready for review September 17, 2025 08:44

elirangoshen requested a review from a team as a code owner September 17, 2025 08:44

melvin-bot Bot requested review from mananjadhav and removed request for a team September 17, 2025 08:44

roryabraham reviewed Sep 17, 2025

View reviewed changes

elirangoshen force-pushed the eliran/perf/disable-logs-CIGitLogic branch from 438ba69 to dc12a41 Compare September 18, 2025 08:32

elirangoshen requested a review from roryabraham September 18, 2025 14:53

mananjadhav reviewed Sep 18, 2025

View reviewed changes

mananjadhav reviewed Sep 19, 2025

View reviewed changes

mananjadhav approved these changes Sep 19, 2025

View reviewed changes

elirangoshen added 2 commits October 1, 2025 14:56

clearing logs

b53c008

reverting error logs visibility

7c39b79

elirangoshen added 6 commits October 1, 2025 14:56

added verbose to show all logs

7081722

fix prettier

913b73f

pr fixes

16e8536

moving log mock to setup,ts

98e2e43

verbose fix

b8de7af

pr fix

46712f4

elirangoshen force-pushed the eliran/perf/disable-logs-CIGitLogic branch from ebabf30 to 46712f4 Compare October 1, 2025 12:57

roryabraham changed the title ~~[No QA] Clearing logs in unit tests~~ [WIP][No QA] Clearing logs in unit tests Oct 2, 2025

revert to using spyOn

548af11

roryabraham changed the title ~~[WIP][No QA] Clearing logs in unit tests~~ [No QA] Clearing logs in unit tests Oct 10, 2025

roryabraham approved these changes Oct 10, 2025

View reviewed changes

roryabraham merged commit b6ece5f into Expensify:main Oct 10, 2025
27 of 83 checks passed

OSBotify mentioned this pull request Oct 10, 2025

Deploy Checklist: New Expensify 2025-10-10 #72300

Closed

81 tasks

situchan mentioned this pull request Oct 29, 2025

[Due for payment 2025-10-28] Investigate account performance with many expenses to submit and approve #72025

Closed


		const isVerbose = process.argv.includes('--verbose') \|\| process.env.JEST_VERBOSE === 'true';

		beforeEach(() => {

Conversation

elirangoshen commented Sep 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Explanation of Change

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

Uh oh!

github-actions Bot commented Sep 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Sep 10, 2025

Uh oh!

elirangoshen commented Sep 15, 2025

Uh oh!

melvin-bot Bot commented Sep 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mananjadhav left a comment

Choose a reason for hiding this comment

Uh oh!

mananjadhav Sep 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mananjadhav left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mananjadhav commented Sep 19, 2025

Uh oh!

mananjadhav commented Sep 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elirangoshen commented Sep 22, 2025

Uh oh!

mananjadhav commented Sep 22, 2025

Uh oh!

elirangoshen commented Sep 24, 2025

Uh oh!

roryabraham commented Sep 29, 2025

Uh oh!

elirangoshen commented Oct 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

melvin-bot Bot commented Oct 2, 2025

Uh oh!

elirangoshen commented Oct 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

elirangoshen commented Sep 8, 2025 •

edited

Loading

github-actions Bot commented Sep 8, 2025 •

edited

Loading

mananjadhav Sep 18, 2025 •

edited

Loading

mananjadhav left a comment •

edited

Loading

mananjadhav commented Sep 19, 2025 •

edited

Loading

elirangoshen commented Oct 1, 2025 •

edited

Loading

elirangoshen commented Oct 8, 2025 •

edited

Loading