E2E test suite #232
E2E test suite #232
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub. |
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
zhongeric
left a comment
zhongeric
left a comment
There was a problem hiding this comment.
Great work, this is looking really good, just some comments
| initialIndex: 0, | ||
| mnemonic: "test test test test test test test test test test test junk", | ||
| path: "m/44'/60'/0'/0", | ||
| accountsBalance: "10000000000000000000000", // 10k ETH |
There was a problem hiding this comment.
this also seems like the default
There was a problem hiding this comment.
AccountsBalance isnt but I will remove the rest
| subtask(TASK_COMPILE_SOLIDITY_GET_SOURCE_PATHS, async (_, { config }) => { | ||
| const root = config.paths.root; | ||
| const contractSources = glob.sync(path.join(root, "src/**/*.sol")); | ||
| const testUtilsSources = glob.sync(path.join(root, "test/utils/**/*.sol")); | ||
| return [...contractSources, ...testUtilsSources].map((p) => path.normalize(p)); | ||
| }); |
There was a problem hiding this comment.
not sure what this is for, can you elaborate?
There was a problem hiding this comment.
Makes sure that files in test/utils get compiled as well, instead of just src. which we need for the WorkingCustomMockToken
There was a problem hiding this comment.
Ah I see, could we just hardcode its abi or something? to reduce complexity
There was a problem hiding this comment.
We would need its bytecode or artifact. Since we need to deploy it we need more than the ABI. But I can copy a copy of its artifact to test/e2e (in an artifact folder) and commit it if you think we should do that
There was a problem hiding this comment.
Actually I would need to investigate if using a copy of its artifact would be fine, since the contract is used in typechain bindings, I may not be able to get to figure out if we could use a copy of its artifact until I get back
test/e2e/src/SingleTestRunner.ts
Outdated
| // mine exactly one block | ||
| await provider.send(METHODS.EVM.MINE, []); | ||
|
|
||
| await provider.send(METHODS.EVM.SET_AUTOMINE, [true]); |
There was a problem hiding this comment.
do you need to send this every time?
There was a problem hiding this comment.
AutoMine is used by default, for after all specified objectives are over. I figured it is better to always keep the expected behavior outside of this function, and behavior is only different for this function. We do have parts of the code that use default automine behavior.
If you want though I can remove, and turn off automine at the start and only turn it on when we use it (like calling state-changing functions, etc)
There was a problem hiding this comment.
Is it not possible to just always explicitly mine when you expect to roll the chain forward?
There was a problem hiding this comment.
We could do that; the only issue would be for state-modifying function calls we wouldn't be able to do await though, we'd have to call the function, then explicitly mine.
Happy to do make either change (either turning off automine at the beginning, and only turning on automine when we need it, or never using automine and explicitly mining for all state-modifying calls, although personally I think the former makes more sense of the two and is cleaner).
Since I am gonna be out of reach I will just do the first option as I am gonna assume that's what you will want
| contract WorkingCustomMockToken is ERC20 { | ||
| uint8 private _decimals; | ||
|
|
||
| constructor(string memory name, string memory symbol, uint8 decimals_, uint256 initialSupply) ERC20(name, symbol) { | ||
| _decimals = decimals_; | ||
| _mint(msg.sender, initialSupply); | ||
| } | ||
|
|
||
| function decimals() public view virtual override returns (uint8) { | ||
| return _decimals; | ||
| } | ||
|
|
||
| function mint(address to, uint256 amount) public { | ||
| _mint(to, amount); | ||
| } | ||
| } |
There was a problem hiding this comment.
What's the need for this?
There was a problem hiding this comment.
ERC20 doesn't have decimals by default. We need this to be able to accurately launch/simulate tokens without 18 decimals
| @@ -0,0 +1,48 @@ | |||
| { | |||
| "name": "twap-auction-e2e", | |||
There was a problem hiding this comment.
Would be really nice to put this in the e2e folder, but understand if that's too big of a refactor!
There was a problem hiding this comment.
Yeah I wanted that originally but it needed to be at this level because otherwise, if the npm root is in test/e2e, hardhat can't compile src/ because it can only get files inside of its project .
Maybe there is some way to do it that I missed but I tried to make it work for a while before accepting that it wasnt possible without a symlink
There was a problem hiding this comment.
Oh i see, that's very reasonable, nw
ericneil-sanc
force-pushed
the
e2e-rebased
branch
from
f0c02cb to
375b853
Compare
Pull Request
Description
Please include a summary of the change and which feature was implemented or which issue was fixed. Also, include relevant motivation and context. List any dependencies that are required for this change.
Fixes # (issue)
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration.
Checklist:
Before deployment
After deployment
Considerations
forge fmtand prettier to ensure the code style is validAdditional context
Add any other context about the pull request here.