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

feat: migrate to testcontainers #161

Merged
merged 13 commits into from
May 18, 2023
Merged

feat: migrate to testcontainers #161

merged 13 commits into from
May 18, 2023

Conversation

itegulov
Copy link
Contributor

@itegulov itegulov commented May 15, 2023

This PR migrates integration tests to testcontainer-rs, which grants us the following benefits:

  • Containers are now destroyed deterministically, no need to clean them up manually
  • Proper wait conditions for when containers are up and ready to receive requests (i.e. no more random sleeps throughout the code and this should prevent test behaving erratically in slower/more constrained environments)
  • Sandbox is now dockerized too, hopefully no more host communication discrepancy between Macos & Linux. See feat: add dockerfile near-sandbox#59
  • Running the entire test suite takes 65s down from 130s on my machine
  • Introduced a new CLI utility that sets up the entire environment except for leader node which gives you an option to debug your code interactively without having to rebuild the docker image after every change

@itegulov itegulov marked this pull request as ready for review May 17, 2023 08:55
near-crypto = "0.16.1"
once_cell = "1"
near-units = "0.2.0"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Once we agreed that this dependency is to havy and can be replaced with something way smaller or just 2 helper functions. Is it necessary to have it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It only depends on num-format and regex according to cratesio. Not sure why you think it is too heavy, I don't remember this conversation.

serde = "1"
serde_json = "1"
testcontainers = { version = "0.14", features = ["experimental"] }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't it a dev dep?

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 am also using testcontainers for that CLI I mentioned in the README

@@ -51,7 +46,6 @@ async fn test_trio() -> anyhow::Result<()> {
.await
}

// TODO: write a test with real token
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess this TODO is not done :/ But ok, we have an issue for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I just moved this TODO to a different place

Running integration tests requires you to have relayer and sandbox docker images present on your machine:

```BASH
docker pull ghcr.io/near/pagoda-relayer-rs-fastauth
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we add this tome kind of setup.sh script?

Copy link
Member

Choose a reason for hiding this comment

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

we can probably do some cargo xtask to automate some of this

Copy link
Contributor

Choose a reason for hiding this comment

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

#92 introduces a Justfile that we could extend to include an init recipie.

You can pass environment variable `TESTCONTAINERS=keep` to keep all of the docker containers. For example:

```bash
$ TESTCONTAINERS=keep cargo test -p mpc-recovery-integration-tests
Copy link
Collaborator

Choose a reason for hiding this comment

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

How it plays with docker build . -t near/mpc-recovery? What is getting build and what is skipped?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The behavior is the same as it was before. Tests do not build containers, they assume those already exist and are available. So every time you change the code, you have to rebuild the image via docker build . -t near/mpc-recovery.

We have a CLI tool that can instantiate a short-lived development environment that has everything except for the leader node set up. You can then seamlessly plug in your own leader node instance that you have set up manually (the tool gives you a CLI command to use as a starting point, but you can attach debugger, enable extra logs etc). Try it out now (sets up 3 signer nodes):

```bash
$ cargo run -p mpc-recovery-integration-tests -- test-leader 3
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's a bit unclear what is the flow here. SHould I start all the env and then manually setup the leader node? How to rebuild and restart it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, running this command will create a test env for you and give you an example command on setting up the leader that communicates with this env. The flow is the same as it would be with any conventional Rust application: you run cargo run, you change code, you stop the server via ctrl+c, you run the server again with cargo run.

Copy link
Collaborator

@volovyks volovyks left a comment

Choose a reason for hiding this comment

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

Looks really awesome! And it should solve a lot of inconsistencies with our tests.
Some of the development flaws were not clear for me, maybe we should extend the README a bit.
I wander if we can use this cool CLI to setup the testing env instead of runing scripts manually.

bollard = "0.14"
bollard = "0.11"
Copy link
Member

Choose a reason for hiding this comment

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

huh version downgrade seems weird. Anything particular with this one?

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like it's to avoid version duplication as testcontainers requires 0.11.x.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

testcontainers is using an older version of bollard unfortunately

Running integration tests requires you to have relayer and sandbox docker images present on your machine:

```BASH
docker pull ghcr.io/near/pagoda-relayer-rs-fastauth
Copy link
Member

Choose a reason for hiding this comment

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

we can probably do some cargo xtask to automate some of this

integration-tests/src/lib.rs Outdated Show resolved Hide resolved
println!();
println!("Press any button to exit and destroy all containers...");

while stdin().read(&mut [0]).await? == 0 {}
Copy link
Contributor

Choose a reason for hiding this comment

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

neat one liner 👍🏽

@itegulov itegulov merged commit 8fc28c8 into develop May 18, 2023
@itegulov itegulov deleted the daniyar/testcontainers branch July 20, 2023 05:24
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.

4 participants