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.

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

Simplify protoanalysis testdata #1546

Open
lumtis opened this issue Sep 6, 2021 · 6 comments
Open

Simplify protoanalysis testdata #1546

lumtis opened this issue Sep 6, 2021 · 6 comments
Labels

Comments

@lumtis
Copy link
Contributor

lumtis commented Sep 6, 2021

Is your feature request related to a problem? Please describe.
The current testdata used to test protoanalysis methods is the entire liquidity module proto sources. Any specific reason to have this test data?
The sources are big and it makes it difficult to maintain the tests when a new feature must be added. For example, the format of the Parse method is modified

Describe the solution you'd like
We should simplify a lot this testdata and still make it reliable for tests.
For example, keeping only 3 txs, 3 queries and 3 messages in liquidity.proto should be already sufficient to test Parse method effectively.

@lumtis lumtis added the type:request Feature request. label Sep 6, 2021
@fadeev
Copy link
Contributor

fadeev commented Sep 6, 2021

The liquidity proto files are written in a different way and we needed to make sure that code gen works with this style of writing proto files. Right now we can basically clone gaia and generate code from there.

@ilgooz
Copy link
Member

ilgooz commented Sep 6, 2021

I agree on Denis, I think it's pretty important to keep a full test that checks everything for complex modules. Otherwise, we'll be basically lowering the test coverage.

@lumtis
Copy link
Contributor Author

lumtis commented Sep 6, 2021

We should detect what are all the edge cases in proto to handle them and have 100% coverage.

liquidity.proto has 10 messages with many many fields. Those messages are most of them very similar in some ways.
The current assertion for messages for Parse is

Messages: []Message{
	{Name: "PoolRecord", Path: "testdata/liquidity/genesis.proto"},
	{Name: "GenesisState", Path: "testdata/liquidity/genesis.proto"},
	{Name: "PoolType", Path: "testdata/liquidity/liquidity.proto"},
	{Name: "Params", Path: "testdata/liquidity/liquidity.proto"},
	{Name: "Pool", Path: "testdata/liquidity/liquidity.proto"},
	{Name: "PoolMetadata", Path: "testdata/liquidity/liquidity.proto"},
	{Name: "PoolMetadataResponse", Path: "testdata/liquidity/liquidity.proto"},
	{Name: "PoolBatch", Path: "testdata/liquidity/liquidity.proto"},
	{Name: "PoolBatchResponse", Path: "testdata/liquidity/liquidity.proto"},
	{Name: "DepositMsgState", Path: "testdata/liquidity/liquidity.proto"},
	{Name: "WithdrawMsgState", Path: "testdata/liquidity/liquidity.proto"},
	{Name: "SwapMsgState", Path: "testdata/liquidity/liquidity.proto"},
	{Name: "QueryLiquidityPoolRequest", Path: "testdata/liquidity/query.proto"},
	{Name: "QueryLiquidityPoolResponse", Path: "testdata/liquidity/query.proto"},
	{Name: "QueryLiquidityPoolBatchRequest", Path: "testdata/liquidity/query.proto"},
	{Name: "QueryLiquidityPoolBatchResponse", Path: "testdata/liquidity/query.proto"},
	{Name: "QueryLiquidityPoolsRequest", Path: "testdata/liquidity/query.proto"},
	{Name: "QueryLiquidityPoolsResponse", Path: "testdata/liquidity/query.proto"},
	{Name: "QueryParamsRequest", Path: "testdata/liquidity/query.proto"},
	{Name: "QueryParamsResponse", Path: "testdata/liquidity/query.proto"},
	{Name: "QueryPoolBatchSwapMsgsRequest", Path: "testdata/liquidity/query.proto"},
	{Name: "QueryPoolBatchSwapMsgRequest", Path: "testdata/liquidity/query.proto"},
	{Name: "QueryPoolBatchSwapMsgsResponse", Path: "testdata/liquidity/query.proto"},
	{Name: "QueryPoolBatchSwapMsgResponse", Path: "testdata/liquidity/query.proto"},
	{Name: "QueryPoolBatchDepositMsgsRequest", Path: "testdata/liquidity/query.proto"},
	{Name: "QueryPoolBatchDepositMsgRequest", Path: "testdata/liquidity/query.proto"},
	{Name: "QueryPoolBatchDepositMsgsResponse", Path: "testdata/liquidity/query.proto"},
	{Name: "QueryPoolBatchDepositMsgResponse", Path: "testdata/liquidity/query.proto"},
	{Name: "QueryPoolBatchWithdrawMsgsRequest", Path: "testdata/liquidity/query.proto"},
	{Name: "QueryPoolBatchWithdrawMsgRequest", Path: "testdata/liquidity/query.proto"},
	{Name: "QueryPoolBatchWithdrawMsgsResponse", Path: "testdata/liquidity/query.proto"},
	{Name: "QueryPoolBatchWithdrawMsgResponse", Path: "testdata/liquidity/query.proto"},
	{Name: "MsgCreatePool", Path: "testdata/liquidity/tx.proto"},
	{Name: "MsgCreatePoolRequest", Path: "testdata/liquidity/tx.proto"},
	{Name: "MsgCreatePoolResponse", Path: "testdata/liquidity/tx.proto"},
	{Name: "MsgDepositWithinBatch", Path: "testdata/liquidity/tx.proto"},
	{Name: "MsgDepositWithinBatchRequest", Path: "testdata/liquidity/tx.proto"},
	{Name: "MsgDepositWithinBatchResponse", Path: "testdata/liquidity/tx.proto"},
	{Name: "MsgWithdrawWithinBatch", Path: "testdata/liquidity/tx.proto"},
	{Name: "MsgWithdrawWithinBatchRequest", Path: "testdata/liquidity/tx.proto"},
	{Name: "MsgWithdrawWithinBatchResponse", Path: "testdata/liquidity/tx.proto"},
	{Name: "MsgSwapWithinBatch", Path: "testdata/liquidity/tx.proto"},
	{Name: "MsgSwapWithinBatchRequest", Path: "testdata/liquidity/tx.proto"},
	{Name: "MsgSwapWithinBatchResponse", Path: "testdata/liquidity/tx.proto"},
	{Name: "BaseReq", Path: "testdata/liquidity/tx.proto"},
	{Name: "Fee", Path: "testdata/liquidity/tx.proto"},
	{Name: "PubKey", Path: "testdata/liquidity/tx.proto"},
	{Name: "Signature", Path: "testdata/liquidity/tx.proto"},
	{Name: "StdTx", Path: "testdata/liquidity/tx.proto"},
},

It's unnecessarily too complex to maintain if we want to extend Parse like parsing fields for messages for example

@ilgooz
Copy link
Member

ilgooz commented Sep 6, 2021

Yes, that also makes sense as long as we cover all the edge cases in liquidity.

@lumtis
Copy link
Contributor Author

lumtis commented Sep 7, 2021

I think we can wait for now, it's not 100% sure we will need to extend those capabilities but it could be if we want to move to scaffolder that doesn't depend on any placeholder in proto

@salmad3 salmad3 added good first issue Good for newcomers type:testing and removed type:request Feature request. needs-triage labels Feb 29, 2024
@salmad3
Copy link
Member

salmad3 commented Feb 29, 2024

Testdata

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants