Update cvat payout calculator for the new manifest#3981
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 5 Skipped Deployments
|
dnechay
left a comment
dnechay
left a comment
There was a problem hiding this comment.
Lgtm overall, some cleanup comments
| const mockedGetReservedFunds = jest | ||
| .fn() | ||
| .mockImplementation(async () => | ||
| BigInt(faker.number.int({ min: 1000, max: 100000 })), |
There was a problem hiding this comment.
Let's not mock default with some random number, because it can confuse in case of incorrect/failing test suite, it's better to mimic real behavior (i.e. funds not reserved) which returns 0n
| BigInt(faker.number.int({ min: 1000, max: 100000 })), | |
| 0n, |
There was a problem hiding this comment.
I dont understand why you want to return 0 here. We use reserved funds to calculate the job bounty
There was a problem hiding this comment.
@portuu3
Here you just declare a mock to be used later, so there can define actual mocked values.
By default it should return something, that will make test:
- pass, where reserved funds aren't necessary
- fail, where you have to override
Setting random value by default satisfies both, but in case something goes wrong - e.g. you forget to set mockResolvedValueOnce in the the test where it actually matters - you will get failed assertions with weird calculations of job bounty something like expected 1234.34, got 45834.3, but in case we set 0 as default, then it would be expected 1234.34, got 0 because of the math, which makes it clear that we just forgot to mock reserved funds call return value
3 participants
Issue tracking
NA
Context behind the change
Update cvat payout calculator for the new manifest
How has this been tested?
Tested using unit tests
Pending to test with CVAT oracles on testnet when ready
Release plan
NA
Potential risks; What to monitor; Rollback plan
NA