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

test: account declaration test with katana #734

Merged
merged 2 commits into from
Sep 4, 2024

Conversation

ICavlek
Copy link
Contributor

@ICavlek ICavlek commented Aug 1, 2024

Added tests that check Beerus functions used for account declaration with katana node

  • Restructured common module
  • Added katana node inside the common module
  • Created account declaration workflow test

@ICavlek ICavlek requested a review from a team August 1, 2024 13:17
@ICavlek ICavlek linked an issue Aug 1, 2024 that may be closed by this pull request
@ICavlek ICavlek changed the title integration tests for account declaration test: integration tests for account declaration Aug 2, 2024
@sergey-melnychuk
Copy link
Collaborator

sergey-melnychuk commented Aug 8, 2024

Sorry for the delay, I need a bit more time to understand what is happening in the tests and what exact cases they protect us from.

At this point it seems like a complicated way to verify request-response JSON roundtrip :)

@ICavlek
Copy link
Contributor Author

ICavlek commented Aug 8, 2024

Sorry for the delay, I need a bit more time to understand what is happening in the tests and what exact cases they protect us from.

At this point it seems like a complicated way to verify request-response JSON roundtrip :)

Inside the account_declare.rs is a starkli_declare_workflow_test. If a user would call starkli declare <contract.json> file, these functions would be called on a starknet node. Instead of a live starknet node, I have created a mock node where we can control what kind of response can come.
Additional test cases are for further verification.

For next step, I was thinking of same starkli_declare_workflow_test, but instead of on mock starknet node, we would be starting katana and would query same requests on development node in this test environment. This would be done in scope of this issue:
#732

@sergey-melnychuk
Copy link
Collaborator

sergey-melnychuk commented Aug 19, 2024

Instead of a live starknet node, I have created a mock node where we can control what kind of response can come.

As far as I recall, the goal was to check the integration with the live testnet node, to make sure that production workflow can run through Beerus. If I'm being completely honest, I don't see how mock-based tests check this integration.

For next step, I was thinking of same starkli_declare_workflow_test, but instead of on mock starknet node, we would be starting katana and would query same requests on development node in this test environment.

This should have been the only step, but against live testnet node. We will still need to run the tests against live node, even if tests against mocks and/or katana succeed. And if we have tests that run successfull against the live node, I personally consider extra testing against mocks and/or katana as redundant.

@ICavlek ICavlek force-pushed the ic/test_create_and_deploy_new_account branch 5 times, most recently from cd13825 to 3a21504 Compare August 29, 2024 08:14
@ICavlek ICavlek linked an issue Aug 29, 2024 that may be closed by this pull request
@sergey-melnychuk
Copy link
Collaborator

Another thought on mock tests: #730 (comment)

They might be very useful if we take PRs from external contributors into account.

@ICavlek ICavlek self-assigned this Aug 29, 2024
@ICavlek ICavlek force-pushed the ic/test_create_and_deploy_new_account branch 4 times, most recently from f487bce to bb8fb1c Compare September 4, 2024 08:45
@ICavlek ICavlek changed the title test: integration tests for account declaration test: account declaration test with katana Sep 4, 2024
@ICavlek ICavlek force-pushed the ic/test_create_and_deploy_new_account branch from bb8fb1c to 0b1f46f Compare September 4, 2024 08:53
Cargo.toml Outdated Show resolved Hide resolved
Copy link
Collaborator

@sergey-melnychuk sergey-melnychuk left a comment

Choose a reason for hiding this comment

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

LGTM % please extract CLOBs to files before merging

@ICavlek ICavlek merged commit c209676 into main Sep 4, 2024
7 checks passed
@ICavlek ICavlek deleted the ic/test_create_and_deploy_new_account branch September 4, 2024 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

test: e2e declare account test with Katana
2 participants