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

test: use --isolate for test #35

Merged
merged 5 commits into from
May 7, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .forge-snapshots/BinHookTest#testBurnSucceedsWithHook.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
143903
181895
Original file line number Diff line number Diff line change
@@ -1 +1 @@
135247
186199
Original file line number Diff line number Diff line change
@@ -1 +1 @@
109118
137378
2 changes: 1 addition & 1 deletion .forge-snapshots/BinHookTest#testMintSucceedsWithHook.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
301295
330403
2 changes: 1 addition & 1 deletion .forge-snapshots/BinHookTest#testSwapSucceedsWithHook.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
139320
193012
Original file line number Diff line number Diff line change
@@ -1 +1 @@
90617
146435
Original file line number Diff line number Diff line change
@@ -1 +1 @@
5062
32866
Original file line number Diff line number Diff line change
@@ -1 +1 @@
9179
30395
Original file line number Diff line number Diff line change
@@ -1 +1 @@
65810
158518
Original file line number Diff line number Diff line change
@@ -1 +1 @@
149365
304088
2 changes: 1 addition & 1 deletion .forge-snapshots/BinPoolManagerTest#testGasBurnOneBin.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
66989
139645
2 changes: 1 addition & 1 deletion .forge-snapshots/BinPoolManagerTest#testGasDonate.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
52582
125450
Original file line number Diff line number Diff line change
@@ -1 +1 @@
968582
1013366
Original file line number Diff line number Diff line change
@@ -1 +1 @@
120096
341380
Original file line number Diff line number Diff line change
@@ -1 +1 @@
337772
380716
Original file line number Diff line number Diff line change
@@ -1 +1 @@
54833
149477
Original file line number Diff line number Diff line change
@@ -1 +1 @@
89370
187658
Original file line number Diff line number Diff line change
@@ -1 +1 @@
91355
193643
Original file line number Diff line number Diff line change
@@ -1 +1 @@
70859
145947
Original file line number Diff line number Diff line change
@@ -1 +1 @@
319479
350083
2 changes: 1 addition & 1 deletion .forge-snapshots/BinPoolManagerTest#testNoOpGas_Burn.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
41726
77022
Original file line number Diff line number Diff line change
@@ -1 +1 @@
19494
54514
Original file line number Diff line number Diff line change
@@ -1 +1 @@
37255
65003
2 changes: 1 addition & 1 deletion .forge-snapshots/BinPoolManagerTest#testNoOpGas_Mint.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
39359
70015
2 changes: 1 addition & 1 deletion .forge-snapshots/BinPoolManagerTest#testNoOpGas_Swap.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
22739
57943
Original file line number Diff line number Diff line change
@@ -1 +1 @@
8148
42036
Original file line number Diff line number Diff line change
@@ -1 +1 @@
349902
402358
Original file line number Diff line number Diff line change
@@ -1 +1 @@
60450
183346
Original file line number Diff line number Diff line change
@@ -1 +1 @@
243492
271028
2 changes: 1 addition & 1 deletion .forge-snapshots/CLPoolManagerTest#donateBothTokens.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
82627
175763
2 changes: 1 addition & 1 deletion .forge-snapshots/CLPoolManagerTest#gasDonateOneToken.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
52589
117213
Original file line number Diff line number Diff line change
@@ -1 +1 @@
36705
59737
Original file line number Diff line number Diff line change
@@ -1 +1 @@
43384
130156
Original file line number Diff line number Diff line change
@@ -1 +1 @@
54761
149129
Original file line number Diff line number Diff line change
@@ -1 +1 @@
100840
177628
Original file line number Diff line number Diff line change
@@ -1 +1 @@
25040547
25138211
2 changes: 1 addition & 1 deletion .forge-snapshots/CLPoolManagerTest#swap_simple.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
35961
79305
Original file line number Diff line number Diff line change
@@ -1 +1 @@
101598
158714
2 changes: 1 addition & 1 deletion .forge-snapshots/CLPoolManagerTest#swap_withHooks.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
41680
95812
2 changes: 1 addition & 1 deletion .forge-snapshots/CLPoolManagerTest#swap_withNative.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
35964
79308
Original file line number Diff line number Diff line change
@@ -1 +1 @@
4857
32661
Original file line number Diff line number Diff line change
@@ -1 +1 @@
19394
54306
Original file line number Diff line number Diff line change
@@ -1 +1 @@
37663
65459
Original file line number Diff line number Diff line change
@@ -1 +1 @@
30109
60625
2 changes: 1 addition & 1 deletion .forge-snapshots/CLPoolManagerTest#testNoOp_gas_Swap.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
21824
57300
2 changes: 1 addition & 1 deletion .forge-snapshots/VaultTest#collectFee.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
25576
53332
Original file line number Diff line number Diff line change
@@ -1 +1 @@
120730
159214
2 changes: 1 addition & 1 deletion .forge-snapshots/VaultTest#lockSettledWhenFlashloan.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
156938
103410
Original file line number Diff line number Diff line change
@@ -1 +1 @@
45186
118170
2 changes: 1 addition & 1 deletion .forge-snapshots/VaultTest#lockSettledWhenSwap.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
45185
118169
2 changes: 1 addition & 1 deletion .forge-snapshots/VaultTest#registerPoolManager.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
24484
47916
2 changes: 1 addition & 1 deletion .forge-snapshots/VaultTest#testLock_NoOp.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
11327
32799
Original file line number Diff line number Diff line change
@@ -1 +1 @@
71681
99905
Original file line number Diff line number Diff line change
@@ -1 +1 @@
33288
55012
2 changes: 1 addition & 1 deletion .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,6 @@ jobs:
version: nightly

