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

Jump to bottom

Dockerize Integration Tests #670

Merged
merged 5 commits into from
May 3, 2021
Merged

Dockerize Integration Tests #670

merged 5 commits into from
May 3, 2021

Conversation

gakonst
Copy link
Contributor

@gakonst gakonst commented Apr 28, 2021 •
edited
Loading

Fixes #654. depends on #669

@gakonst gakonst requested review from karlfloersch and tynes April 28, 2021 12:51
@changeset-bot
Copy link

changeset-bot bot commented Apr 28, 2021 •
edited
Loading

⚠️ No Changeset found

Latest commit: 7f8cfae

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@gakonst gakonst force-pushed the test/configurable-integration branch from 09ebcf1 to f228618 Compare April 28, 2021 12:53
@gakonst gakonst force-pushed the test/dockerize-integration branch from 8a31706 to 7fc5bea Compare April 28, 2021 12:54
Base automatically changed from test/configurable-integration to master April 28, 2021 13:08
@gakonst gakonst force-pushed the test/dockerize-integration branch 2 times, most recently from 46918cd to a1521e4 Compare April 28, 2021 13:51
@gakonst gakonst force-pushed the test/dockerize-integration branch from 4cd623c to e3705b4 Compare April 28, 2021 16:32
@gakonst gakonst marked this pull request as draft April 28, 2021 16:34
@gakonst gakonst self-assigned this Apr 28, 2021
tynes
tynes reviewed Apr 29, 2021
View reviewed changes
ops/docker-compose.yml Outdated Show resolved Hide resolved
@gakonst gakonst force-pushed the test/dockerize-integration branch 3 times, most recently from 4c50b40 to d56a290 Compare April 30, 2021 18:23
@gakonst gakonst marked this pull request as ready for review April 30, 2021 18:25
@gakonst gakonst force-pushed the test/dockerize-integration branch from e13ebc9 to 87350f4 Compare April 30, 2021 19:38
@gakonst gakonst requested review from tynes and removed request for karlfloersch, ben-chain, smartcontracts and maurelian April 30, 2021 19:40
tynes
tynes reviewed Apr 30, 2021
View reviewed changes
integration-tests/hardhat.config.ts Outdated
ovm: true,
},
},
solidity: '0.7.6',
ovm: {
solcVersion: '0.7.6',
solcVersion: '0.7.6-allow_kall_2', // temporary until we fix the build for 0.7.6
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be an env var with this as the default value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No this is not supposed to be configurable. Kall support is exposed by the compiler now so this change is no longer required.

tynes
tynes reviewed Apr 30, 2021
View reviewed changes
ops/docker/Dockerfile.monorepo Outdated
# copy over solc to save time building (35+ seconds vs not doing this step)
COPY --from=downloader solc /root/.cache/hardhat-nodejs/compilers/linux-amd64/solc-linux-amd64-v0.7.6+commit.7338295f
COPY --from=downloader ovm-solc /root/.cache/hardhat-nodejs/compilers/ovm/0.7.6.js
COPY --from=downloader ovm-solc /root/.cache/hardhat-nodejs/compilers/ovm/0.7.6-allow_kall_2.js
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we copy the compilers directory over so that we don't need to hard code versions here?

@tynes
Copy link
Contributor

tynes commented Apr 30, 2021

This will be nice because it will allow us to run the integration tests in kind.

This will also require parameterizing the network related information such that an arbitrary network and address manager can be provided.

Is this solved for or should we open another issue for this specifically? I'd like to be able to run the integration tests against a testnet deployment, so things like the timeout need to be very configurable.

@tynes
Copy link
Contributor

tynes commented Apr 30, 2021

One additional thing is that if we want to be able to run the integration tests against something that is not a testnet, we will need a hardhat environment to act as L1. We do not publish the hardhat image from this repo, I have published a hardhat repo in the past at ethereumoptimism/hardhat

@gakonst
Copy link
Contributor Author

gakonst commented May 1, 2021 •
edited
Loading

@tynes the tests take env vars as parameters: #669

Publishing a hardhat image / improving the dockerfile format seems out of scope of this PR.

@gakonst gakonst force-pushed the test/dockerize-integration branch from 87350f4 to 416a67d Compare May 1, 2021 17:27
@gakonst gakonst requested review from snario and tynes May 2, 2021 09:58
snario
snario reviewed May 3, 2021
View reviewed changes
ops/scripts/integration-tests.sh Outdated

if [[ ! -z "$URL" ]]; then
# get the addrs from the URL provided
ADDRESSES=$(curl --silent --retry-connrefused --retry $RETRIES --retry-delay 5 $URL)
Copy link
Contributor

Choose a reason for hiding this comment

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

--fail

snario
snario reviewed May 3, 2021
View reviewed changes
ops/scripts/integration-tests.sh

# wait for the sequencer to be up
curl \
--silent \
Copy link
Contributor

Choose a reason for hiding this comment

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

--fail --show-error

@gakonst gakonst merged commit 5622660 into master May 3, 2021
@gakonst gakonst deleted the test/dockerize-integration branch May 3, 2021 12:30
@gakonst gakonst mentioned this pull request May 3, 2021
Merged
InoMurko pushed a commit to omgnetwork/optimism that referenced this pull request May 25, 2021
@gakonst
Dockerize Integration Tests (ethereum-optimism#670)
cf0907e 
* test: add docker image for integration tests

* feat: add to docker-compose with scale 0

* ci: run and publish integration tests to dockerhub

* test: add option to not bring network up for docker integration tests

* chore: add --fail --show-error to curl
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@snario snario snario left review comments

@tynes tynes tynes approved these changes

Assignees

@gakonst gakonst

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Dockerize integration tests
3 participants
@gakonst @tynes @snario