Skip to content
This repository has been archived by the owner on Jan 11, 2024. It is now read-only.

Integration tests #143

Merged
merged 16 commits into from
Apr 3, 2023
Merged

Integration tests #143

merged 16 commits into from
Apr 3, 2023

Conversation

cryptoAtwill
Copy link
Contributor

Integration testing script for subnet lifecycle.

@cryptoAtwill cryptoAtwill changed the base branch from main to refactor_config March 30, 2023 08:04
Base automatically changed from refactor_config to main March 30, 2023 10:15
Copy link
Contributor

@adlrocha adlrocha left a comment

Choose a reason for hiding this comment

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

I think there are some additional code that got committed from the rebase making it harder to review.

For now, we are missing a README describing how to run the integration tests and I think we can merge this as a first iteration.

@adlrocha adlrocha merged commit e6b86af into main Apr 3, 2023
@adlrocha adlrocha deleted the integration_tests branch April 3, 2023 08:05
@@ -6,7 +6,7 @@ build:
cargo build -Z unstable-options --release --out-dir ./bin

test:
cargo test --release --workspace
cargo test --release --workspace --lib # only run unit tests
Copy link
Contributor

Choose a reason for hiding this comment

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

The problem with this is that it will prevent all integration tests from running, for example it will stop the ipld/resolver/tests/smoke.rs as well, which does not rely on a cluster to be set up.

Looking at the ref-fvm CI workflow, they use cargo test --exclude to prevent multiple tests from running. This only works on the crate level I think, which is why the integration tests are in a separate create within the workspace.

Other options would be to put these tests behind a feature flag and only switch them on when you want to run the end to end tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

All of them work for me, at this point I would suggest doing whatever is easier and less cumbersome for users while keeping the door open to eventually integrating it on CI.

Copy link
Contributor

Choose a reason for hiding this comment

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

They might work locally, but not on CI with make test. Compare the first passing commit to the last commit:

The first runs the smoke test:

     Running unittests src/lib.rs (target/release/deps/ipc_ipld_resolver-e87f2ddc0fd49dee)

running 9 tests
test hash::tests::vector_hashing ... ok
test behaviour::content::tests::non_ephemeral_addr ... ok
test limiter::tests::basics ... ok
test provider_cache::tests::prop_providers_pruned ... ok
test provider_cache::tests::prop_providers_listed ... ok
test provider_cache::tests::prop_subnets_pinned ... ok
test provider_cache::tests::prop_subnets_pruned ... ok
test provider_record::tests::prop_tamper_proof ... ok
test provider_record::tests::prop_roundtrip ... ok

test result: ok. 9 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.12s

     Running tests/smoke.rs (target/release/deps/smoke-20edbb0b97145a65)

running 1 test
test single_bootstrap_single_provider_resolve_one ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 3.03s

The second only runs the top section, not the bottom one.

IMO we should not let CI fall back on running the tests that it can run in the name of convenience of just not having to organise code better.

OTOH maybe it would be better to move the resolver into its own repository, since it doesn't rely on the IPC agent library at all, IIRC.

Copy link
Contributor

Choose a reason for hiding this comment

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

Here's a PR to move it; I don't think it's any less convenient: #153

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

Successfully merging this pull request may close these issues.

3 participants