feat: add advancedOptions to streams #722
Conversation
🦋 Changeset detectedLatest commit: 0312b44 The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| it('should create a stream with advanced options', async () => { | ||
| const result = await StreamApi.add({ | ||
| chains: ['0x3'], | ||
| tag: 'test', | ||
| description: 'test', | ||
| webhookUrl: 'https://webhook.site/4f1b1b1b-1b1b-4f1b-1b1b-1b1b1b1b1b1b', | ||
| networkType: 'evm', | ||
| advancedOptions: [ | ||
| { | ||
| topic0: 'SomeEvent(address,uint256)', | ||
| filter: { eq: ['myCoolTopic', '0x0000000000000000000000000000000000000000'] }, | ||
| includeNativeTxs: true, | ||
| }, | ||
| ], | ||
| abi: [], | ||
| allAddresses: true, | ||
| includeContractLogs: true, | ||
| includeInternalTxs: true, | ||
| includeNativeTxs: true, | ||
| topic0: ['SomeEvent(address,uint256)'], | ||
| }); | ||
|
|
||
| expect(result).toBeDefined(); | ||
| expect(result).toEqual(expect.objectContaining({})); | ||
| expect(result.result.chainIds).toContain('0x3'); | ||
| }); | ||
|
|
There was a problem hiding this comment.
This test doesn't check that, the advancedOptions parameter is passed correctly. Check this mock and these tests.
There was a problem hiding this comment.
Yeah that is correct. But I'm not sure if this is the correct approach.
This will not test any logic in the code. It adds extra logic in the mock, and we test that that logic works.
With mocking apis like this we can only properly test:
- Input: make sure that the input is parsed correctly (via our resolvers)
- Output: different kinds of outputs should be parsed correctly
- Errors: different kinds of errors should be handled correctly
There is no good way for us with these tests to test that certain inputs will return certain outputs.
If we want to test that, then we need to look into other testing methods, where we call the actual API. which is something worth doing
There was a problem hiding this comment.
This will not test any logic in the code. It adds extra logic in the mock, and we test that that logic works.
I think my current implementation is too complex and the main idea is not obvious. So I've deleted all conditions for these tests and now it should be more clear. Check this pull request.
There is no good way for us with these tests to test that certain inputs will return certain outputs.
IMO this claim is incorrect. Because the main idea of integration tests is that, we want to check if a specific input (or some sequence) returns a specific output. Especially for stateless REST services.
So, we should test scenarios/cases. And of course we can test many scenarios. Basically, we should have the same number of jest tests as we have dedicated mocks in the mock server.
This is common practice. And I can achieve it not only by a code. For example, I've created 3 test mocks for the getTokenPrice endpoint using the Postman. I can use the Postman mock server to create 3 integration tests.
Check these URLS:
https://ff0c1798-b3ac-4987-b9f4-58cab995a194.mock.pstmn.io/erc20/0x0000000000000000000000000000000000000001/price?chain=0x1
https://ff0c1798-b3ac-4987-b9f4-58cab995a194.mock.pstmn.io/erc20/0x0000000000000000000000000000000000000002/price?chain=0x13881
https://ff0c1798-b3ac-4987-b9f4-58cab995a194.mock.pstmn.io/erc20/0x0000000000000000000000000000000000000003/price?chain=0x1&to_block=512
This approach is compatible with the "Red, Green, Refactor". Because when you get a bug, at the beginning you should write a "red" test. After this you should fix your code and make the test green. This is exactly that what I've done in this pull request.
After all, I think the main reason why we need tests/integration tests is that, we want to have a proof that the code works properly. And my remarks refer to that, I don't see the proof that, the advancedOptions parameter is passed to the backend service.
There was a problem hiding this comment.
Ok I see now what you mean. Yeah, this way we can check if the params are correctly provided via check on the server. We should keep the logic minimal though. I like the idea of 'scenarios' the most. I did a similar approach with some other tests.
There is no good way for us with these tests to test that specific inputs will return certain outputs.
What I mean by this is that we cannot/should not perfectly simulate server behavior.
We can generalize it to specific scenarios like:
{
condition: // all the checks that we verify for, most likely these are url-params, and request body values
response: // returned response data
responseStatus: // returned response status
}
There was a problem hiding this comment.
Yes, exactly. Maybe there is some package to handle it nicely. But for now the lodash approach is fine too.
name: 'Pull request'
about: A new pull request
New Pull Request
Checklist
Issue Description
Add advancedOptions to streams