-
Notifications
You must be signed in to change notification settings - Fork 28
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
Improve Test Coverage for packages/web3/src/utils #468
base: master
Are you sure you want to change the base?
Improve Test Coverage for packages/web3/src/utils #468
Conversation
@polarker please review |
Thanks for the contribution. Please make sure all the updated test suite pass, right now all of them have failing tests. You can run them by
|
Alright ser |
@h0ngcha0 Will update my PR Soon |
|
@h0ngcha0 Please review. |
@@ -22,4 +22,4 @@ export default function djb2(bytes: Uint8Array): number { | |||
hash = (hash << 5) + hash + (bytes[`${i}`] & 0xff) | |||
} | |||
return hash | |||
} | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change of this file is unnecessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
@@ -34,7 +34,7 @@ | |||
"author": "Alephium dev <[email protected]>", | |||
"config": { | |||
"alephium_version": "3.9.0", | |||
"explorer_backend_version": "2.2.6" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this change relevant in this PR? Usually the version is updated when we want to update the explorer backend API file, which doesn't seem to be the case here.
@@ -98,4 +98,4 @@ | |||
"node": ">=14.0.0", | |||
"npm": ">=7.0.0" | |||
} | |||
} | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert the end-of-file change to all updated files, maybe it is a editor config issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was a mistake. will revert it
@@ -36,11 +36,13 @@ export function isBase58(s: string): boolean { | |||
} | |||
|
|||
export function base58ToBytes(s: string): Uint8Array { | |||
if (s === '') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we we should allow empty string here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
}); | ||
}); | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove extra lines.
it('should convert valid base58 strings to bytes', () => { | ||
const result = base58ToBytes('32UWxgjUHwH7P1J61tb12') | ||
expect(result).toBeInstanceOf(Uint8Array) | ||
expect(result.length).toBeGreaterThan(0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we can check bs58.encode(result)
would give us 32UWxgjUHwH7P1J61tb12
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Closes #457
Improve Test Coverage for packages/web3/src/utils
Overview
This pull request significantly enhances the test coverage for the
utils.ts
file and other utility functions in thepackages/web3/src/utils
directory. We've added new test cases, improved existing ones, and created new test files to ensure comprehensive coverage of our utility functions.Main Changes
utils.test.ts
bs58.test.ts
isBase58
function:base58ToBytes
function:bs58
object:djb2.test.ts
djb2
function:subscription.test.ts (New File)
Subscription
class:TestSubscription
to test abstractSubscription
classAdditional Notes
utils.ts
resolve type errors related to theSignatureInput
typesubscription.test.ts
provides thorough coverage for theSubscription
class