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.

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

test: improved tests for packages/web3/src/address #471

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

SoarinSkySagar
Copy link

resolves #451

@SoarinSkySagar SoarinSkySagar marked this pull request as ready for review November 25, 2024 19:30
@SoarinSkySagar
Copy link
Author

@polarker @kfastov PTAL

@h0ngcha0
Copy link
Member

h0ngcha0 commented Nov 27, 2024

Hey, thanks for the contribution.

Please make sure that the tests pass, right now several of them are failing:

> npm run test -- address.test.ts --collectCoverageOnlyFrom ''
....
Test Suites: 1 failed, 1 total
Tests:       3 failed, 36 passed, 39 total

@SoarinSkySagar
Copy link
Author

PTAL @h0ngcha0

@SoarinSkySagar SoarinSkySagar marked this pull request as draft November 27, 2024 22:53
@SoarinSkySagar SoarinSkySagar marked this pull request as ready for review November 27, 2024 22:53
packages/web3/src/address/address.test.ts Outdated Show resolved Hide resolved
packages/web3/src/address/address.test.ts Outdated Show resolved Hide resolved
packages/web3/src/address/address.test.ts Outdated Show resolved Hide resolved
packages/web3/src/address/address.test.ts Show resolved Hide resolved
packages/web3/src/address/address.test.ts Show resolved Hide resolved
packages/web3/src/address/address.test.ts Show resolved Hide resolved
packages/web3/src/address/address.test.ts Show resolved Hide resolved
packages/web3/src/address/address.test.ts Outdated Show resolved Hide resolved
packages/web3/src/address/address.test.ts Show resolved Hide resolved
packages/web3/src/address/address.test.ts Show resolved Hide resolved
@SoarinSkySagar
Copy link
Author

PTAL @h0ngcha0

packages/web3/src/address/address.test.ts Outdated Show resolved Hide resolved
packages/web3/src/address/address.test.ts Outdated Show resolved Hide resolved
packages/web3/src/address/address.test.ts Outdated Show resolved Hide resolved
packages/web3/src/address/address.test.ts Outdated Show resolved Hide resolved
packages/web3/src/address/address.test.ts Outdated Show resolved Hide resolved
packages/web3/src/address/address.test.ts Outdated Show resolved Hide resolved
@h0ngcha0
Copy link
Member

h0ngcha0 commented Dec 3, 2024

Please run npm run lint -- --fix to fix the formatting of the updated files.

@pragmaxim
Copy link

Wouldn't it be a major improvement of these tests if the literal addresses and contractI-ids were extracted into variables of meaningful name, possibly generating them?

@SoarinSkySagar
Copy link
Author

Wouldn't it be a major improvement of these tests if the literal addresses and contractI-ids were extracted into variables of meaningful name, possibly generating them?

I guess to generate the addresses I would need to import from @alephium/web3-wallet package, which is not possible as we're not able to import rn, as has been my conclusion from our chats on telegram group. So it has been done this way

@SoarinSkySagar
Copy link
Author

Made all the required changes according to your reviews @h0ngcha0 ser, PTAL

@SoarinSkySagar
Copy link
Author

SoarinSkySagar commented Dec 3, 2024

Please run npm run lint -- --fix to fix the formatting of the updated files.

Umm I don't think there are any formatting errors in the updated file, just checked with the command

@pragmaxim
Copy link

I guess to generate the addresses I would need to import from @alephium/web3-wallet package, which is not possible as we're not able to import rn, as has been my conclusion from our chats on telegram group. So it has been done this way

Yes, but I noticed of same string literal to be used 5 times in the same spec which is a perfect candidate for a constant I think

@SoarinSkySagar
Copy link
Author

Yes, but I noticed of same string literal to be used 5 times in the same spec which is a perfect candidate for a constant I think

Done, extracted all repetitive addresses and contractIds into separate variables

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve Test Coverage for packages/web3/src/address
3 participants