-
Notifications
You must be signed in to change notification settings - Fork 0
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
Static env files #37
Static env files #37
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these are really nice changes, thank you. Cleaning up and productionising the makefile and segmenting the tests is excellent.
There's a slight extra overhead having to learn the makefile syntax which I wasn't intimately familiar with, but piping it through GPT it became pretty clear. I would expect most users just use the default commands in the readme.
.env.dev.example
Outdated
# The whale needs to hold at least 3000 tokens | ||
TEST_TOKEN_WHALE="0x0000000000000000000000000000000000000000" | ||
|
||
# If you are testing with FORK_TEST_MODE='fork-existing', define the address of the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit confusing that this env var isn't defined in any .env file but I can see later where this gets injected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point @jordaniza. Since the developer experience is about using the makefile (rather than tweaking it), we can maybe reference the related targets (make test-exfork-testnet
, ...) instead of naming the env var?
Makefile
Outdated
# linux: allow shell scripts to be executed | ||
allow-scripts:; chmod +x ./coverage.sh | ||
# Set the RPC URL's for each target | ||
test-fork-testnet: export RPC_URL = $(TESTNET_RPC_URL) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, I can see what you're doing here having all the instantiation of env vars in the same place - was jumping around a bit to follow the make dependencies but now I get it - maybe we move them next to each other?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 👍
I have left only 3 at the top, because they might need to be changed and I think it's good that they remain easy to spot.
Makefile
Outdated
test-exfork-holesky: export FORK_TEST_MODE = fork-existing | ||
test-exfork-sepolia: export FORK_TEST_MODE = fork-existing | ||
|
||
TEST_SRC_FILES=$(wildcard test/*.sol test/**/*.sol script/*.sol script/**/*.sol src/escrow/increasing/delegation/*.sol src/libs/ProxyLib.sol) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think w/o a comment it's kinda confusing why these particular filenames are here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it work with a name like TEST_COVERAGE_SRC_FILES
?
The main goal would be to just 'use' the makefile rather than needing to read/edit it, but for the sake of the review it obviously needs to be clear :) Among the idiomatic makefile details to remark, I should probably have mention:
Is this what needed a bit of clarification? |
make help
targetmake init
target