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.

Sign up for GitHub

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

Run E2E shielder tests in the CI. #25

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Run E2E shielder tests in the CI. #25

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
ac3706b
Run E2E shielder tests in the CI.
deuszx Mar 22, 2023
a502920
Run E2E tests on push to main only.
deuszx Mar 27, 2023
ed043ce
Remove installation of wasm32 target
deuszx Mar 27, 2023
530fe35
Revert "Remove installation of wasm32 target"
deuszx Mar 27, 2023
a0cb15e
Temporarily disable special CI handling
deuszx Apr 18, 2023
a7bd0e2
Merge remote-tracking branch 'origin/main' into e2e-tests-ci
deuszx Apr 18, 2023
0f06693
Run e2e tests on PR as well - temp
deuszx Apr 18, 2023
4db263f
Fix name used in deploy.sh
deuszx Apr 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions .github/workflows/integration_tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
name: E2E Shielder tests.

on:
pull_request:
push:
branches:
- main

concurrency:
group: ${{ github.ref }}
pmikolajczyk41 marked this conversation as resolved.
Show resolved Hide resolved
cancel-in-progress: true

jobs:
rust-checks:
runs-on: self-hosted
steps:
- name: Checkout source code
uses: actions/checkout@v3

- name: Install Rust toolchain
uses: actions-rs/toolchain@v1
Copy link
Contributor

Choose a reason for hiding this comment

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

needed?

Copy link
Member

Choose a reason for hiding this comment

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

shouldn't be...

Copy link
Contributor Author

@deuszx deuszx Mar 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

No, wait. This is needed. Just look at one of the recent, failed jobs. All self-hosted workflows require that https://github.com/Cardinal-Cryptography/aleph-node/blob/main/.github/workflows/_build-liminal-node.yml

Copy link
Contributor

Choose a reason for hiding this comment

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

See if toolchain.yaml with a version won't solve this (next comment)


- name: Install WASM target
run: rustup target add wasm32-unknown-unknown
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't the target be baked into in the docker image already?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Target backed to docker image can be overriden by toolchain in the contract itself. If they don't match it will fail when building.

Copy link
Contributor

@fbielejec fbielejec Apr 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

For solution try:

[toolchain]
channel = "nightly-2022-10-30"
targets = [ "wasm32-unknown-unknown" ]
components = [ "rust-src" ]

then imho it should pull the required toolchain, target and components

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, this will work. It's just an alternative way of doing the same thing.


- name: Run e2e tests for shielder
run: |
cd shielder && make test-shielder-clean
4 changes: 1 addition & 3 deletions shielder/cli/tests/setup_local.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,7 @@ get_timestamp() {
}

log_progress() {
bold=$(tput bold)
normal=$(tput sgr0)
echo "[$(get_timestamp)] [INFO] ${bold}${1}${normal}"
pmikolajczyk41 marked this conversation as resolved.
Show resolved Hide resolved
echo "[$(get_timestamp)] [INFO] ${1}"
}

