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

Foundry prbmath external test #13873

Merged
merged 2 commits into from
Jul 21, 2023
Merged

Foundry prbmath external test #13873

merged 2 commits into from
Jul 21, 2023

Conversation

r0qs
Copy link
Member

@r0qs r0qs commented Jan 16, 2023

Fixes #13767.

This PR migrates the prb-math external tests script to python and add support to foundry.

The PR also adds an initial architecture to the python-based external tests. The idea was to allow us to easily migrate from different test frameworks as external projects change their dependencies.

I didn't migrate all the shellscripts yet, for instance, the store_benchmark_report still missing for the prb-math. But it would be nice to discuss some ideas around the current architecture.

Depends on: #13982, #14064 and #14140.

  • The dependence on Cleanup base images and CI config #13982 is due to the change of the t_ems_ext, which was renamed to t_ext and now receives the image as parameter instead of only the nodejs version. This allows running some external tests on different base images, e.g. cimg/rust:1.66.1. The change was required since Foundry depends on rust and not nodejs. Actually, as we are now downloading the forge binary, rust is not needed.

  • The dependence on External tests script refactor #14064 is based on the replacement of the old test/externalTests.sh by a python script that allows run one, multiple or all external tests.

  • The dependence on Foundry ci setup #14140 is due to the new install_python3 and install_foundry CI commands.

TODO:

  • Update external tests README (I think we can update the docs once we migrate the other tests to python as well. So we can have a PR that introduces the new way of setup a external test using the python-based suite)
  • Add support to Foundry in parse_eth_gas_report.py (will be done in a future PR)
  • Store Foundry benchmarks report (will be done in a future PR)

@r0qs r0qs force-pushed the foundry-prbmath-external-test branch 2 times, most recently from 349c8b9 to b512bd7 Compare January 17, 2023 00:41
test/externalTests/external_test.py Outdated Show resolved Hide resolved
test/externalTests/external_test.py Outdated Show resolved Hide resolved
test/externalTests/external_test.py Outdated Show resolved Hide resolved
test/externalTests/prb-math.py Outdated Show resolved Hide resolved
test/externalTests/prb-math.py Outdated Show resolved Hide resolved
test/externalTests/prb-math.py Outdated Show resolved Hide resolved
test/externalTests/foundry.py Outdated Show resolved Hide resolved
test/externalTests/external_test.py Outdated Show resolved Hide resolved
.circleci/config.yml Outdated Show resolved Hide resolved
.circleci/config.yml Outdated Show resolved Hide resolved
.circleci/config.yml Outdated Show resolved Hide resolved
.circleci/config.yml Outdated Show resolved Hide resolved
.circleci/config.yml Outdated Show resolved Hide resolved
test/externalTests/foundry.py Outdated Show resolved Hide resolved
test/externalTests/external_test.py Outdated Show resolved Hide resolved
test/externalTests/external_test.py Outdated Show resolved Hide resolved
test/externalTests/external_test.py Outdated Show resolved Hide resolved
test/externalTests/foundry.py Outdated Show resolved Hide resolved
@r0qs r0qs force-pushed the foundry-prbmath-external-test branch 3 times, most recently from eda4a30 to 55464fd Compare January 23, 2023 18:17
@r0qs r0qs self-assigned this Jan 27, 2023
.circleci/config.yml Outdated Show resolved Hide resolved
.circleci/config.yml Outdated Show resolved Hide resolved
.circleci/config.yml Outdated Show resolved Hide resolved
.circleci/config.yml Outdated Show resolved Hide resolved
.circleci/config.yml Outdated Show resolved Hide resolved
.circleci/config.yml Outdated Show resolved Hide resolved
@github-actions github-actions bot added the stale The issue/PR was marked as stale because it has been open for too long. label Feb 11, 2023
@ethereum ethereum deleted a comment from github-actions bot Feb 11, 2023
@r0qs r0qs removed the stale The issue/PR was marked as stale because it has been open for too long. label Feb 11, 2023
@github-actions github-actions bot added the stale The issue/PR was marked as stale because it has been open for too long. label Apr 6, 2023
@r0qs r0qs force-pushed the foundry-prbmath-external-test branch from 7ba7c7e to 47b4ec0 Compare April 6, 2023 14:01
@r0qs r0qs added has dependencies The PR depends on other PRs that must be merged first and removed stale The issue/PR was marked as stale because it has been open for too long. labels Apr 6, 2023
@r0qs r0qs changed the base branch from develop to external-tests-refactor April 6, 2023 14:39
@r0qs r0qs force-pushed the foundry-prbmath-external-test branch from 47b4ec0 to ea839c6 Compare April 6, 2023 18:32
@r0qs r0qs force-pushed the external-tests-refactor branch from 3486a83 to 5468baf Compare April 6, 2023 18:37
@r0qs r0qs force-pushed the foundry-prbmath-external-test branch 2 times, most recently from f37184a to 39aac12 Compare April 7, 2023 13:06
@r0qs r0qs force-pushed the external-tests-refactor branch from 26fe47f to f1a27c2 Compare April 18, 2023 21:30
@r0qs r0qs force-pushed the foundry-prbmath-external-test branch from 39aac12 to d77f8ad Compare April 18, 2023 21:32
@r0qs r0qs mentioned this pull request Apr 19, 2023
@ethereum ethereum deleted a comment from github-actions bot Apr 19, 2023
@nikola-matic nikola-matic removed the stale The issue/PR was marked as stale because it has been open for too long. label Jul 15, 2023
@ethereum ethereum deleted a comment from github-actions bot Jul 17, 2023
@cameel cameel force-pushed the foundry-prbmath-external-test branch 3 times, most recently from 414d669 to 6b24116 Compare July 20, 2023 14:42
Copy link
Member

