DTPPSDK-275: Fix hashStr function and unit test coverage #141
Conversation
borodovisin
changed the title
borodovisin
changed the title
Codecov Report
@@ Coverage Diff @@
## main #141 +/- ##
==========================================
+ Coverage 94.40% 94.51% +0.10%
==========================================
Files 7 7
Lines 161 164 +3
Branches 55 56 +1
==========================================
+ Hits 152 155 +3
Misses 9 9
Continue to review full report at Codecov.
|
src/utils.test.js
Outdated
|
|
||
| describe("hashStr", () => { | ||
| test("should match the hash from the argument string", () => { | ||
| expect(hashStr("react")).toMatchSnapshot(); |
There was a problem hiding this comment.
Sorry to be a bit picky on this comment, but generally speaking for tests like this I'd almost rather just see the expected string inline vs in a snapshot. I find when everything is contained in one file its easier to quickly see expected behavior. Given that these are shorter strings I think inlining would be easier than snapshots, but I am totally open to whatever folks feel is best!
There was a problem hiding this comment.
Sure I can use the toMatchInlineSnapshot because I'm calling a hash function and don't know from the top of my head the result. Let me know if it is better in that way...
|
|
||
| describe("hashStr", () => { | ||
| test("should match the hash from the argument string", () => { | ||
| expect(hashStr("react")).toMatchInlineSnapshot(`"xxhjw"`); |
There was a problem hiding this comment.
TIL about Inline Snapshots. Very cool!
| for (let i = 0; i < str.length; i++) { | ||
| hash += str[i].charCodeAt(0) * Math.pow((i % 10) + 1, 5); | ||
| let total = str[i].charCodeAt(0) * i; |
There was a problem hiding this comment.
This does change the characteristics of the generated hash. I just think we should double check that it we are okay with that and that we don't forsee any issues.
| */ | ||
| export function hashStr(str: string): number { | ||
| let hash = 0; | ||
| export function hashStr(str: string): string { |
There was a problem hiding this comment.
I know there were some previous discussions around the hashStr function in belter which this is sourced from. Maybe we can make an issue in belter to acknowledge there might be overflow problems with that method of generating a hash
|
|
||
| describe("hashStr", () => { | ||
| test("should match the hash from the argument string", () => { | ||
| expect(hashStr("react")).toMatchInlineSnapshot(`"xxhjw"`); |
There was a problem hiding this comment.
the old thread got marked outdated, but i dont think you need the snapshot methods at all since the hashing is deterministic right? so you could just have expect(hashStr("react")).toEqual("xxhjw"); correct?
There was a problem hiding this comment.
Yeah, that is right, the only think I do not create the string, that was generate from the jest library, because that I not use the toEqual. To make that I need to run the function externally and copy the result.
The 'data-react-paypal-script-id' attribute uses the hash value from this `hashStr(JSON.stringify(options))` function. This change fixes an issue where a large `options` object could cause a numeric overflow.
Fixed overflow problem with the hashStr function and unit test coverage