-
Notifications
You must be signed in to change notification settings - Fork 35
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
test: use --isolate for test #35
Conversation
|
||
// If the test is run with `--isolate` flag, the value should be 0 | ||
// as --isolate run each top level call as seperate transaction, so tload will return 0 | ||
assertEq(val, 0, "did you forget to use --isolate flag for 'forge test'?"); |
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.
This seems a bit counter-intuitive to me. Just to confirm, does that mean:
- Both
storageLib.tstore(1, 2);
anduint256 val = storageLib.tload(1);
are separate txs - But if there are some external calls inside for example
storageLib.tload
it will behave like normal function call instead of stacking up another new tx
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.
- Both
storageLib.tstore(1, 2);
anduint256 val = storageLib.tload(1);
are separate txs
Yes, they will run as seperate transaction which is why tload(1)
will return 0 not 2 if you run test with forge test --isolate
- If there are more external calls inside for example
storageLib.tload
it will behave like normal function call instead of stacking up another new tx
Yes. the seperate transaction is only from top level call.
Do you have a recommendation for the comment on the test or assert error message so it can be clearer? we can update to it
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.
ehhh, maybe we can mention it in README or FAQ somewhere. That should avoid most unnecessary debugging if someone encounters it 😂
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.
updated readme with details 😁
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.
lgtm
This PR migrate
forge test
toforge test --isolate
.From https://book.getfoundry.sh/reference/cli/forge/test - on what
--isolate
flag doesContext
--isolate
will give us more accurate gas estimation as each call will now include a new transaction (21k) gas overhead--isolate
will ensure that we don't use thetstore
value in previous call.reserveOfVault