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

TS Integration Tests Framework #1244

Merged
merged 142 commits into from
Aug 1, 2024
Merged

TS Integration Tests Framework #1244

merged 142 commits into from
Aug 1, 2024

Conversation

assafmo
Copy link
Contributor

@assafmo assafmo commented Jul 19, 2024

Adds a framework for writing integration tests using typescript, stridejs & dockernet.

assafmo and others added 30 commits July 11, 2024 13:03
- codec
- msg server
- keeper skeleton
- query server skeleton
- genesis
- GetAirdropRecords
- SetAirdropRecords
- GetAllocationRecords
- SetAllocationRecords
assafmo added 5 commits July 21, 2024 16:05
- rename test case to 'MsgCreateAirdrop' for clarity
- set broadcastPollIntervalMs to 100ms for faster tests
@github-actions github-actions bot added the T:CI label Jul 22, 2024
Copy link
Collaborator

@sampocs sampocs left a comment

Choose a reason for hiding this comment

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

These are BEAUTIFUL!! So excited to finally switch over to this, this will be a HUGE lift!

Mostly nit comments and then I would just gut the dockernet + CI changes and then lets merge this guy in!

.github/workflows/integration-tests.yml Outdated Show resolved Hide resolved
dockernet/config/relayer_config_ics.yaml Outdated Show resolved Hide resolved
dockernet/ts-tests/.gitignore Outdated Show resolved Hide resolved
dockernet/ts-tests/README.md Show resolved Hide resolved
dockernet/ts-tests/package.json Show resolved Hide resolved
dockernet/ts-tests/test/main.test.ts Outdated Show resolved Hide resolved
dockernet/ts-tests/test/main.test.ts Outdated Show resolved Hide resolved
while (true) {
try {
const block =
await accounts.user.query.cosmos.base.tendermint.v1beta1.getLatestBlock(
Copy link
Collaborator

Choose a reason for hiding this comment

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

do you have to execute all queries through a user like this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes though it's the same query client. we can export it to a global query variable for these tests, but in general I've learned from secret that it's more convenient to have the query and tx clients under the same object

Copy link
Contributor Author

Choose a reason for hiding this comment

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

open to more feedback though

Copy link
Collaborator

Choose a reason for hiding this comment

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

Gotchya - let's keep it for now and we can iterate on it!

Fwiw, the global query option sounds like a good option. I setup firehose with the query/tx client on the user and always felt like it was a bit of an antipattern

Copy link
Collaborator

Choose a reason for hiding this comment

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

just curious - what's the benefit you noticed from secret in having it be shared?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you just have the one StrideClient object, you can traverse it just like the cli
stridejs.query.bank.balance...
stridejs.tx.bank.send...

we've got a lot of good feedback on this after multiple iterations, but mostly that it's nice that you can explore the object with the ide's intelisense like you would explore the cli with typing half a command and appendig --help

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah i see, yeah that makes a lot of sense! So do you normally just add stridejs = accounts.X to the top of the test?

What are the different ways to send a tx? Cause I don't see anything txs in these tests like stridejs.tx.bank.send

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So do you normally just add stridejs = accounts.X to the top of the test?

I do, but it's a matter of personal preference.

What are the different ways to send a tx?

currently you can only create a message object and send it using signAndBroadcast(), but I plan to add a function for each message.

For example, you'd be able to send a tx like that:

stridejs.tx.bank.send({from:x,to:y,amount:"1ustrd"})

or if you want to send multiple messages in the same tx then do something like that:

const msg1 = new MsgSend({from:x,to:y,amount:"1ustrd"})
const msg2 = new MsgSend({from:x,to:z,amount:"2ustrd"})
stridejs.tx.signAndBroadcast([msg1,msg2])

Copy link
Collaborator

@sampocs sampocs Aug 1, 2024

Choose a reason for hiding this comment

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

nice, make sense! I do think it breaks down a bit though when you have multiple accounts that you're sending txs from in one test. In which case, I could imagine it being cleaner to have a separate global query config

query.bank...
accounts.X.tx.bank.send(...)
accounts.Y.tx.bank.send(...)

But just ideating - obviously no need to change anything now!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree

dockernet/ts-tests/test/main.test.ts Show resolved Hide resolved
dockernet/ts-tests/test/main.test.ts Show resolved Hide resolved
@github-actions github-actions bot removed the T:CI label Aug 1, 2024
Copy link
Collaborator

@sampocs sampocs left a comment

Choose a reason for hiding this comment

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

🚢 !

assafmo added a commit to Stride-Labs/stridejs that referenced this pull request Aug 1, 2024
@assafmo assafmo merged commit b6f4d2e into main Aug 1, 2024
10 checks passed
@assafmo assafmo deleted the assaf-airdrop-tests branch August 1, 2024 17:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants