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

refactor(tests/e2e-evm): Restructure ABI tests for extensibility & reduce duplication #2031

Open
wants to merge 11 commits into
base: nd-implement-basic-precompile-calling
Choose a base branch
from

Conversation

drklee3
Copy link
Member

@drklee3 drklee3 commented Oct 9, 2024

Description

  • De-duplicate reused logic
  • Group test cases into logical groups that run depending on the ABI function
  • Resolve type issues
    • Remove type casts for test cases to allow for type checking
    • Remove unsafe assignments

Fallback tests are not run on each function but rather only for the specific fallback.
This means whether or not to run the test is determined by only the receive and fallback
functions, without any other ABI function.
Previously used type assertions to bypass certain TypeScript issues with
test cases, along with using any & unsafe assignments. This resolves the
types to be properly valid and enforced to prevent any potential errors.
Changes from including a field in each test case from conditionally
running the case, to building the cases dynamically. This allows for
logical grouping of test cases and organization with logic instead of
using comments. Slightly less explicit for each test case, but with the
grouping of test cases, it reduces the mental overhead of figuring out
when each test case is run.
Resolves use type casting and unsafe access, validation of revert errors
for matches and types
Previously runs all the time, which is currently okay with the current
single testing contract that includes both functions. This conditionally
adds these test cases if the respective functions exist so we can test
additional contract behavior that may not have these functions and may
produce a different error.
Most of these issues are intentional and are okay to ignore. This also
sets the solhint ignoreConstructors option to true for the
func-visibility rule, as we are using solidity >=0.7.0
"extends": "solhint:recommended"
"extends": "solhint:recommended",
"rules": {
"func-visibility": ["warn", { "ignoreConstructors": true }]
Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

tyty I did not know about this configuration

@@ -10,6 +10,7 @@ import "hardhat-ignore-warnings";
// Chai setup
//
chai.use(chaiAsPromised);
chai.config.truncateThreshold = 0;
Copy link
Member

Choose a reason for hiding this comment

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

❤️ Idk why I didn't find this setting, so much nicer without any truncation

const expectedMatch = /call not allowed to disabled contract/;
expect(expectedMatch.test(revertDetail), `expected ${revertDetail} to match ${expectedMatch}`).to.be.true;
const expectedMatch = "call not allowed to disabled contract";
expect(revertDetail).to.equal(revertDetail, `expected ${revertDetail} to match ${expectedMatch}`);
Copy link
Member

Choose a reason for hiding this comment

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

Nice simplification -- we may have to change it back at some point though, but let's keep for now.

The error response of viem for hardhat, kvtool, etc can be a bit different so I meant to only match the revert message here. Unfortunately, chai doesn't have a great matcher for regex like testify.

mockAddress = (await hre.viem.deployContract(mockContractName)).address;

const mockArtifact = await hre.artifacts.readArtifact(mockContractName);
if (isHex(mockArtifact.deployedBytecode)) {
Copy link
Member

Choose a reason for hiding this comment

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

Great type guard

@@ -52,7 +59,7 @@ contract Caller {
// High level caller
//
contract NoopCaller {
NoopNoReceiveNoFallback target;
NoopNoReceiveNoFallback private target;
Copy link
Member

Choose a reason for hiding this comment

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

Nice reduction in scope 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants