feat: better blockchain api mocking #255
Conversation
There was a problem hiding this comment.
Overall, I like this approach. I would improve the default mock functions of __mocked_api though and maybe add some explanation there for non-intuitive mocks. For instance, your DID mock inside Mocktest.spec.ts was lacking information and had a kind of "what is this sorcery"-feeling. Moreover, the order of your explanatory comment was incorrect.
I will leave some comments at specific lines and submit my proposal for changes. Feel free to discard them as you wish.
src/blockchain/Mocktest.spec.ts
Outdated
| new Option( | ||
| Tuple, | ||
| new Tuple( | ||
| // (publicBoxKey, publicSigningKey, documentStore?) |
There was a problem hiding this comment.
Order is publicSigningKey, publicBoxKey, documentStore
There was a problem hiding this comment.
Oh, thanks, good catch! I copied that from somewhere, I'll try to keep this in mind for later.
| } | ||
|
|
||
| function __getMockSubmittableExtrinsic(): SubmittableExtrinsic { | ||
| const result: SubmittableResult = TxresultsQueue.shift() || defaultTxResult |
There was a problem hiding this comment.
Would replace TxresultsQueue with TxResultsQueue
src/blockchain/Mocktest.spec.ts
Outdated
| expect(result).toMatchObject({ isFinalized: true }) | ||
| }) | ||
|
|
||
| it('tx succeeds then fails', async () => { |
There was a problem hiding this comment.
Would also add a single test for failed transaction, just to keep it simple.
| children: jest.fn((id: string) => new Vec(Text, [])), | ||
| }, | ||
| did: { | ||
| dIDs: jest.fn(id => new Option(Tuple)), |
There was a problem hiding this comment.
I will submit a proposal for a default query here. There, one can add explanatory comments and reference to this in custom mocks.
There was a problem hiding this comment.
My approach was to return null result queries by default (what would be returned by a query for something that is not there), because it seems odd to return a specific result as a default value. Would prefer to stick to that approach, but you're right that documentation and example cases must be given. I could comment this out and leave it as an example return value, and also add similar examples to the other queries?
There was a problem hiding this comment.
Makes sense and I like your proposal.
There was a problem hiding this comment.
As for documentation on how to set return values, I could also add a comment at the top of the mock or file that illustrates this once.
There was a problem hiding this comment.
Sounds good if it's not too obvious as in this case!
|
@wischli Thank you for your thorough review! Just so that we're on the same page: I added the |
Do as you wish. My suggestions can be treated as refactor hints 😇 |
|
@wischli I added documentation, is this better understandable now? |
|
| ) */ | ||
|
|
||
| // default return value decodes to [], represents: no children found | ||
| children: jest.fn((id: string) => new Vec(H256, [])), |
There was a problem hiding this comment.
| children: jest.fn((id: string) => new Vec(H256, [])), | |
| children: jest.fn((id: string) => new Vec(H256)), |
src/blockchainApiConnection/__mocks__/BlockchainApiConnection.ts
Outdated
Show resolved
Hide resolved
src/blockchainApiConnection/__mocks__/BlockchainApiConnection.ts
Outdated
Show resolved
Hide resolved
NO-TICKET
This is a suggestion for better mocking of the blockchain api connection. Instead of mocking the entire
Blockchainmodule, we would be able to mock only api calls, thus allowing us to use the original functions on the module. We would also only mockgetCachedon theBlockchainApiConnectionmodule to inject our mocked API and retain originals of the remaining functions.I have fixed unit tests that were relying on the old mocks via minimal effort solutions, but the new mocking would allow refactoring to make tests a lot more elegant (and realistic). Not sure whether this should be part of this PR or whether that calls for a ticket - given that this one PR itself wasn't a modification we had planned.
How to test:
unit tests are already using the new mocking.
Checklist: