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

[REPLACED] Set up memory output comparison for contracts run in starknet-devnet tests #163

Closed
wants to merge 46 commits into from

Conversation

fmoletta
Copy link
Contributor

@fmoletta fmoletta commented Nov 29, 2022

Add a Makefile target compare_memory_devnet that runs the relevant starknet-devnet tests with cairo-rs-py runner and the original runner and compares the relocated memory outputs
Also add a workflow that runs this target
NOTE: Current CI time for this new workflow is 1 hour, some of it can be reduced by using cache

Steps:

  • Add patched envs build script
  • Expose write_binary_memory in PyCairoRunner
  • Add script/target to run each test in both envs and compare memory outputs after each run
  • Filter failing tests
  • Add Makefile target compare-memory-devnet
  • Integrate into CI

Future plans:

  • Add trace comparison
  • Produce different named files then running contracts so that we can compare every output instead of the last sets
  • Run contracts outside of devnet tests
  • Target other projects that use starknet

This PR has been replaced by its improved version #174

@fmoletta fmoletta changed the title Set up memory output comparison for contracts run in starkness-devnet tests Set up memory output comparison for contracts run in starknet-devnet tests Nov 29, 2022
@codecov-commenter
Copy link

codecov-commenter commented Nov 29, 2022

Codecov Report

Merging #163 (c257bcd) into main (37d5b6d) will increase coverage by 1.22%.
The diff coverage is 0.00%.

❗ Current head c257bcd differs from pull request most recent head b17141f. Consider uploading reports for the commit b17141f to get more accurate results

@@            Coverage Diff             @@
##             main     #163      +/-   ##
==========================================
+ Coverage   81.23%   82.45%   +1.22%     
==========================================
  Files          13       13              
  Lines         714      724      +10     
==========================================
+ Hits          580      597      +17     
+ Misses        134      127       -7     
Impacted Files Coverage Δ
src/cairo_runner.rs 70.00% <0.00%> (-0.53%) ⬇️
src/ids.rs 89.13% <0.00%> (+7.59%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@Oppen Oppen left a comment

Choose a reason for hiding this comment

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

There's a lot of unnecessary duplication of code. There's also duplicate work being performed that should have proper handling via dependency management in the Makefile.

# Set up the virtual envs
scripts/memory_comparator/build_envs.sh
# Clone the starknet-devnet from github
git clone [email protected]:Shard-Labs/starknet-devnet.git
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's checkout a fixed tag instead of main. We need stability for comparisons.

Comment on lines +110 to +113
# Set up the virtual envs
scripts/memory_comparator/build_envs.sh
# Clone the starknet-devnet from github
git clone [email protected]:Shard-Labs/starknet-devnet.git
Copy link
Contributor

Choose a reason for hiding this comment

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

This belongs into a separate setup target.

Comment on lines +119 to +130
# cairo-lang
. scripts/memory_comparator/cairo-lang/bin/activate && \
pip install starknet-devnet && \
cd starknet-devnet; scripts/install_dev_tools.sh
# Create the folder where we will store the memory outputs
cd starknet-devnet; mkdir memory_files
# Compile test files
. scripts/memory_comparator/cairo-lang/bin/activate && \
cd starknet-devnet; scripts/compile_contracts.sh
# Patch both envs
patch --directory scripts/memory_comparator/cairo-rs-py/lib/python3.9/site-packages/ --strip 2 < scripts/memory_comparator/output-memory-cairo-rs-py.patch
patch --directory scripts/memory_comparator/cairo-lang/lib/python3.9/site-packages/ --strip 2 < scripts/memory_comparator/output-memory-cairo-lang.patch
Copy link
Contributor

Choose a reason for hiding this comment

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

All the cairo-lang commands can run from a different target that builds the outputs, so it only runs once if we need to repeatedly run this test.

Comment on lines +1 to +16
#!/bin/sh

set -e

# This is not reaaaaally a robust way to find it, but you need to be actively
# trying to break it for this to fail :)
SCRIPT_DIR="scripts/memory_comparator"

python3.9 -m venv --upgrade-deps ${SCRIPT_DIR}/cairo-lang ${SCRIPT_DIR}/cairo-rs-py
${SCRIPT_DIR}/cairo-lang/bin/pip install cairo-lang==0.10.1
${SCRIPT_DIR}/cairo-rs-py/bin/pip install maturin==0.14.1 cairo-lang==0.10.1
${SCRIPT_DIR}/cairo-rs-py/bin/maturin build --manifest-path Cargo.toml --release --strip --interpreter 3.9 --no-default-features --features extension
${SCRIPT_DIR}/cairo-rs-py/bin/pip install target/wheels/cairo_rs_py-*.whl

${SCRIPT_DIR}/cairo-rs-py/bin/cairo-run --version
${SCRIPT_DIR}/cairo-rs-py/bin/starknet --version
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we copy-pasting this script?

Comment on lines +134 to +155
compare_memory_devnet_ci:
# Set up the virtual envs
scripts/memory_comparator/build_envs.sh
# Set up the starknet-devnet in each env
# cairo-rs-py
. scripts/memory_comparator/cairo-rs-py/bin/activate && \
pip install starknet-devnet && \
cd starknet-devnet; scripts/install_dev_tools.sh
# cairo-lang
. scripts/memory_comparator/cairo-lang/bin/activate && \
pip install starknet-devnet && \
cd starknet-devnet; scripts/install_dev_tools.sh
# Create the folder where we will store the memory outputs
cd starknet-devnet; mkdir memory_files
# Compile test files
. scripts/memory_comparator/cairo-lang/bin/activate && \
cd starknet-devnet; scripts/compile_contracts.sh
# Patch both envs
patch --directory scripts/memory_comparator/cairo-rs-py/lib/python3.9/site-packages/ --strip 2 < scripts/memory_comparator/output-memory-cairo-rs-py.patch
patch --directory scripts/memory_comparator/cairo-lang/lib/python3.9/site-packages/ --strip 2 < scripts/memory_comparator/output-memory-cairo-lang.patch
# Run each test one by one in each env and run the memory comparator
. ./scripts/memory_comparator/run_tests_compare_memory.sh
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like a lot of duplication.

@Oppen
Copy link
Contributor

Oppen commented Dec 6, 2022

Nevermind tho, I just realized it's a work in progress 🤦‍♂️

@fmoletta fmoletta changed the title Set up memory output comparison for contracts run in starknet-devnet tests [REPLACED] Set up memory output comparison for contracts run in starknet-devnet tests Dec 7, 2022
@fmoletta fmoletta marked this pull request as draft December 7, 2022 20:50
@Jrigada Jrigada closed this Dec 13, 2022
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