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

🐳 Dockerize spec tests #3475

Closed

Conversation

wenceslas-sanchez
Copy link
Contributor

@wenceslas-sanchez wenceslas-sanchez commented Aug 7, 2023

Associated to #3395

Docker may help us to facilitate the test running accros system and python versions.

First you need to build the image; be at the root of the project and run docker build -f ./docker/Dockerfile -t consensus-specs ..
Then you can locate yourself in "./scripts" and run "run_test.sh".

What was added

  • a Dockerfile that contains the consensus-specs project.
  • a .dockerfile to ignore potential venv folders.
  • a script run_test.sh to run project tests.
  • a requirements-dev.txt containing all dependencies to run the tests.
  • a script dockerfile_test.sh to test the Dockerfile consistency.

TODO

  • complete the documentation to add the docker build + the script run
  • add a CircleCI process to test Dockerfile consistency
  • add a GitHub action to test Dockerfile consistency
  • test coverage copy if no coverage report is generated

@wenceslas-sanchez wenceslas-sanchez changed the title 🐳 Dockerize tests 🐳 Dockerize spec tests Aug 7, 2023

COPY . .

RUN pip install -r requirements-dev.txt -r requirements_preinstallation.txt
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. requirements_preinstallation.txt should be installed before requirements-dev.txt
  2. Can python -m pip install -e .[test] replace pip install -r requirements-dev.txt so that we don't need to maintain requirements-dev.txt file?

Copy link
Contributor Author

@wenceslas-sanchez wenceslas-sanchez Aug 8, 2023

Choose a reason for hiding this comment

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

  1. requirements_preinstallation.txt should be installed before requirements-dev.txt

You are right indeed.

  1. Can python -m pip install -e .[test] replace pip install -r requirements-dev.txt so that we don't need to maintain requirements-dev.txt file?

Indeed good idea !

Copy link
Contributor

@hwwhww hwwhww left a comment

Choose a reason for hiding this comment

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

Good job! It looks like it was run successfully with GitHub Action?

/cc @michaelneuder if you have some suggestions. :)

scripts/run_test.sh Outdated Show resolved Hide resolved
Co-authored-by: Hsiao-Wei Wang <[email protected]>
@wenceslas-sanchez
Copy link
Contributor Author

What the GitHub action is doing is to run pylint on the folder pysetup; it breaks if an error or a fatal error is encountered. However I am not sure it's a good idea to run it here.

@wenceslas-sanchez
Copy link
Contributor Author

Good job! It looks like it was run successfully with GitHub Action?

Yes it runs successfully 😄

/cc @michaelneuder if you have some suggestions. :)

@mrthankyou
Copy link

@wenceslas-sanchez,

Is there any additional help you need here? I'm wanting to work on an issue with the tests and am new to consensus-specs so it would be awesome if I can use a docker version of the spec-tests.

@wenceslas-sanchez
Copy link
Contributor Author

@wenceslas-sanchez,

Is there any additional help you need here? I'm wanting to work on an issue with the tests and am new to consensus-specs so it would be awesome if I can use a docker version of the spec-tests.

IDK, there are works done here #3477 by @parithosh regarding the CI.

@wenceslas-sanchez wenceslas-sanchez marked this pull request as ready for review October 25, 2023 19:29
@@ -76,6 +76,18 @@ Options:
- `--disable-bls`, to disable BLS (only for tests that can run without)
- `--bls-type`, `milagro` or `py_ecc` (default)

#### With Docker

Run `docker build -f ./docker/Dockerfile -t consensus-specs .` from the root of the specs repository to build the test image.
Copy link

@mrthankyou mrthankyou Nov 2, 2023

Choose a reason for hiding this comment

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

When I ran the docker command, I first thought the sentence meant that the docker command should run inside spec/ and not the root folder of the repo. Not sure if you want to clarify that but just thought it worth mentioning.

Great work on this!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Understood, does replacing from the root of the specs repository by from the repository root make it more understandable?

Choose a reason for hiding this comment

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

Yes! That makes sense to me.

@wenceslas-sanchez
Copy link
Contributor Author

Not needed anymore.

@wenceslas-sanchez wenceslas-sanchez deleted the test-dockerisation branch January 4, 2024 16:24
@parithosh
Copy link
Member

parithosh commented Jan 4, 2024

Not needed anymore.

Thank you very much for the initial PR! happy to iterate on the approach taken in the merged one

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