Skip to content
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

Jump to bottom

unit test for filtering accounts #14455

Merged
merged 1 commit into from
Oct 1, 2024
Merged

unit test for filtering accounts #14455

merged 1 commit into from
Oct 1, 2024

Conversation

enjojoy
Copy link
Contributor

@enjojoy enjojoy commented Sep 20, 2024 •
edited
Loading

Description

Test a complex condition in account filtering

Related Issue

Resolve #13978

@enjojoy enjojoy changed the title draft(suite): test fulter accounts draft(suite): test filter accounts Sep 20, 2024
@enjojoy enjojoy force-pushed the test/account-filtering branch 2 times, most recently from 48a50d0 to d578e82 Compare September 20, 2024 14:59
tomasklim
tomasklim requested changes Sep 23, 2024
View reviewed changes
suite-common/wallet-utils/src/accountUtils.ts Outdated Show resolved Hide resolved
suite-common/wallet-utils/src/accountUtils.ts Outdated Show resolved Hide resolved
@enjojoy enjojoy force-pushed the test/account-filtering branch 4 times, most recently from 3d1bf42 to bfb70e8 Compare September 24, 2024 10:23
@enjojoy enjojoy force-pushed the test/account-filtering branch from bfb70e8 to 06b8690 Compare September 24, 2024 10:24
@enjojoy enjojoy marked this pull request as ready for review September 24, 2024 10:24
@enjojoy enjojoy force-pushed the test/account-filtering branch from 06b8690 to d6c8eeb Compare September 24, 2024 10:33
@enjojoy enjojoy changed the title draft(suite): test filter accounts unit test for filtering accounts Sep 24, 2024
@enjojoy enjojoy force-pushed the test/account-filtering branch from d6c8eeb to a073c5d Compare September 24, 2024 10:47
peter-sanderson
peter-sanderson reviewed Sep 24, 2024
View reviewed changes
suite-common/wallet-utils/src/__tests__/accountUtils.test.ts Outdated
@@ -337,4 +340,119 @@ describe('account utils', () => {
account.addresses = enhanceAddresses(accountInfo, account);
expect(account.addresses.change).toEqual([]);
});

