Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: add advancedOptions to streams #722

Merged
merged 5 commits into from
Oct 5, 2022
Merged

feat: add advancedOptions to streams #722

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
67edad2
feat: add advancedOptions to streams
ErnoW Oct 4, 2022
36a4096
docs: add changeset
ErnoW Oct 4, 2022
0bfe5b0
style: format
ErnoW Oct 4, 2022
dab6ca7
Merge branch 'main' into add-advancedoptions-to-streams
ErnoW Oct 4, 2022
0312b44
test: update test scenarios
ErnoW Oct 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions packages/integration/test/streamApi/createStream.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,33 @@ describe('Create stream', () => {
expect(result.result.chainIds).toContain('0x3');
});

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');
});

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test doesn't check that, the advancedOptions parameter is passed correctly. Check this mock and these tests.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

image

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.

Copy link
Member Author

@ErnoW ErnoW Oct 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, exactly. Maybe there is some package to handle it nicely. But for now the lodash approach is fine too.

it('should not create stream', async () => {
const failedResult = await StreamApi.add({
chains: ['0x3'],
Expand Down
24 changes: 15 additions & 9 deletions packages/streams/src/generated/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ export interface components {
/** Format: double */
gas?: number;
};
IAbi: { [key: string]: components["schemas"]["AbiItem"] };
IAbi: { [key: string]: components["schemas"]["AbiItem"][] };
IERC20Transfer: {
transactionHash: string;
contract: string;
Expand Down Expand Up @@ -300,6 +300,12 @@ export interface components {
* @example {}
*/
StreamsFilter: { [key: string]: unknown };
/** @description Advanced Options for each specific topic */
advancedOptions: {
topic0: string;
filter?: components["schemas"]["StreamsFilter"];
includeNativeTxs?: boolean;
};
StreamsModel: {
/** @description Webhook URL where moralis will send the POST request. */
webhookUrl: string;
Expand All @@ -317,8 +323,8 @@ export interface components {
includeContractLogs?: boolean;
/** @description Include or not include internal transactions defaults to false */
includeInternalTxs?: boolean;
abi?: components["schemas"]["StreamsAbi"] | null;
filter?: components["schemas"]["StreamsFilter"] | null;
abi?: components["schemas"]["StreamsAbi"][] | null;
advancedOptions?: components["schemas"]["advancedOptions"][] | null;
/** @description The ids of the chains for this stream in hex Ex: ["0x1","0x38"] */
chainIds: string[];
/** @description The unique uuid of the stream */
Expand Down Expand Up @@ -356,8 +362,8 @@ export interface components {
includeContractLogs?: boolean;
/** @description Include or not include internal transactions defaults to false */
includeInternalTxs?: boolean;
abi?: components["schemas"]["StreamsAbi"] | null;
filter?: components["schemas"]["StreamsFilter"] | null;
abi?: components["schemas"]["StreamsAbi"][] | null;
advancedOptions?: components["schemas"]["advancedOptions"][] | null;
/** @description The ids of the chains for this stream in hex Ex: ["0x1","0x38"] */
chainIds: string[];
/** @description The unique uuid of the stream */
Expand All @@ -384,8 +390,8 @@ export interface components {
includeContractLogs?: boolean;
/** @description Include or not include internal transactions defaults to false */
includeInternalTxs?: boolean;
abi?: components["schemas"]["StreamsAbi"] | null;
filter?: components["schemas"]["StreamsFilter"] | null;
abi?: components["schemas"]["StreamsAbi"][] | null;
advancedOptions?: components["schemas"]["advancedOptions"][] | null;
/** @description The ids of the chains for this stream in hex Ex: ["0x1","0x38"] */
chainIds: string[];
};
Expand Down Expand Up @@ -413,8 +419,8 @@ export interface components {
includeContractLogs?: boolean;
/** @description Include or not include internal transactions defaults to false */
includeInternalTxs?: boolean;
abi?: components["schemas"]["StreamsAbi"] | null;
filter?: components["schemas"]["StreamsFilter"] | null;
abi?: components["schemas"]["StreamsAbi"][] | null;
advancedOptions?: components["schemas"]["advancedOptions"][] | null;
/** @description The ids of the chains for this stream in hex Ex: ["0x1","0x38"] */
chainIds?: string[];
};
Expand Down
2 changes: 1 addition & 1 deletion packages/streams/src/resolvers/evmStreams/createStreamEvm.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,9 @@ const bodyParams = [
'includeNativeTxs',
'includeContractLogs',
'includeInternalTxs',
'filter',
'chainIds',
'abi',
'advancedOptions',
] as const;

export interface CreateStreamEvmParams extends Camelize<Omit<ApiParams, 'chainIds'>> {
Expand Down
2 changes: 1 addition & 1 deletion packages/streams/src/resolvers/evmStreams/updateStreamEvm.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@ const bodyParams = [
'includeContractLogs',
'includeInternalTxs',
'abi',
'filter',
'chainIds',
'advancedOptions',
] as const;

export interface UpdateStreamEvmParams extends Camelize<Omit<ApiParams, 'chainIds'>> {
Expand Down