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

290 Tests in docker #296

Closed
wants to merge 32 commits into from
Closed

290 Tests in docker #296

wants to merge 32 commits into from

Conversation

shramee
Copy link
Contributor

@shramee shramee commented Jan 21, 2023

Usage related changes

  • Users are unaffected.

Development related changes

Executing tests in docker

You can use docker tester image. The image includes testing environment set up with everything required to run the tests. The spawned container has access to docker daemon so it can also run the tests that require spawning Devnet/Cairo CLI etc containers.

npm run docker-tester

This is take you in the container shell. Once in the container, you can use the test- scripts defined in package.json in an isolated environment.

Fixes #290

Checklist:

  • Formatted the code
  • No linter errors + tried to avoid introducing linter warnings
  • Performed a self-review of the code
  • Rebased to the last commit of the target branch (or merged it into my branch)
  • Documented the changes
  • Updated the test directory (with a test case consisting of network.json, hardhat.config.ts, check.ts)
  • Linked issues which this PR resolves
  • Created a PR to the plugin branch of starknet-hardhat-example:
    • < EXAMPLE_REPO_PR_URL >
    • Modified test.sh to use my example repo branch
    • Restored test.sh to to use the original branch (after the example repo PR has been merged)
  • All tests are passing (for external contributors who don't have access to the CI/CD pipeline)

@FabijanC To see if this looks good.
* Env defaults
* Skip ppyenv install if already installed
* `https` url for git
@shramee
Copy link
Contributor Author

shramee commented Jan 22, 2023

Sorry, this needs more testing.

@shramee shramee marked this pull request as draft January 22, 2023 07:03
getFixVolumeHostPathCallback()
Uses environment variable STARKNET_HARDHAT_REPLACE_HOST_PATH
And STARKNET_HARDHAT_REPLACE_HOST_PATH env var to let JS know about the volume paths that need changing.
`STARKNET_HARDHAT_REPLACE_HOST_PATH` is now called
`STARKNET_HARDHAT_DIND_HOST_PATH`
@shramee shramee marked this pull request as ready for review February 22, 2023 14:36
@shramee
Copy link
Contributor Author

shramee commented Feb 23, 2023

Add a ton of docs, let me know if it's a bit much and some of it is unnecessary...


Dockerized testing environment

We have added a Docker in Docker, called DinD, setup to run the tests that need to use Docker from within a Docker container.
This allows running tests easily without changing your development environment. Read about running tests in Executing tests in docker section. If you are not adding Docker container volumes you can skip this section.

Running tests inside DinD requires some additional setup/parsing when adding new Docker container volumes to map container paths.

Adding new docker container

If you are adding a new docker container with volumes please refer to the next section, Adding docker volumes, to properly parse host paths for Docker in Docker to work.

Adding docker volumes

For volume mounts, Use getVolumeHostPathFilter function from ./external-server/external-server to get the filter function to prepare paths for DinD if required.

Here's an example,

// import ...
import { getVolumeHostPathFilter } from "./external-server/external-server";

// A bunch of code...

const volumeHostPathFilter = getVolumeHostPathFilter();
// Docker volume mounting args
const volumes = [
    "-v",
    `${volumeHostPathFilter(pathInHost)}:${pathInContainer}`,
    "-v",
    `${volumeHostPathFilter(pathInHost2)}:${pathInContainer2}`
];

// Other bunch of code...

We get the filter function from getVolumeHostPathFilter(). Then host parts for every volume are passed filtered with volumeHostPathFilter function.

Testing

...

Executing tests in docker

You can use docker tester image. The image includes testing environment set up with everything required to run the tests. The spawned container has access to docker daemon so it can also run the tests that require spawning Devnet/Cairo CLI etc containers. Read more in Dockerized testing environment section.

npm run docker-tester

This is take you in the container shell. Once in the container, you can use the test- scripts defined in package.json in an isolated environment.

...

@@ -7,6 +7,7 @@ set -eu
PY_VERSION=3.8.9

if [[ "$OSTYPE" == "linux-gnu"* ]]; then
which "/opt/circleci/.pyenv/versions/$PY_VERSION/bin/python" || pyenv install "$PY_VERSION"
# Install version PY_VERSION, but skip if already installed.
which "/opt/circleci/.pyenv/versions/$PY_VERSION/bin/python" || pyenv install "$PY_VERSION" -s
Copy link
Contributor Author

Choose a reason for hiding this comment

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

-s flag skips if pyenv version is already installed.
https://github.com/pyenv/pyenv/blob/master/COMMANDS.md#pyenv-install

-s/--skip-existing Skip the installation if the version appears to be installed already

@@ -2,6 +2,9 @@

set -eu

pip3 install "starknet-devnet==$STARKNET_DEVNET"
# Get default from config.json file
STARKNET_DEVNET_DEFAULT=$(node -e "console.log(require('../config.json').STARKNET_DEVNET)")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sets default for STARKNET_DEVNET env var from config.json

# Get default from config.json file
STARKNET_DEVNET_DEFAULT=$(node -e "console.log(require('../config.json').STARKNET_DEVNET)")

pip3 install "starknet-devnet==${STARKNET_DEVNET:=$STARKNET_DEVNET_DEFAULT}" | { grep --invert-match 'Requirement already satisfied:' || :; }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. Uses STARKNET_DEVNET_DEFAULT fallback
  2. grep removes Requirement already satisfied logs.
  3. || :; part, which is like or nothing;, prevents grep from exiting the process.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of adding this comments to the PR, they would be more useful in the file itself (applicable for other such comments).

@@ -13,7 +13,7 @@ echo "python version: $(python --version)"
if [[ "$OSTYPE" == "darwin"* ]]; then
export HOMEBREW_NO_INSTALL_CLEANUP=1
brew ls --versions gmp || brew install gmp
CFLAGS=-I`brew --prefix gmp`/include LDFLAGS=-L`brew --prefix gmp`/lib pip3 install fastecdsa
CFLAGS=-I`brew --prefix gmp`/include LDFLAGS=-L`brew --prefix gmp`/lib pip3 install fastecdsa | { grep --invert-match 'Requirement already satisfied:' || :; }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as https://github.com/Shard-Labs/starknet-hardhat-plugin/pull/296/files#r1115318562
Strips Requirement already satisfied logs and stops grep from exiting the process.

scripts/test.sh Outdated
@@ -10,12 +10,12 @@ CONFIG_FILE_NAME="hardhat.config.ts"
# setup example repo
rm -rf starknet-hardhat-example
EXAMPLE_REPO_BRANCH="plugin"
if [[ "$CIRCLE_BRANCH" == "master" ]] && [[ "$EXAMPLE_REPO_BRANCH" != "plugin" ]]; then
if [[ "${CIRCLE_BRANCH:=}" == "master" ]] && [[ "$EXAMPLE_REPO_BRANCH" != "plugin" ]]; then
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Empty default from CIRCLE_BRANCH

Devnet takes long to spawn sometimes, but does eventually.

[skip ci]
@FabijanC
Setting up for tests is now optional and can be skipped by setting environment variable `STARKNET_HARDHAT_TEST_SKIP_SETUP=1`.

[skip ci]
@FabijanC FabijanC self-requested a review February 27, 2023 09:33
Copy link
Collaborator

@FabijanC FabijanC left a comment

Choose a reason for hiding this comment

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

Your PR seems to introduce changes that are already on master. Perhaps merging master into your branch would help (also there are conflicts with the target branch). It should be easier to review after that.

I haven't fully reviewed, but if I understand correctly, some parts of the coding (non-testing code) are now working differently just to enable testing with DinD?

# Get default from config.json file
STARKNET_DEVNET_DEFAULT=$(node -e "console.log(require('../config.json').STARKNET_DEVNET)")

pip3 install "starknet-devnet==${STARKNET_DEVNET:=$STARKNET_DEVNET_DEFAULT}" | { grep --invert-match 'Requirement already satisfied:' || :; }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of adding this comments to the PR, they would be more useful in the file itself (applicable for other such comments).

@shramee
Copy link
Contributor Author

shramee commented Mar 6, 2023

Closed in favour of #331

@shramee shramee closed this Mar 6, 2023
@shramee shramee mentioned this pull request Mar 6, 2023
11 tasks
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.

Make running tests easier
5 participants