Test reorg + various improvements #284

PaulRBerg · 2023-01-21T09:57:28Z

This PR makes many improvements, most of them related to the tests.

andreivladbrg

Add {} brackets around Sablier contract names, functions, and events

I like these.

Commit a gas snapshot

Why is this useful?

Re-enables integration tests in CI. Hopefully #233 is fixed now after my foundry-rs/foundry-toolchain#19.

Unfortunately it doesn't. Tested locally and it took more than 15 minutes. Also a high number of requests.

Why do you want to run the e2e tests in the default foundry profile? I know you added a script in the json file for the yarn test CLI, but I think we should leave the basic forge test command to work without implementing an .env file.

I have a suggestion regarding the calculation functions from the Base_Test contract:

https://github.com/sablierhq/v2-core/blob/a99585a6e842d5156d34473ec9e31008231ff387/test/Base.t.sol#L147-L243

Let's add an abstract contract called Calculations in the helpers dir and move those there. Ofc it would inherit the Constants contract.

These should be also moved into the Constants contract.

https://github.com/sablierhq/v2-core/blob/a99585a6e842d5156d34473ec9e31008231ff387/test/Base.t.sol#L62-L67

Shouldn't we move the test/shared/helpers into a different path test/helpers?

test/fuzz/Fuzz.t.sol

test/unit/Unit.t.sol

andreivladbrg · 2023-01-25T14:45:46Z

test/Base.t.sol

                initialAdmin: users.admin,
                initialComptroller: comptroller,
                maxFee: DEFAULT_MAX_FEE
            });