function setup_testdir() {
Expand Down
27 changes: 23 additions & 4 deletions shielder/deploy/deploy.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ set -euo pipefail

# Check if run in e2e shielder test context. Defaults to unset.
E2E_TEST_CONTEXT=${E2E_TEST:-}
# Check if running in CI context.
CI=${CI:-}

SCRIPT_DIR=$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" &>/dev/null && pwd)

Expand Down Expand Up @@ -41,9 +43,7 @@ get_timestamp() {
}

log_progress() {
bold=$(tput bold)
normal=$(tput sgr0)
echo "[$(get_timestamp)] [INFO] ${bold}${1}${normal}"
echo "[$(get_timestamp)] [INFO] ${1}"
}

random_salt() {
Expand Down Expand Up @@ -142,6 +142,14 @@ move_keys() {
}

docker_ink_dev() {
# if [[ -z "${CI}" ]]; then
local_docker_cargo "$@"
# else
# ci_docker_cargo "$@"
# fi
}

local_docker_cargo() {
docker run --rm \
-u "${DOCKER_USER}" \
-v "${PWD}":/code \
Expand All @@ -153,6 +161,15 @@ docker_ink_dev() {
-c "${1}"
}

ci_docker_cargo() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need a different version of the method when running in CI due to issues w/ permissions when running as a docker process - we can't create directories in cargo cache (and we try to do that when compiling contracts). Hence mounting of these dirs is removed.

Copy link
Contributor

@fbielejec fbielejec Mar 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I find that hard to believe - since it works just fine for compiling here. Take a good look at mapping users and user groups when running these containers, that's usually what is responsible for no read/write privileges.

Copy link
Contributor Author

@deuszx deuszx Mar 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

In TheButton you're copying everything and running Makefile inside docker. It makes "some" sense there as there are many contracts to be built and Yapee didn't want to spin up the same docker container multiple times (and couldn't mount cargo dirs directly for the exact same reason I can't here), but it's not the case here.

tl;dr; in TheButton you don't write to underlying file system but the dir owned by the docker.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Take a good look at mapping users and user groups when running these containers, that's usually what is responsible for no read/write privileges.

I had tried that, asked on Slack last week as well, but it doesn't work. Docker user is probably not a part of the required group on these machines and is not allowed to write to ~/.cargo/* dirs.

Copy link
Contributor

@fbielejec fbielejec Mar 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Then I don't understand why is the deployment script set up differently than in TheButton? It runs CI on exactly the same self-hosted instance. It mounts and writes to the contract directories, same.

Either you don't have to mount .cargo, or just ask devops guys to make it writable for the usergroup that the docker is part of. Or launch docker as a user in one of those groups. We control that instance.

Having two methods to launch will come back and bite, and will make any problems on CI irreproducible locally, making it even harder to debug potential problems.

Copy link
Contributor Author

@deuszx deuszx Mar 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

In TheButton you don't have the self-contained Makefile and CI is set up differently from the local development. Here I'm re-using the same scripts, commands, etc except for these 3 lines in cargo-docker.

Either you don't have to mount .cargo,

Well, you don't have to - it's just speeding up the build time.

Having two methods to launch will come back and bite, and will make any problems on CI irreproducible locally, making it even harder to debug potential problems.

This setup is IMO actually going to help w/ that - the local vs CI setup is basically identical (again, except for the part whether you use cached dependencies to download them every time). It's much better than the situation TheButton in that case.

docker run --rm \
-v "${PWD}":/code \
--network host \
--entrypoint /bin/sh \
"${INK_DEV_IMAGE}" \
-c "cargo ${1}"
}

build() {
cd "${SCRIPT_DIR}"/..

Expand Down Expand Up @@ -193,6 +210,7 @@ prefund_users() {
for recipient in "${DAMIAN_PUBKEY}" "${HANS_PUBKEY}"; do
transfer ${recipient}
done
log_progress "✅ Test accounts prefunded with SNZERO tokens."
}

# Distribute TOKEN_PER_PERSON of TOKEN_A and TOKEN_B to DAMIAN and HANS.
Expand All @@ -204,13 +222,14 @@ distribute_tokens() {
contract_call "--contract ${token} --message PSP22::transfer --args ${recipient} ${TOKEN_PER_PERSON} 0x00 --suri ${ADMIN}" 1>/dev/null
done
done
log_progress "✅ PSP22 tokens distributed"
}

deploy_shielder_contract() {
cd "${SCRIPT_DIR}"/..
SHIELDER_ADDRESS=$(contract_instantiate "--args ${MERKLE_LEAVES} --manifest-path contract/Cargo.toml" | jq -r '.contract')
export SHIELDER_ADDRESS
log_progress "Shielder address: ${SHIELDER_ADDRESS}"
log_progress "✅ Shielder address: ${SHIELDER_ADDRESS}"
}

# Set allowance at TOKEN_ALLOWANCE on TOKEN_A and TOKEN_B from SHIELDER, from DAMIAN and HANS.
Expand Down