[No QA] Improve test cases and comments in OptionsListUtilsTest.ts#61194
Merged
puneetlath merged 4 commits intoMay 5, 2025
Conversation
Contributor
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
rayane-d
reviewed
May 5, 2025
| expect(results.recentReports).not.toEqual(expect.arrayContaining([expect.objectContaining({login: 'receipts@expensify.com'})])); | ||
| }); | ||
|
|
||
| it('should keep admin rooms if specified', () => { |
Contributor
There was a problem hiding this comment.
Is there a reason to remove this test case?
Contributor
Author
There was a problem hiding this comment.
Right, this should not have been removed, I'm bringing it back!
rayane-d
suggested changes
May 5, 2025
| // When we filter in the Search view without providing a searchValue | ||
| const results = getSearchOptions(OPTIONS, [CONST.BETAS.ALL]); | ||
| describe('getSearchOptions()', () => { | ||
| it('should filter options correctly', () => { |
Contributor
There was a problem hiding this comment.
Suggested change
| it('should filter options correctly', () => { | |
| it('should return all options when no search value is provided', () => { |
| personalDetails: [], | ||
| reports: [], | ||
| describe('orderOptions()', () => { | ||
| it('should sort all options correctly', () => { |
Contributor
There was a problem hiding this comment.
Suggested change
| it('should sort all options correctly', () => { | |
| it('should sort options alphabetically and preserves reportID for personal details with existing reports', () => { |
| expect(personalDetailWithExistingReport?.reportID).toBe('2'); | ||
| }); | ||
|
|
||
| it('should sort all options correctly with no reports', () => { |
Contributor
There was a problem hiding this comment.
Suggested change
| it('should sort all options correctly with no reports', () => { | |
| it('should sort personal details options alphabetically when only personal details are provided', () => { |
| // (minus the currently logged in user) | ||
| // Filtering of personalDetails that have reports is done in filterOptions | ||
| describe('getValidOptions()', () => { | ||
| it('should correctly filter when there are no reports or personalDetails', () => { |
Contributor
There was a problem hiding this comment.
Suggested change
| it('should correctly filter when there are no reports or personalDetails', () => { | |
| it('should return empty options when no reports or personal details are provided', () => { |
| expect(results.selfDMChat).toEqual(undefined); | ||
| }); | ||
|
|
||
| it('should include Concierge in results', () => { |
Contributor
There was a problem hiding this comment.
Suggested change
| it('should include Concierge in results', () => { | |
| it('should include Concierge by default in results', () => { |
|
|
||
| // Test by excluding Chronos from the results | ||
| results = getValidOptions({reports: OPTIONS_WITH_CHRONOS.reports, personalDetails: OPTIONS_WITH_CHRONOS.personalDetails}, {excludeLogins: {[CONST.EMAIL.CHRONOS]: true}}); | ||
| it('should exclude Chronos from the results', () => { |
Contributor
There was a problem hiding this comment.
Suggested change
| it('should exclude Chronos from the results', () => { | |
| it('should exclude Chronos from the results when it is specified in excludedLogins', () => { |
|
|
||
| // Test by excluding Receipts from the results | ||
| results = getValidOptions( | ||
| it('should exclude receipts from the results', () => { |
Contributor
There was a problem hiding this comment.
Suggested change
| it('should exclude receipts from the results', () => { | |
| it('should exclude Receipts from the results when it is specified in excludedLogins', () => { |
| includeOwnedWorkspaceChats: true, | ||
| }); | ||
| describe('getShareDestinationsOptions()', () => { | ||
| it('should exclude archived rooms and hidden threads', () => { |
Contributor
There was a problem hiding this comment.
Suggested change
| it('should exclude archived rooms and hidden threads', () => { | |
| it('should exclude archived rooms and hidden threads from share destinations', () => { |
| expect(results.personalDetails).not.toEqual(expect.arrayContaining([expect.objectContaining({login: 'receipts@expensify.com'})])); | ||
| expect(results.recentReports).not.toEqual(expect.arrayContaining([expect.objectContaining({login: 'receipts@expensify.com'})])); | ||
| }); | ||
| it('should return the DMS, the group chats and the workspace room', () => { |
Contributor
There was a problem hiding this comment.
Suggested change
| it('should return the DMS, the group chats and the workspace room', () => { | |
| it('should include DMS, group chats, and workspace rooms in share destinations', () => { |
| // When we only pass personal details | ||
| const results = getMemberInviteOptions(OPTIONS.personalDetails, []); | ||
| describe('getMemberInviteOptions()', () => { | ||
| it('should sort personal details', () => { |
Contributor
There was a problem hiding this comment.
Suggested change
| it('should sort personal details', () => { | |
| it('should sort personal details alphabetically', () => { |
Contributor
Author
|
@rayane-d changes applied, thanks for reviewing this so thoroughly! |
77 tasks
|
@JKobrynski @puneetlath Could you share the QA steps? |
Contributor
|
🚀 Deployed to staging by https://github.com/puneetlath in version: 9.1.40-0 🚀
|
Contributor
|
@kavimuru This should be |
Contributor
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 9.1.40-7 🚀
|
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
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
Fixed Issues
$ #60955
PROPOSAL: N/A
Tests
npm run test tests/unit/OptionsListUtilsTest.tsOffline tests
N/A
QA Steps
Same as Tests section above
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))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.MacOS: Desktop