-            pro = new SablierV2LockupPro({
+            pro = new DeployLockupPro().run({


Why do you deploy each of these when we can use DeployProtocol?

(comptroller, linear, pro) = new DeployProtocol().run({ initialAdmin: users.admin, maxFee: DEFAULT_MAX_FEE, maxSegmentCount: DEFAULT_MAX_SEGMENT_COUNT });

The point is to test the scripts, as many as possible. We want to test the individual scripts because it gives us confidence that they work as expected.

There is absolutely no reason why would DeployProtocol work and DeployComptroller DeployLockupLinear DeployLockupPro won't.

DeployProtocol = DeployComptroller + DeployLockupLinear + DeployLockupPro

I'm sorry, I don't understand what you mean. DeployProtocol does NOT import and does NOT use the other scripts.

However, this is an interesting idea. We could refactor the deployment scripts to do this, i.e. have DeployProtocol depend upon all the other scripts. If we do this, then we can use DeployProtocol in the tests, too.

test/Base.t.sol

test/unit/Unit.t.sol

src/abstracts/SablierV2FlashLoan.sol

PaulRBerg · 2023-01-26T11:55:48Z

Thanks, @andreivladbrg.

Why is this useful?

Why not? It gives us - and any potential reader of the repo - an overview of the current gas costs for running tests. I think that you should start thinking about this repo as a public good that will eventually be seen by many people.

Unfortunately it doesn't. Tested locally and it took more than 15 minutes. Also a high number of requests.

I think that you have misunderstood my intention with #233 (I admit that the name of the title isn't great).

The idea wasn't to make the end-to-end tests as fast as the unit or the fuzz tests - that is just not possible, because even with the cache, the data has to be loaded from the file system, which takes more time than generating in-memory values.

In short, my intention was to make the RPC caching work (see my PRs in the foundry-toolchain repo), and also cache the fuzzing seed on a weekly basis.

Based on the latest CI runs, I think that I have succeeded in achieving the above. The fact that it took 15 minutes to run locally is good.

Why do you want to run the e2e tests in the default foundry profile?

We have talked about this already - we need to include the e2e tests in the default Foundry profile because the test config option is NOT a glob pattern. There's no way to go around that now because we have both unit and fuzz tests - we can't set a path that includes only test/unit and test/fuzz but not test/e2e`.

I know you added a script in the json file for the yarn test

Yup, you should get used to it.

I think we should leave the basic forge test command to work without implementing an .env file.

We can't, based on the explanation above.

However, what you can do is set FOUNDRY_PROFILE to lite on your local machne, which will compile the contracts without the traditional optimizer enabled, and it will fuzz only 50 times. With this profile, it takes ~30 seconds on my machine to run all tests, including e2e tests.

I recommend you install direnv and add this env var in a .envrc file.

PaulRBerg · 2023-01-26T12:00:07Z

Let's add an abstract contract called Calculations in the helpers dir and move those there

I'm pretty agnostic about this - go for it if you wish.

These should be also moved into the Constants contract.

Fine, but make sure that they are initialized properly in the constructor of Constants.

Shouldn't we move the test/shared/helpers into a different path test/helpers?

That's where they used to be, and I moved them to test/shared/helpers in this commit:

8a84135

The rationale is the following - it's just easier to have only one "special" directory in test (i.e. shared) rather than two special directories (shared and helpers). The helpers are technically shared, so it made sense to me to place them under shared.

You will see that once I finish my work on invariants, it will look odd to have several tests and non-tests directories under test.

PaulRBerg · 2023-01-26T12:04:14Z

Regarding your comments:

Let's move these in the Base_Test contract, they are used in both fuzz and unit tests.

I agree.

andreivladbrg · 2023-01-27T12:05:20Z

an overview of the current gas costs for running tests. I think that you should start thinking about this repo as a public good that will eventually be seen by many people.

As discussed before, these gas costs are not accurate at all. I don't think it is in our favor, would be better to leave a screenshot of the --gas-report in the README file

I think that you have misunderstood my intention with #233 (I admit that the name of the title isn't great).

Yes, I did

we can't set a path that includes only test/unit and test/fuzz but not test/e2e`

Fair enough

However, what you can do is set FOUNDRY_PROFILE to lite on your local machne, which will compile the contracts without the traditional optimizer enabled, and it will fuzz only 50 times.

I personally know this, I was thinking more about a beginner who wants to just run the repo with the basic instructions given in the foundry book.

including e2e tests.

The e2e tests are not included in the yarn test CLI.
https://github.com/sablierhq/v2-core/blob/6eee3bc0b3a9a31a0dceaf74d564bcc56e05ecd7/package.json#L47

The rationale is the following - it's just easier to have only one "special" directory in test (i.e. shared) rather than two special directories (shared and helpers). The helpers are technically shared, so it made sense to me to place them under
shared

Fair enough.

build: upgrade to latest "forge-std" ci: re-enable integration tests style: format "foundry.toml" with Taplo test: add "lite" profile for quick testing locally test: fix integration tests test: use "getTokenBalances" from "forge-std"

chore: add "snapshot" script in "package.json"

build: add "build:optimized" command in "package.json" ci: rename "test" job to "build-and-test" ci: say "Uploaded to Codecov" in coverage result docs: document gas report and snapshot commands docs: document "test:optimized" command test:restrict gas reports to the Sablier contracts

ci: update paths to deployment scripts ci: update workflow names to match contracts

refactor: sort struct imports alphabetically test: add "DEFAULT_ASSET" and use it in stead of "dai" test: add "err" argument to most struct comparison assertions test: define "streamId" and load it in "setUp" function] test: delete "asset" in flash loan tests test: extend custom assertions with overloads that have "err" param test: fuzz sender in create function tests test: merge bottom leaves in all tests test: test event emit in the same unit tests test: use more named args

refactor: rename "admin" to "initialAdmin" in scripts refactor: rename "comptroller" to "initialComptroller" in scripts refactor: rename "approveSablierContracts" to "approveProtocol" refactor: rename "deploySablierContracts" to "deployProtocol" test: add "Deployers" struct tets: use "vm.startPrank" instead of "changePrank" in "setUp"

test: add "_Integration" suffix to all integration test contracts

chore: set verbosity 4 in "test-optimized" Foundry profile

docs: update "Tests" section of the README refactor: rename "FeeTooHigh" to "CalculatedFeeTooHigh" refactor: transfer "senderAmount" before "recipientAmount" test: add basic tests with harcoded values test: define all shared linear tests in "Linear.t.sol" test: define all shared pro tests in "Pro.t.sol" test: define "MAX_SEGMENTS" in "BaseTest" test: delete unused imports test: dryify test leaves test: ensure no same value for user addresses in integration tests test: fix tests names (remove "Fuzz_" when no fuzzing) test: fix typos in code comments test: fuzz amounts in "SegmentAmountsSumOverflows" branch test: import using relative paths (delete "test/=test/" remapping) test: improve the inheritance structure in the shared tests test: improve wording in test descriptions test: move "calculateStreamedAmount" helpers in "BaseTest" test: rename "DEFAULT_CREATE_AMOUNTS" to "DEFAULT_LOCKUP_CREATE_AMOUNTS" test: rename "depositDelta" to "depositDiff" test: restructure shared lockup tests test: run all tests by default test: test all statuses and withdrawn amounts in "withdrawMultiple" tests test: unfuzz "eve" and "operator" test: use default streams in "getStreamedAmount" and "getWithdrawableAmount" test: use named args for "changePrank" util test: use structs to fix StackTooDepp in "withdrawMultiple"

chore: pass "--no-match-path" to gas report and snapshot commands docs: add "sablier-v2" keyword in "package.json"

ci: make "test" and "coverage" jobs need "lint" and "build" jobs test: exclude end-to-end tests in "test:optimized" script

test: move "test/helpers/flash-loan" under "test/shared" test: move "test/helpers/hooks" under "test/shared"

test: move "test/shared/hooks" under "test/shared/mockups"

PaulRBerg · 2023-01-27T14:39:15Z

I don't think it is in our favor, would be better to leave a screenshot of the --gas-report in the README file

As discussed in person, we will keep the gas snapshot because it can be useful even if it's inaccurate.

I was thinking more about a beginner who wants to just run the repo with the basic instructions given in the foundry book.

Well, this is precisely why I have opened issue 3841 in the Foundry repo. It would be pretty cool to be able to set glob patterns as the value for the test option.

However, as a repo grows in complexity, it becomes impossible to stick with the default configs of any given open-source tool. The remedy for this is good developer documentation - which we will make sure to have.

The e2e tests are not included in the yarn test CLI.

I was referring to running forge test. It takes ~30 seconds to run all tests (including end-to-end) on my machine with the lite profile.

chore: use "override" specifier in pro chore: correct comment in flashloan contract test: add a calculations helper contract test: move logic from "Base_Test" to "Calculations" test: move constants from "Base_Test" to "Constants" test: move contracts that are used in both fuzz and unit test in the "Base_Test" test: label contracts in the "Base_Test" test: remove unused imports

PaulRBerg added enhancement labels Jan 21, 2023

PaulRBerg requested a review from andreivladbrg January 21, 2023 10:00

PaulRBerg force-pushed the prb/misc branch 7 times, most recently from 748b724 to f481635 Compare January 22, 2023 17:53

PaulRBerg mentioned this pull request Jan 22, 2023

refactor: add "Lockup" prefix #271

Merged

PaulRBerg force-pushed the prb/misc branch 3 times, most recently from b2d9aa7 to b2e3052 Compare January 24, 2023 13:17

PaulRBerg mentioned this pull request Jan 25, 2023

ERC-3156 flash loans #277

Merged

andreivladbrg reviewed Jan 25, 2023

View reviewed changes

PaulRBerg force-pushed the prb/misc branch from a99585a to 6eee3bc Compare January 25, 2023 21:15

PaulRBerg added 8 commits January 27, 2023 16:32

test: use Multicall from "forge-std"

3069f61

build: upgrade to latest "forge-std" ci: re-enable integration tests style: format "foundry.toml" with Taplo test: add "lite" profile for quick testing locally test: fix integration tests test: use "getTokenBalances" from "forge-std"

build: separate normal from deterministic deployment scripts

dd3b0b6

build: use Forge Std's CREATE2 factory

e3fd376

docs: improve wording in NatSpec comments

144446c

build: add gas snapshot

c2f19aa

chore: add "snapshot" script in "package.json"

docs: fix contract names in NatSpec comments

bc252d4

ci: update paths to deployment scripts ci: update workflow names to match contracts

PaulRBerg added 19 commits January 27, 2023 16:32

build: gas snapshot for optimized profile

dc33fcb

test: reference event name in all comments

ccb7dbd

test: pass "err" string to all assertions

80156b1

chore: commit ".vscode" in version control

24b82b1

build: regenerate gas snapshot

86b7416

test: add "_Unit" suffix to all unit test contracts

1541752

test: add "_Integration" suffix to all integration test contracts

chore: delete stale "ci" Foundry profile

cb5f800

chore: set verbosity 4 in "test-optimized" Foundry profile

build: regenerate gas snapshot

b985d26

chore: pass "--no-match-path" to gas report and snapshot commands docs: add "sablier-v2" keyword in "package.json"

test: rename "integration" to "e2e" tests

046eaab

ci: split "build-and-test" job

3c1f312

ci: make "test" and "coverage" jobs need "lint" and "build" jobs test: exclude end-to-end tests in "test:optimized" script

build: ugprade "forge-std" and "prb-math"

85e6356

test: add header separators for shared functions

0f45b48

test: test all values initialized in constructors

c8d8d0a

chore: delete unusable "gas:report:optimized" script

77b770f

test: nest "test/helpers" under "test/shared"

1173668

test: move "test/helpers/flash-loan" under "test/shared" test: move "test/helpers/hooks" under "test/shared"

test: move "test/shared/flash-loan" under "test/shared/mockups"

19d22c8

test: move "test/shared/hooks" under "test/shared/mockups"

ci: re-add weekly fuzz seed

f5745f1

PaulRBerg force-pushed the prb/misc branch from acc3a3d to 700be15 Compare January 27, 2023 14:35

PaulRBerg and others added 2 commits January 27, 2023 16:41

ci: selective fuzz runs

3df796c

PaulRBerg force-pushed the prb/misc branch from 700be15 to 76be144 Compare January 27, 2023 14:42

PaulRBerg mentioned this pull request Jan 27, 2023

Refactor the DeployProtocol to use the other deployment scripts #300

Closed

PaulRBerg merged commit cc0ad39 into main Jan 27, 2023

PaulRBerg deleted the prb/misc branch January 27, 2023 15:01

This was referenced Jan 28, 2023

refactor: rename dynamic amount calculation functions #296

Merged

Write end-to-end tests for the cancel functionality #175

Closed

Take a gas snapshot and store it in version control #272

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test reorg + various improvements #284

Test reorg + various improvements #284

PaulRBerg commented Jan 21, 2023 •

edited

Loading

andreivladbrg left a comment

andreivladbrg Jan 25, 2023

PaulRBerg Jan 26, 2023

andreivladbrg Jan 27, 2023

PaulRBerg Jan 27, 2023

PaulRBerg commented Jan 26, 2023

PaulRBerg commented Jan 26, 2023

PaulRBerg commented Jan 26, 2023

andreivladbrg commented Jan 27, 2023

PaulRBerg commented Jan 27, 2023

Test reorg + various improvements #284

Test reorg + various improvements #284

Conversation

PaulRBerg commented Jan 21, 2023 • edited Loading

andreivladbrg left a comment

Choose a reason for hiding this comment

andreivladbrg Jan 25, 2023

Choose a reason for hiding this comment

PaulRBerg Jan 26, 2023

Choose a reason for hiding this comment

andreivladbrg Jan 27, 2023

Choose a reason for hiding this comment

PaulRBerg Jan 27, 2023

Choose a reason for hiding this comment

PaulRBerg commented Jan 26, 2023

PaulRBerg commented Jan 26, 2023

PaulRBerg commented Jan 26, 2023

andreivladbrg commented Jan 27, 2023

PaulRBerg commented Jan 27, 2023

PaulRBerg commented Jan 21, 2023 •

edited

Loading