Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Zombienet add tests #1321

Merged
merged 45 commits into from
Aug 18, 2022
Merged

Zombienet add tests #1321

merged 45 commits into from
Aug 18, 2022

Conversation

pepoviola
Copy link
Contributor

Hi @skunert, this add the missing integration test to zombienet_test and rename the command of the previous. Also, there is an small change in the test binary to allow to load the chain-spec from json because is needed to create a different genesis-state in the migrate_solo_to_para test.

NOTE: The ci configuration isn't included here, tracked in this issue: https://github.com/paritytech/ci_cd/issues/436

Thanks!

@pepoviola pepoviola requested a review from skunert May 31, 2022 16:42
dave: is up
alice: parachain 2000 is registered within 225 seconds
charlie: reports block height is at least 5 within 250 seconds
charlie: parachain 2000 perform upgrade with <wasm_binary_spec_version_incremented.rs.compact.compressed.wasm> within 200 seconds
Copy link
Contributor Author

Choose a reason for hiding this comment

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

NOTE: We need to have access to this artifact locally or through http to make the upgrade.

Copy link
Contributor

Choose a reason for hiding this comment

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

How does this work in practice? We upload the to-be-upgraded runtime somewhere and enter a url here which would befetches?

@pepoviola pepoviola added A0-pleasereview B0-silent Changes should not be mentioned in any release notes labels May 31, 2022

[[relaychain.nodes]]
name = "bob"
image = "docker.io/paritypr/polkadot-debug:5236-0.9.18-c55660e9-be16bd72"
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this image here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @bkchr, no this image should be set as env var by the ci job. The ci isn't ready yet, I will update this in all the files.

Thanks!

@skunert
Copy link
Contributor

skunert commented Jun 5, 2022

This looks good to me. @pepoviola title says WIP, what needs to be done here?
@bkchr after this is merged and CI is working, we want to completely remove the old integration tests, correct?

@pepoviola
Copy link
Contributor Author

This looks good to me. @pepoviola title says WIP, what needs to be done here? @bkchr after this is merged and CI is working, we want to completely remove the old integration tests, correct?

Hi @skunert, yes. I will need to make a couple of changes and also add the test to .gitlab-ci.yaml.
@bkchr is ok to use the image polkadot-debug:master for the relay chain nodes?

Thanks!

@pepoviola pepoviola requested a review from a team as a code owner June 7, 2022 09:09
@bkchr
Copy link
Member

bkchr commented Jun 7, 2022

@bkchr is ok to use the image polkadot-debug:master for the relay chain nodes?

IDK. What does this entails?

Copy link
Contributor

@alvicsam alvicsam left a comment

Choose a reason for hiding this comment

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

CI part looks good

@paritytech-ci paritytech-ci requested a review from a team June 7, 2022 15:08
@pepoviola
Copy link
Contributor Author

@bkchr is ok to use the image polkadot-debug:master for the relay chain nodes?

IDK. What does this entails?

That we always use the latest image from master for polkadot. Maybe make more sense to use the latest released version, but we should add more logic to check that and not update manually the test spec.

Thanks!

@pepoviola pepoviola changed the title [wip] Zombienet add tests Zombienet add tests Jul 2, 2022
@pepoviola
Copy link
Contributor Author

Hi @skunert, all tests are now migrated and working on gitlab.

  • 0001-sync_blocks_from_tip_without_connected_collator
  • 0002-pov_recovery
  • 0003-full_node_catching_up
  • 0004-runtime_upgrade
  • 0005-migrate_solo_to_para

Let me know if you want to add more tests.

Thanks!

@bkchr
Copy link
Member

bkchr commented Aug 8, 2022

@pepoviola sounds really good! So, we can merge this now?

@pepoviola
Copy link
Contributor Author

@pepoviola sounds really good! So, we can merge this now?

Hi @bkchr, thanks! Yes, we can merge now :)

@skunert
Copy link
Contributor

skunert commented Aug 9, 2022

Nice work here 👍.
One thing though, I compared all the tests again and noticed that none of the zombienet tests test the RPC functionality.
For example, the original test

.use_external_relay_chain_node_at_port(ws_port)
.build()
connects one node via RPC, this does not happen in the zombienet tests.

Missing for tests

  • 001
  • 002
  • 003

If this is a technical limitation for now, we could merge nevertheless but can not deprecate the rust tests until we have this. As I understand both zombienet and the old ones would run on CI for now?

@pepoviola
Copy link
Contributor Author

Nice work here 👍. One thing though, I compared all the tests again and noticed that none of the zombienet tests test the RPC functionality. For example, the original test

.use_external_relay_chain_node_at_port(ws_port)
.build()

connects one node via RPC, this does not happen in the zombienet tests.
Missing for tests

  • 001
  • 002
  • 003

If this is a technical limitation for now, we could merge nevertheless but can not deprecate the rust tests until we have this. As I understand both zombienet and the old ones would run on CI for now?

Hi @skunert, thanks for the feedback!. Those test (0001, 0002, 0003) check if the parachain 2000 is registered through the query interface using polakadotjs api.

const parachains = (await api.query.paras.parachains<Vec<ParaId>>()) || [];

If you want we can add one extra rcp call like api.rpc.system.chain()

Thanks!

@skunert
Copy link
Contributor

skunert commented Aug 9, 2022

Hey @pepoviola,
checking that the parachain is registered is not the problem here.

In test 0003, in the original rust test we start node eve as full node with cli argument --relay-chain-rpc-url <url_to_alice>:<alice_ws_port> which tests the rpc functionality. So this is what we would need to do in zombienet too.

@pepoviola
Copy link
Contributor Author

Hey @pepoviola, checking that the parachain is registered is not the problem here.

In test 0003, in the original rust test we start node eve as full node with cli argument --relay-chain-rpc-url <url_to_alice>:<alice_ws_port> which tests the rpc functionality. So this is what we would need to do in zombienet too.

Ahh... my bad @skunert. Yes, we can modify the network config to test the rpc with that cli argument. I will change that and ping you again.

Thanks!!

@pepoviola
Copy link
Contributor Author

Hi @skunert, I add the logic to use relay-chain-rpc-url in zombienet and I updated the tests.

Thanks!

Copy link
Contributor

@skunert skunert left a comment

Choose a reason for hiding this comment

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

LGTM!

@the-right-joyce the-right-joyce added A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. I5-tests Tests need fixing, improving or augmenting. and removed A0-pleasereview labels Aug 12, 2022
@pepoviola
Copy link
Contributor Author

Ping @skunert we need one more review to merge this one :)

@bkchr bkchr merged commit 725d007 into master Aug 18, 2022
@bkchr bkchr deleted the zombienet-add-tests branch August 18, 2022 15:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. I5-tests Tests need fixing, improving or augmenting.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants