-
Notifications
You must be signed in to change notification settings - Fork 13
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
feat(iota-indexer): Refactor ExtendedApi tests in indexer to use shared test cluster #3018
feat(iota-indexer): Refactor ExtendedApi tests in indexer to use shared test cluster #3018
Conversation
e0539ec
to
6658ad6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm ✨
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good @tomxey
Left a few comments because I think it can be simplified
@@ -58,10 +63,47 @@ impl ApiTestSetup { | |||
} | |||
} | |||
|
|||
pub struct SimulacrumApiTestEnvDefinition { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find it hard to reason about the semantics and the necessity of this type.
Why not initializing directly the InitializedSimulacrumEnv
passing a Simulacrum
value and not a closure that produces it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason was that originally I wanted to make instances of this type global, have the initialized env also stored inside of it, and initialize env like:
let ... = env_definition_instance.get_or_init()
To better show that all the params and the env are tied together.
I lost a fight with the Rust ownership checking system and couldn't make such variable global though.
Finally I settled with approach where the coupling is 'guaranteed' by having a dedicated function for every environment. In this case this type is really not needed I think. I will proceed with removing it.
Having the closure that produces the Simulacrum value it is possible for initialization to happen only once, in case the env already exists then the closure is just not called. It seems cleaner that way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pub env_initializer: Box<dyn Fn() -> Simulacrum>, | ||
} | ||
|
||
pub struct InitializedSimulacrumEnv { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name is a bit confusing to me. With this present it would seem plausible to me to have two setups:
ApiTestSetup -> ClusterTestSetup
SimulacrumTestSetup
pub struct InitializedSimulacrumEnv { | |
pub struct ClusterTestSetup { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can rename to SimulacrumTestSetup
.
This name was like this to distinguish the already Initialized
env from the EnvDefinition
that is yet to be initialized. But since we are removing the SimulacrumApiTestEnvDefinition
type we can also rename this one to something simpler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
5d26c9c
to
c81f663
Compare
3e109c6
to
e56aaa5
Compare
This pull request has been deployed to Vercel. Latest commit: e56aaa5 ✅ Preview: https://apps-ui-qiuohxype-iota1.vercel.app |
This pull request has been deployed to Vercel. Latest commit: e56aaa5 ✅ Preview: https://iota-rebased-explorer-092603f1de8088243ccb8b861b23e-ez1hbggqy.vercel.app |
This pull request has been deployed to Vercel. Latest commit: e56aaa5 ✅ Preview: https://apps-backend-29a719c9ccbdb9b80c97b599a599d8953354da-9j454sddn.vercel.app |
This pull request has been deployed to Vercel. Latest commit: e56aaa5 ✅ Preview: https://wallet-dashboard-a225fcfa1e2f852205a61a7cd1d5d93e92-c3fbw7tnz.vercel.app |
@@ -4,7 +4,7 @@ | |||
#[allow(dead_code)] | |||
#[path = "../common/mod.rs"] | |||
mod common; | |||
#[cfg(feature = "pg_integration")] | |||
#[cfg(feature = "shared_test_runtime")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in the README.md we have some outdated instructions on how to run the tests. Could you please update them too? 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind, doesn't need to be adjustment in the readme
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adjusted readme to use --test-threads=1
.
e56aaa5
to
f796733
Compare
This pull request has been deployed to Vercel. Latest commit: f796733 ✅ Preview: https://apps-ui-ha2v8ohpa-iota1.vercel.app |
This pull request has been deployed to Vercel. Latest commit: f796733 ✅ Preview: https://apps-backend-29a719c9ccbdb9b80c97b599a599d8953354da-mavt18c4y.vercel.app |
d08f374
into
sc-platform/indexer-new-rpc-tests
This pull request has been deployed to Vercel. Latest commit: f796733 ✅ Preview: https://iota-rebased-explorer-092603f1de8088243ccb8b861b23e-nuo5y05gw.vercel.app |
This pull request has been deployed to Vercel. Latest commit: f796733 ✅ Preview: https://wallet-dashboard-a225fcfa1e2f852205a61a7cd1d5d93e92-5hxaoftzo.vercel.app |
…ed test cluster (#3018) * feat(iota-indexer): Refactor ExtendedApi tests in indexer to use shared test cluster * Reuse function for finding available port/socket * Get rid of the `replace_db_name` function * Use &str instad of String for database_name where possible * Remove SimulacrumApiTestEnvDefinition type, rename InitializedSimulacrumEnv type * Fixes after rebase on recent feature branch * Update README to run tests with `--test-threads 1`
…ed test cluster (#3018) * feat(iota-indexer): Refactor ExtendedApi tests in indexer to use shared test cluster * Reuse function for finding available port/socket * Get rid of the `replace_db_name` function * Use &str instad of String for database_name where possible * Remove SimulacrumApiTestEnvDefinition type, rename InitializedSimulacrumEnv type * Fixes after rebase on recent feature branch * Update README to run tests with `--test-threads 1`
…ed test cluster (#3018) * feat(iota-indexer): Refactor ExtendedApi tests in indexer to use shared test cluster * Reuse function for finding available port/socket * Get rid of the `replace_db_name` function * Use &str instad of String for database_name where possible * Remove SimulacrumApiTestEnvDefinition type, rename InitializedSimulacrumEnv type * Fixes after rebase on recent feature branch * Update README to run tests with `--test-threads 1`
Description of change
Introduce shared simulacrum cluster, to use in extended api tests.
With this PR execution of all tests in iota-indexer takes 30 seconds.
With this change the tests are using
shared_test_indexer_db
for tests with shared cluster,simulacrum_env_db_extended_api
for extended_api tests with shared simulacrum andindexer_ingestion_tests_db
for ingestion tests in iota-indexer.Test indexer reader is always run on some free port, instead of a hardcoded one.
Simulacrum fullnode rpc api is run on free port by default, instead of hardcoded one.
In general it allows to have several simulacrum envs and indexers to be running next to each other without conflict.
Links to any relevant issues
fixes #2951
Type of change
How the change has been tested
cargo test --profile simulator --features shared_test_runtime --test rpc-tests
cargo test --profile simulator --features pg_integration --test ingestion_tests -- --test-threads=1
Change checklist
Tick the boxes that are relevant to your changes, and delete any items that are not.