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

Add unit tests to blockchain reader #93

Merged
merged 3 commits into from
Dec 21, 2024

Conversation

stephenctw
Copy link
Contributor

No description provided.

@stephenctw stephenctw requested a review from GCdePaula December 18, 2024 13:12
@stephenctw stephenctw self-assigned this Dec 18, 2024
@stephenctw stephenctw force-pushed the feature/blockchain-reader-tests branch from 02fa90d to f8a3d71 Compare December 19, 2024 02:25
Copy link
Collaborator

@GCdePaula GCdePaula left a comment

Choose a reason for hiding this comment

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

Great work! I left a few comments.


async fn spawn_anvil() -> Result<Child> {
// Start the Anvil node
let anvil = Command::new("anvil")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think alloy provides convenience methods to interact with anvil.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it could simplify a lot, such as removing kill_child etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion, I'll look into that.


#[tokio::test]
// note: the tests involve interacting with anvil blockchain and a sqlite3 database, can be a bit time consuming (30+ seconds)
async fn test_readers_sequentially() -> Result<()> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What are the blockers to run everything in parallel?

Copy link
Contributor Author

@stephenctw stephenctw Dec 19, 2024

Choose a reason for hiding this comment

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

I think I was trying to spawn a new anvil process (with fixed port) for each test, but maybe that's not necessary, all tests can share one anvil node.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see there are many ways in the codebase to launch anvil and deploy our smart contracts. Is there maybe a way to unify it?

I think the preferable way is through forge scripts. I believe it's possible to store the state of anvil and load it later:

anvil --dump-state state.json
anvil --load-state state.json

Just an idea, but maybe we could have a build target that generates this "template" (spawn anvil with --dump-state, runs forge script, exits), and all the tests that need an anvil+contracts just run anvil --load-state state.json.

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 really like this idea. I think this can be a separate task/PR from the current one.

@stephenctw stephenctw force-pushed the feature/blockchain-reader-tests branch from f8a3d71 to 12b4c31 Compare December 20, 2024 05:41
Copy link
Collaborator

@GCdePaula GCdePaula left a comment

Choose a reason for hiding this comment

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

LGTM!

@stephenctw stephenctw merged commit e3e151f into main Dec 21, 2024
1 check passed
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.

2 participants