refactor: mock interval in transaction success step #475
Conversation
| "https://ark-test.arkvault.io/api/transactions/ea63bf9a4b3eaf75a1dfff721967c45dce64eb7facf1aef29461868681b5c79b", | ||
| transactionsFixture, | ||
| ), | ||
| ); |
There was a problem hiding this comment.
As I mentioned earlier, this will bite us later, arkvault should not be aware of PSDK internals, PSDK should be free to change implementation details and when that happens this change either will be obsolete or make tests fail which is far from ideal.
There was a problem hiding this comment.
@shahin-hq by adding a server mock, we are also ensuring that psdk functions properly and we are not muting a potential problem when signing or broadcasting. This is the approach that has been taken in vault, and it has indeed caught a lot of issues with sdk. By mocking the useTransactionConfirmation we are practically muting the problem, and would not be able to spot those issues, as we assume that psdk works fine.
There was a problem hiding this comment.
It is not responsibility of arkvault to ensure PSDK functions properly, it is a unit test and should be limited to the SUT, it will introduce more breaking changes when we change stuff in PSDK, at the end of the day PSDK is a library and it has own tests, it is not up to consumer to ensure its behavior..
Also mocking it is not muting problem it is called isolating side factors, you have to rule out some factors when doing unit tests otherwise changing a single function implementation would cause hundreds of tests fail
There was a problem hiding this comment.
@shahin-hq in theory I agree with you, but practically in the case of arkvault, psdk was always tested through arkvault as it's the only consumer, although it's technically another package.
In arkvault we cover numerous cases that have been thoroughly tested and proven by time, and we have full unit coverage as well as e2e tests that cover a lot more (in terms of flows & edge cases) that are not handled in psdk. The approach from the beginning was to mock only the api requests (both for unit & e2e) so that psdk changes that are breaking could be foreseen or caught in arkvault as final protection layer before they go to production.
It would be fine to mock useTransactionConfirmation if there were tests that were handling all the transaction types. But if you look at the file there aren't any, there are just a couple of tests that check isConfirmed state
To give you another important example. There were recent breaking changes in psdk beta (develop branch) to support CSP, and have precompiled standalone tx schema validators, which is a breaking change. Arkvault is the only app that this can be tested, both in terms of QA but also in tests. If we mock psdk, we are losing the ability to catch important edge cases in transaction schema validations in arkvault that were probably missed in psdk. Something that I think we are going to see it in tests still because they are not fixed in feat/mainsail branch that uses psdk beta.
There was a problem hiding this comment.
As a matter of fact, it indeed happened with CSP as mentioned. https://github.com/ArdentHQ/arkvault/actions/runs/8801725477/job/24155794355?pr=472#step:9:10428 arkvault caught this validation error in transactions, which was due to a bug in psdk.
vitest.setup.ts
Outdated
| @@ -15,6 +15,10 @@ vi.mock("@/utils/debounce", () => ({ | |||
| debounceAsync: (promise) => promise, | |||
| })); | |||
|
|
|||
| vi.mock("@/utils/interval", () => ({ | |||
| interval: (callback: () => void) => callback(), | |||
| })); | |||
There was a problem hiding this comment.
I think adding this has a few implications:
- This leaks implementation details of
useConfirmedTransaction, when implementation of hook changes this will provide no value; besides having this along HTTP call mock makes stuff even worse - Makes tests hard to reason, if I had to work on the
SendTransfertests having this hidden configs/overrides would take time to discover - Having
intervaloversetIntervalfor the sake of tests doesn't make sense to me..
There was a problem hiding this comment.
I see what you mean. The reason I moved it to a dedicated utility function was to have it centrally for better control instead of calling them here n there, it's not only for tests (think of debounce et.al). Plus that it makes it very easy to mock them in tests, instead of using fake timers. We do the same with "setTimeout" in ./src/utils/delay.ts, but I'll remove it, sure, and add fake timers. it's not an issue.
Summary
useTransactionConfirmationand uses fake timers instead (see discussion test: fix failing tests #471 (comment))All the server mocks & fixtures are in place, so it shouldn't affect the tests.
Checklist