@cameel cameel left a comment

Choose a reason for hiding this comment

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

Here's more feedback.

I also pushed some new commits that address minor things that weren't worth starting a discussion.

test/externalTests/prb-math.sh Show resolved Hide resolved
scripts/externalTests/test_helpers.py Outdated Show resolved Hide resolved
scripts/externalTests/test_helpers.py Outdated Show resolved Hide resolved
scripts/externalTests/test_helpers.py Outdated Show resolved Hide resolved
scripts/externalTests/test_helpers.py Outdated Show resolved Hide resolved
scripts/externalTests/test_helpers.py Outdated Show resolved Hide resolved
scripts/externalTests/test_helpers.py Outdated Show resolved Hide resolved
scripts/externalTests/test_helpers.py Outdated Show resolved Hide resolved
scripts/externalTests/test_helpers.py Outdated Show resolved Hide resolved
scripts/externalTests/test_helpers.py Outdated Show resolved Hide resolved
@cameel cameel force-pushed the foundry-prbmath-external-test branch from 98578ff to 1605683 Compare July 20, 2023 15:04
@r0qs r0qs force-pushed the foundry-prbmath-external-test branch from 3d46980 to 300f32f Compare July 21, 2023 13:22
scripts/pylintrc Outdated Show resolved Hide resolved
@r0qs r0qs force-pushed the foundry-prbmath-external-test branch 3 times, most recently from e01b456 to f777a98 Compare July 21, 2023 15:59
scripts/externalTests/runners/base.py Outdated Show resolved Hide resolved
scripts/externalTests/runners/base.py Outdated Show resolved Hide resolved
cameel
cameel previously approved these changes Jul 21, 2023
Copy link
Member

@cameel cameel left a comment

Choose a reason for hiding this comment

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

I'm approving since this is already good enough.

There are still a few minor comments that I see you're in process of addressing and you need to squash, but I'm assuming all of that will be dealt with. Feel free to merge once you're done.

@r0qs r0qs force-pushed the foundry-prbmath-external-test branch from 006df04 to 469dd8b Compare July 21, 2023 16:26
@r0qs r0qs force-pushed the foundry-prbmath-external-test branch from 469dd8b to 0940a56 Compare July 21, 2023 16:36
cameel
cameel previously approved these changes Jul 21, 2023
@r0qs r0qs force-pushed the foundry-prbmath-external-test branch from 0940a56 to b802782 Compare July 21, 2023 18:24
@r0qs r0qs force-pushed the foundry-prbmath-external-test branch from b802782 to dcbd645 Compare July 21, 2023 18:26
@cameel cameel merged commit 957a9e7 into develop Jul 21, 2023
@cameel cameel deleted the foundry-prbmath-external-test branch July 21, 2023 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

Add Foundry support to external tests scripts
4 participants