- name: Run tests
run: forge test -vvv
run: forge test --isolate -vvv
env:
FOUNDRY_PROFILE: ${{ github.ref_name == 'main' && 'ci_main' || 'ci' }}
4 changes: 3 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,9 @@
## Running test

1. Install dependencies with `forge install` and `yarn`
2. Run test with `forge test`
2. Run test with `forge test --isolate`

See https://github.com/pancakeswap/pancake-v4-core/pull/35 on why `--isolate` flag is used.

## Update dependencies

Expand Down
6 changes: 3 additions & 3 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,9 @@
},
"scripts": {
"compile": "forge compile",
"test": "forge test",
"dev": "forge test -vvv -w",
"snapshot": "rm -fr .forge-snapshots && forge test",
"test": "forge test --isolate",
"dev": "forge test --isolate -vvv -w",
"snapshot": "rm -fr .forge-snapshots && forge test --isolate",
"prettier": "forge fmt src/ && forge fmt test/",
"prettier-check": "forge fmt --check",
"prepare": "husky install"
Expand Down
40 changes: 40 additions & 0 deletions test/Isolate.t.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.24;

import "forge-std/Test.sol";

/// @dev A test contract to ensure developers are using `--isolate` flag when running forge test
contract IsolateTest is Test {
StorageLib storageLib;

function setUp() public {
storageLib = new StorageLib();
}

function testIsolateTest() public {
// tstore key: 1 with value :2
storageLib.tstore(1, 2);

// toload key: 1
uint256 val = storageLib.tload(1);

// 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'?");
Copy link
Collaborator

@chefburger chefburger May 7, 2024

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:

  1. Both storageLib.tstore(1, 2); and uint256 val = storageLib.tload(1); are separate txs
  2. 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

Copy link
Collaborator Author

@ChefMist ChefMist May 7, 2024

Choose a reason for hiding this comment

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

  1. Both storageLib.tstore(1, 2); and uint256 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

  1. 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

Copy link
Collaborator

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 😂

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated readme with details 😁

}
}

contract StorageLib {
function tstore(uint256 key, uint256 val) public {
assembly {
tstore(key, val)
}
}

function tload(uint256 key) public view returns (uint256 val) {
assembly {
val := tload(key)
}
return val;
}
}
Loading