it('isDebugOnlyAccountType', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that the test description shall be informative and shall explain what is expected and why:

something like => it('asserts that only ledger and legacy eth accounts are considered as debug, as we don't ..... <real reason why here>' ...

I even think we have something like this in some code style guide.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry, re-requested review the same moment you posted this comment. will do

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will leave the description consistent with the file

Copy link
Contributor

@peter-sanderson peter-sanderson Sep 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't really agree with the argument of consistency in general, as it can be used to stop any improvement.

However, if you separate this isDebugOnlyAccountType (and filterReceiveAccounts) into its own files as Tom asks here the consistency concern won't be the case anyway

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay yes, I extracted it to another file and will reword the descs

@enjojoy enjojoy force-pushed the test/account-filtering branch 4 times, most recently from 97d61ca to 311093e Compare September 25, 2024 11:02
@enjojoy enjojoy force-pushed the test/account-filtering branch from 311093e to a5c0f49 Compare September 25, 2024 11:05
Lemonexe
Lemonexe approved these changes Sep 25, 2024
View reviewed changes
Copy link
Contributor

@Lemonexe Lemonexe left a comment

Choose a reason for hiding this comment

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

As far as I'm concerned, LGTM 🚀 but please address other reviewers first 🙂
Also it'd be ideal to just slightly expand the tests, see my new comments.

suite-common/wallet-utils/src/__tests__/accountUtils.test.ts Outdated Show resolved Hide resolved
suite-common/wallet-utils/src/__tests__/accountUtils.test.ts Outdated Show resolved Hide resolved
@enjojoy enjojoy force-pushed the test/account-filtering branch 4 times, most recently from ce1d4e4 to 2cd093e Compare September 25, 2024 16:31
tomasklim
tomasklim requested changes Sep 26, 2024
View reviewed changes
suite-common/wallet-utils/src/__tests__/accountUtils.test.ts Outdated Show resolved Hide resolved
suite-common/wallet-utils/src/accountUtils.ts Outdated
@@ -1153,3 +1154,52 @@ export const isTokenMatchesSearch = (token: TokenInfo, search: string) => {
token.policyId?.toLowerCase().includes(search)
);
};

export const isDebugOnlyAccountType = (
Copy link
Member

@tomasklim tomasklim Sep 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

tbh this file is huge, I would put those new methods into a new file and same with tests. It is going to be better also because the fact that there are helper methods and types for the tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I extracted it into a new file

Copy link
Member

Choose a reason for hiding this comment

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

Where?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh sorry, I only extracted tests, will extract this as well🤦🏻‍♀️

suite-common/wallet-utils/src/__tests__/accountUtils.test.ts Outdated Show resolved Hide resolved
suite-common/wallet-utils/src/__tests__/accountUtils.test.ts Outdated Show resolved Hide resolved
suite-common/wallet-utils/src/__tests__/accountUtils.test.ts Outdated Show resolved Hide resolved
tomasklim
tomasklim previously requested changes Sep 26, 2024
View reviewed changes
suite-common/wallet-utils/src/accountUtils.ts Outdated Show resolved Hide resolved
@enjojoy enjojoy force-pushed the test/account-filtering branch from 2cd093e to 0a991ce Compare September 30, 2024 08:42
@enjojoy enjojoy requested a review from tomasklim September 30, 2024 09:57
@enjojoy enjojoy force-pushed the test/account-filtering branch 4 times, most recently from 1bde8ab to 0891b7a Compare September 30, 2024 20:27
peter-sanderson
peter-sanderson reviewed Oct 1, 2024
View reviewed changes
suite-common/wallet-utils/src/__tests__/filterReceiveAccounts.test.ts Outdated
};

describe('filter receive accounts', () => {
it('check if account is debug only type', () => {
Copy link
Contributor

@peter-sanderson peter-sanderson Oct 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

absolute nitpick:

Suggested change
it('check if account is debug only type', () => {
it('checks if account is debug only type', () => {

peter-sanderson
peter-sanderson reviewed Oct 1, 2024
View reviewed changes
suite-common/wallet-utils/src/__tests__/filterReceiveAccounts.test.ts Outdated
expect(isDebugOnlyAccountType('normal', 'regtest')).toBe(false);
});

it('empty accounts array', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
it('empty accounts array', () => {
it('returns no results, when no accounts are given', () => {

peter-sanderson
peter-sanderson reviewed Oct 1, 2024
View reviewed changes
suite-common/wallet-utils/src/__tests__/filterReceiveAccounts.test.ts Outdated
expect(runFilterReceiveAccouns({ accounts: [] })).toEqual([]);
});

it('when given a non-existing network in acccounts list', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
it('when given a non-existing network in acccounts list', () => {
it('returns no results for non-existing network in acccounts list', () => {

peter-sanderson
peter-sanderson reviewed Oct 1, 2024
View reviewed changes
suite-common/wallet-utils/src/__tests__/filterReceiveAccounts.test.ts Outdated
expect(runFilterReceiveAccouns({ receiveNetwork: 'bnb' })).toEqual([]);
});

it('when debug mode is on', () => {
Copy link
Contributor

@peter-sanderson peter-sanderson Oct 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
it('when debug mode is on', () => {
it('filters only debug accounts, when debug mode is on', () => {

.. and similarly for rest of the its

peter-sanderson
peter-sanderson approved these changes Oct 1, 2024
View reviewed changes
Copy link
Contributor

@peter-sanderson peter-sanderson left a comment

Choose a reason for hiding this comment

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

Good job adding new test 👏

@enjojoy enjojoy force-pushed the test/account-filtering branch from 0891b7a to ed9e076 Compare October 1, 2024 12:41
@enjojoy enjojoy force-pushed the test/account-filtering branch from ed9e076 to 810bda2 Compare October 1, 2024 13:11
@enjojoy
Copy link
Contributor Author

enjojoy commented Oct 1, 2024

Good job adding new test 👏

Thank you, I reworded the its

@enjojoy enjojoy self-assigned this Oct 1, 2024
@enjojoy enjojoy enabled auto-merge (rebase) October 1, 2024 13:17
@enjojoy enjojoy merged commit 7b3e25e into develop Oct 1, 2024
15 of 22 checks passed
@enjojoy enjojoy deleted the test/account-filtering branch October 1, 2024 13:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Lemonexe Lemonexe Lemonexe approved these changes

@peter-sanderson peter-sanderson peter-sanderson approved these changes

@adderpositive adderpositive Awaiting requested review from adderpositive

@tomasklim tomasklim Awaiting requested review from tomasklim

Assignees

@enjojoy enjojoy

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Create unit tests to test cover the complex condition for accounts filtering
4 participants
@enjojoy @Lemonexe @tomasklim @peter-sanderson