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 all 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 @@
143154
181146
Original file line number Diff line number Diff line change
@@ -1 +1 @@
134501
185453
Original file line number Diff line number Diff line change
@@ -1 +1 @@
108737
136997
2 changes: 1 addition & 1 deletion .forge-snapshots/BinHookTest#testMintSucceedsWithHook.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
300549
329657
2 changes: 1 addition & 1 deletion .forge-snapshots/BinHookTest#testSwapSucceedsWithHook.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
138567
192259
Original file line number Diff line number Diff line change
@@ -1 +1 @@
89917
142035
Original file line number Diff line number Diff line change
@@ -1 +1 @@
4679
32483
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 @@
65499
158207
Original file line number Diff line number Diff line change
@@ -1 +1 @@
149045
303831
2 changes: 1 addition & 1 deletion .forge-snapshots/BinPoolManagerTest#testGasBurnOneBin.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
66678
139396
2 changes: 1 addition & 1 deletion .forge-snapshots/BinPoolManagerTest#testGasDonate.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
52325
125193
Original file line number Diff line number Diff line change
@@ -1 +1 @@
968335
1013119
Original file line number Diff line number Diff line change
@@ -1 +1 @@
119849
341133
Original file line number Diff line number Diff line change
@@ -1 +1 @@
337527
380471
Original file line number Diff line number Diff line change
@@ -1 +1 @@
54588
149232
Original file line number Diff line number Diff line change
@@ -1 +1 @@
89025
187313
Original file line number Diff line number Diff line change
@@ -1 +1 @@
91010
193298
Original file line number Diff line number Diff line change
@@ -1 +1 @@
70574
145662
Original file line number Diff line number Diff line change
@@ -1 +1 @@
296760
327364
2 changes: 1 addition & 1 deletion .forge-snapshots/BinPoolManagerTest#testNoOpGas_Burn.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
41345
76641
Original file line number Diff line number Diff line change
@@ -1 +1 @@
19114
54134
Original file line number Diff line number Diff line change
@@ -1 +1 @@
36874
64622
2 changes: 1 addition & 1 deletion .forge-snapshots/BinPoolManagerTest#testNoOpGas_Mint.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
38978
69634
2 changes: 1 addition & 1 deletion .forge-snapshots/BinPoolManagerTest#testNoOpGas_Swap.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
22359
57563
Original file line number Diff line number Diff line change
@@ -1 +1 @@
7461
41349
Original file line number Diff line number Diff line change
@@ -1 +1 @@
349410
401866
Original file line number Diff line number Diff line change
@@ -1 +1 @@
59958
182854
Original file line number Diff line number Diff line change
@@ -1 +1 @@
220457
247993
2 changes: 1 addition & 1 deletion .forge-snapshots/CLPoolManagerTest#donateBothTokens.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
82382
175518
2 changes: 1 addition & 1 deletion .forge-snapshots/CLPoolManagerTest#gasDonateOneToken.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
52276
116900
Original file line number Diff line number Diff line change
@@ -1 +1 @@
36324
59356
Original file line number Diff line number Diff line change
@@ -1 +1 @@
42829
129601
Original file line number Diff line number Diff line change
@@ -1 +1 @@
54399
148767
Original file line number Diff line number Diff line change
@@ -1 +1 @@
100389
177177
Original file line number Diff line number Diff line change
@@ -1 +1 @@
24843277
24940941
2 changes: 1 addition & 1 deletion .forge-snapshots/CLPoolManagerTest#swap_simple.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
35478
78822
Original file line number Diff line number Diff line change
@@ -1 +1 @@
101168
158284
2 changes: 1 addition & 1 deletion .forge-snapshots/CLPoolManagerTest#swap_withHooks.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
41197
95329
2 changes: 1 addition & 1 deletion .forge-snapshots/CLPoolManagerTest#swap_withNative.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
35481
78825
Original file line number Diff line number Diff line change
@@ -1 +1 @@
4453
32257
Original file line number Diff line number Diff line change
@@ -1 +1 @@
19014
53926
Original file line number Diff line number Diff line change
@@ -1 +1 @@
37282
65078
Original file line number Diff line number Diff line change
@@ -1 +1 @@
29728
60244
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 @@
21443
56919
2 changes: 1 addition & 1 deletion .forge-snapshots/VaultTest#collectFee.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
25611
53360
Original file line number Diff line number Diff line change
@@ -1 +1 @@
120866
159350
2 changes: 1 addition & 1 deletion .forge-snapshots/VaultTest#lockSettledWhenFlashloan.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
157144
103575
Original file line number Diff line number Diff line change
@@ -1 +1 @@
45289
118273
2 changes: 1 addition & 1 deletion .forge-snapshots/VaultTest#lockSettledWhenSwap.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
45288
118272
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 @@
11517
32989
Original file line number Diff line number Diff line change
@@ -1 +1 @@
71811
100035
Original file line number Diff line number Diff line change
@@ -1 +1 @@
33418
55142
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