[NoQA] test: add unit tests for NetSuite credential commands#85288
Merged
madmax330 merged 4 commits intoExpensify:mainfrom Mar 18, 2026
Merged
[NoQA] test: add unit tests for NetSuite credential commands#85288madmax330 merged 4 commits intoExpensify:mainfrom
madmax330 merged 4 commits intoExpensify:mainfrom
Conversation
melvin-bot
bot
requested review from
ShridharGoel
and removed request for
a team
|
@ShridharGoel 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] |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b475176a86
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| // This is the regression scenario: first-time connection failed with bad tokens | ||
| // isAuthenticationError is true but connection is unverified, so we must NOT use updateNetSuiteTokens | ||
| const shouldUseUpdate = isAuthenticationError(policy, CONST.POLICY.CONNECTIONS.NAME.NETSUITE) && !isConnectionUnverified(policy, CONST.POLICY.CONNECTIONS.NAME.NETSUITE); |
There was a problem hiding this comment.
This assertion computes shouldUseUpdate inside the test itself instead of executing the production selection path, so the suite can pass even when app behavior is wrong; for example, NetSuiteTokenInputForm still branches only on isAuthenticationError, which would call updateNetSuiteTokens for unverified connections (the regression case) while this test still passes. Please invoke the real submit logic (or a shared helper used by production code) and assert which command function is called.
Useful? React with 👍 / 👎.
…handling Follow-up to PR Expensify#85200. Adds tests covering: - connectPolicyToNetSuite sends the correct write command and parameters - updateNetSuiteTokens sends the correct write command and parameters - Both commands set optimistic sync progress data - Credential command selection logic: verified connections with auth errors use updateNetSuiteTokens, while unverified connections use connectPolicyToNetSuite (the regression fix from Expensify#85196) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…red helper Follow-up to PR Expensify#85200. Changes: - Extract shouldUseUpdateNetSuiteTokens helper into connections/index.ts, shared by both NetSuiteTokenInputForm and the test suite - Update NetSuiteTokenInputForm to use the shared helper - Add unit tests covering connectPolicyToNetSuite, updateNetSuiteTokens, and the shouldUseUpdateNetSuiteTokens command selection logic Fixes Expensify#85196 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
imgyf
force-pushed
the
imgyf/85196-follow-up-tests
branch
from
b475176 to
c7c711f
Compare
melvin-bot
bot
requested review from
heyjennahay
and removed request for
a team
52 tasks
Contributor
|
TS check is failing |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
…icy> OnyxEntry<T> is defined as T | undefined (not T | null), so the test was passing null which is not assignable to the parameter type. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
Author
|
@ShridharGoel fixed failing TS checks |
ShridharGoel
approved these changes
Mar 18, 2026
Contributor
Author
|
@ShridharGoel missing your PR Reviewer Checklist |
Contributor
|
Thanks for pointing that out |
Contributor
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
Contributor
|
Can you add NoQA in the title? |
ShridharGoel
approved these changes
Mar 18, 2026
imgyf
changed the title
Contributor
Author
|
@ShridharGoel added |
madmax330
approved these changes
Mar 18, 2026
Contributor
|
🚧 @madmax330 has triggered a test Expensify/App build. You can view the workflow run here. |
Contributor
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
Contributor
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
100 tasks
Contributor
|
🚀 Deployed to staging by https://github.com/madmax330 in version: 9.3.40-0 🚀
|
Contributor
|
🚀 Deployed to staging by https://github.com/madmax330 in version: 9.3.40-0 🚀
|
Contributor
|
🚀 Deployed to production by https://github.com/cristipaval in version: 9.3.41-4 🚀
|
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Explanation of Change
Follow-up to PR #85200 (fix for #85196). This PR:
shouldUseUpdateNetSuiteTokenshelper inconnections/index.ts, used by bothNetSuiteTokenInputFormand the test suiteNetSuiteTokenInputFormto use the shared helper instead of inlining the logicTests cover:
connectPolicyToNetSuitesendsConnectPolicyToNetSuitewrite command with correct parameters and optimistic dataupdateNetSuiteTokenssendsUpdateNetSuiteTokenswrite command with correct parameters and optimistic datashouldUseUpdateNetSuiteTokensreturns the correct result for each scenario:false(use full init, the regression case)true(preserve config)falsefalseFixed Issues
$ #85196
Tests
npx jest tests/actions/connections/NetSuite.ts --no-coverageOffline tests
N/A - These are unit tests that run in Jest, not in the app.
QA Steps
[No QA] - Unit tests and a refactor to extract a shared helper. No user-facing behavior change.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
N/A - Unit tests and refactor only
Android: mWeb Chrome
N/A - Unit tests and refactor only
iOS: Native
N/A - Unit tests and refactor only
iOS: mWeb Safari
N/A - Unit tests and refactor only
MacOS: Chrome / Safari
N/A - Unit tests and refactor only