ZK-559: (part 2) unit tests for shielder-sdk #90
Conversation
kroist
changed the title
|
| @@ -107,11 +109,11 @@ class MockedConverter implements Converter { | |||
|
|
|||
| class MockedNoteTreeConfig implements NoteTreeConfig { | |||
| treeHeight(): Promise<number> { | |||
| return Promise.resolve(13); | |||
| return Promise.resolve(1); | |||
There was a problem hiding this comment.
nice that we have control over this and get short test values
| ) | ||
| ).toBe(true); | ||
|
|
||
| // merkleRoot should be equal to input merkleRoot |
There was a problem hiding this comment.
nit: These 3 lines are self-explanatory and don't really need comments, do they?
There was a problem hiding this comment.
maybe they don't need comments, but my goal was to comment as much as I can
There was a problem hiding this comment.
I claim that code with more comments is not always better. A comment helps if reading it is faster than reading the code being commented, or if it provides extra information, and this is not the case here.
So that it is not just me: https://medium.com/@bpnorlander/stop-writing-code-comments-28fef5272752
There was a problem hiding this comment.
I'm also a proponent of "good code doesn't have to be commented"
BUT
in such complex unit tests I really believe commenting is good.
There was a problem hiding this comment.
disagreeing here but it's not a big deal, feel free to merge
| @@ -1,7 +1,9 @@ | |||
| import { | |||
There was a problem hiding this comment.
It would be really cool to see not only coverage percentage but also which lines are covered. Do you know if it's easy to do?
There was a problem hiding this comment.
if you run pnpm coverage locally, you can see which lines are uncovered, however it's tricky to add that to report.
There was a problem hiding this comment.
Thanks, it works for me. We probably can also use some vs code extension to overlay this on editor view, but it seems to take some time to configure.
| ); | ||
| expect(isValid).toBe(true); | ||
|
|
||
| // Amount should be equal to input amount |
There was a problem hiding this comment.
nit: I suggest removing the 3 comments as the commented code is self-explanatory.
There was a problem hiding this comment.
want to comment as much as it possible
| ) | ||
| ).toBe(true); | ||
|
|
||
| // merkleRoot should be equal to input merkleRoot |
There was a problem hiding this comment.
nit: these 2 lines are self-explanatory, consider removing the 2 comments
There was a problem hiding this comment.
want to comment as much as it possible
| ); | ||
| expect(isValid).toBe(true); | ||
|
|
||
| // Amount should be equal to input amount |
There was a problem hiding this comment.
nit: the 3 comments below do not add value
There was a problem hiding this comment.
want to keep as commented as possible
No description provided.