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

New Rule: foundry test function names and other fixes #476

Merged
merged 3 commits into from
Aug 11, 2023
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 .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,4 @@ convertLib.sol
antlr4.jar
/docs/.sass-cache/
_temp/
.solhint.json

2 changes: 2 additions & 0 deletions conf/rulesets/solhint-all.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
module.exports = Object.freeze({
rules: {
'code-complexity': ['warn', 7],
'custom-errors': 'warn',
'explicit-types': ['warn', 'explicit'],
'function-max-lines': ['warn', 50],
'max-line-length': ['error', 120],
Expand All @@ -28,6 +29,7 @@ module.exports = Object.freeze({
'const-name-snakecase': 'warn',
'contract-name-camelcase': 'warn',
'event-name-camelcase': 'warn',
'foundry-test-functions': ['off', ['setUp']],
'func-name-mixedcase': 'warn',
'func-named-parameters': ['warn', 4],
'func-param-name-mixedcase': 'warn',
Expand Down
1 change: 1 addition & 0 deletions conf/rulesets/solhint-recommended.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

module.exports = Object.freeze({
rules: {
'custom-errors': 'warn',
'explicit-types': ['warn', 'explicit'],
'max-states-count': ['warn', 15],
'no-console': 'error',
Expand Down
2 changes: 2 additions & 0 deletions docs/rules.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ title: "Rule Index of Solhint"
| Rule Id | Error | Recommended | Deprecated |
| ------------------------------------------------------------------ | --------------------------------------------------------------------------------------------------------------------- | ------------ | ---------- |
| [code-complexity](./rules/best-practises/code-complexity.md) | Function has cyclomatic complexity "current" but allowed no more than maxcompl. | | |
| [custom-errors](./rules/best-practises/custom-errors.md) | Enforces the use of Custom Errors over Require and Revert statements | $~~~~~~~~$✔️ | |
| [explicit-types](./rules/best-practises/explicit-types.md) | Forbid or enforce explicit types (like uint256) that have an alias (like uint). | $~~~~~~~~$✔️ | |
| [function-max-lines](./rules/best-practises/function-max-lines.md) | Function body contains "count" lines but allowed no more than maxlines. | | |
| [max-line-length](./rules/best-practises/max-line-length.md) | Line length must be no more than maxlen. | | |
Expand Down Expand Up @@ -38,6 +39,7 @@ title: "Rule Index of Solhint"
| [const-name-snakecase](./rules/naming/const-name-snakecase.md) | Constant name must be in capitalized SNAKE_CASE. (Does not check IMMUTABLES, use immutable-vars-naming) | $~~~~~~~~$✔️ | |
| [contract-name-camelcase](./rules/naming/contract-name-camelcase.md) | Contract name must be in CamelCase. | $~~~~~~~~$✔️ | |
| [event-name-camelcase](./rules/naming/event-name-camelcase.md) | Event name must be in CamelCase. | $~~~~~~~~$✔️ | |
| [foundry-test-functions](./rules/naming/foundry-test-functions.md) | Enforce naming convention on functions for Foundry test cases | | |
| [func-name-mixedcase](./rules/naming/func-name-mixedcase.md) | Function name must be in mixedCase. | $~~~~~~~~$✔️ | |
| [func-named-parameters](./rules/naming/func-named-parameters.md) | Enforce named parameters for function calls with 4 or more arguments. This rule may have some false positives | | |
| [func-param-name-mixedcase](./rules/naming/func-param-name-mixedcase.md) | Function param name must be in mixedCase. | | |
Expand Down
71 changes: 71 additions & 0 deletions docs/rules/best-practises/custom-errors.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
---
warning: "This is a dynamically generated file. Do not edit manually."
layout: "default"
title: "custom-errors | Solhint"
---

# custom-errors
![Recommended Badge](https://img.shields.io/badge/-Recommended-brightgreen)
![Category Badge](https://img.shields.io/badge/-Best%20Practise%20Rules-informational)
![Default Severity Badge warn](https://img.shields.io/badge/Default%20Severity-warn-yellow)
> The {"extends": "solhint:recommended"} property in a configuration file enables this rule.


## Description
Enforces the use of Custom Errors over Require and Revert statements

## Options
This rule accepts a string option of rule severity. Must be one of "error", "warn", "off". Default to warn.

### Example Config
```json
{
"rules": {
"custom-errors": "warn"
}
}
```


## Examples
### 👍 Examples of **correct** code for this rule

#### Use of Custom Errors

```solidity
revert CustomErrorFunction();
```

#### Use of Custom Errors with arguments

```solidity
revert CustomErrorFunction({ msg: "Insufficient Balance" });
```

### 👎 Examples of **incorrect** code for this rule

#### Use of require statement

```solidity
require(userBalance >= availableAmount, "Insufficient Balance");
```

#### Use of plain revert statement

```solidity
revert();
```

#### Use of revert statement with message

```solidity
revert("Insufficient Balance");
```

## Version
This rule is introduced in the latest version.

## Resources
- [Rule source](https://github.com/protofire/solhint/tree/master/lib/rules/best-practises/custom-errors.js)
- [Document source](https://github.com/protofire/solhint/tree/master/docs/rules/best-practises/custom-errors.md)
- [Test cases](https://github.com/protofire/solhint/tree/master/test/rules/best-practises/custom-errors.js)
2 changes: 1 addition & 1 deletion docs/rules/best-practises/explicit-types.md
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ uint256 public variableName
```

## Version
This rule is introduced in the latest version.
This rule was introduced in [Solhint 3.5.1](https://github.com/protofire/solhint/tree/v3.5.1)

## Resources
- [Rule source](https://github.com/protofire/solhint/tree/master/lib/rules/best-practises/explicit-types.js)
Expand Down
2 changes: 1 addition & 1 deletion docs/rules/best-practises/no-unused-import.md
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ This rule accepts a string option of rule severity. Must be one of "error", "war
```

## Version
This rule is introduced in the latest version.
This rule was introduced in [Solhint 3.5.1](https://github.com/protofire/solhint/tree/v3.5.1)

## Resources
- [Rule source](https://github.com/protofire/solhint/tree/master/lib/rules/best-practises/no-unused-import.js)
Expand Down
73 changes: 73 additions & 0 deletions docs/rules/naming/foundry-test-functions.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
---
warning: "This is a dynamically generated file. Do not edit manually."
layout: "default"
title: "foundry-test-functions | Solhint"
---

# foundry-test-functions
![Category Badge](https://img.shields.io/badge/-Style%20Guide%20Rules-informational)
![Default Severity Badge off](https://img.shields.io/badge/Default%20Severity-off-undefined)

## Description
Enforce naming convention on functions for Foundry test cases

## Options
This rule accepts an array of options:

| Index | Description | Default Value |
| ----- | ----------------------------------------------------- | ------------- |
| 0 | Rule severity. Must be one of "error", "warn", "off". | off |


### Example Config
```json
{
"rules": {
"foundry-test-functions": ["off",["setUp"]]
}
}
```

### Notes
- This rule can be configured to skip certain function names in the SKIP array. In Example Config. ```setUp``` function will be skipped
- Supported Regex: ```test(Fork)?(Fuzz)?(Fail)?_(Revert(If_|When_){1})?\w{1,}```
- This rule should be executed in a separate folder with a separate .solhint.json => ```solhint --config .solhint.json testFolder/**/*.sol```
- This rule applies only to `external` and `public` functions
- This rule skips the `setUp()` function

## Examples
### 👍 Examples of **correct** code for this rule

#### Foundry test case with correct Function declaration

```solidity
function test_NumberIs42() public {}
```

#### Foundry test case with correct Function declaration

```solidity
function testFail_Subtract43() public {}
```

#### Foundry test case with correct Function declaration

```solidity
function testFuzz_FuzzyTest() public {}
```

### 👎 Examples of **incorrect** code for this rule

#### Foundry test case with incorrect Function declaration

```solidity
function numberIs42() public {}
```

## Version
This rule is introduced in the latest version.

## Resources
- [Rule source](https://github.com/protofire/solhint/tree/master/lib/rules/naming/foundry-test-functions.js)
- [Document source](https://github.com/protofire/solhint/tree/master/docs/rules/naming/foundry-test-functions.md)
- [Test cases](https://github.com/protofire/solhint/tree/master/test/rules/naming/foundry-test-functions.js)
2 changes: 1 addition & 1 deletion docs/rules/naming/func-named-parameters.md
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ functionName(_senderAddress, 1e18, _tokenAddress, _receiverAddress )
```

## Version
This rule is introduced in the latest version.
This rule was introduced in [Solhint 3.5.1](https://github.com/protofire/solhint/tree/v3.5.1)

## Resources
- [Rule source](https://github.com/protofire/solhint/tree/master/lib/rules/naming/func-named-parameters.js)
Expand Down
2 changes: 1 addition & 1 deletion docs/rules/naming/immutable-vars-naming.md
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ This rule accepts an array of options:
This rule does not have examples.

## Version
This rule is introduced in the latest version.
This rule was introduced in [Solhint 3.5.1](https://github.com/protofire/solhint/tree/v3.5.1)

## Resources
- [Rule source](https://github.com/protofire/solhint/tree/master/lib/rules/naming/immutable-vars-naming.js)
Expand Down
7 changes: 7 additions & 0 deletions e2e/07-foundry-test/.solhint.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"extends": "solhint:recommended",
"rules": {
"compiler-version": "off",
"func-visibility": "off"
}
}
10 changes: 10 additions & 0 deletions e2e/07-foundry-test/contracts/Foo.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
// SPDX-License-Identifier: MIT
pragma solidity >=0.6.0;

contract Foo {
uint256 public constant TEST = 1;

constructor() {

}
}
5 changes: 5 additions & 0 deletions e2e/07-foundry-test/test/.solhint.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"rules": {
"foundry-test-functions": ["error", ["setUp", "finish"]]
}
}
20 changes: 20 additions & 0 deletions e2e/07-foundry-test/test/FooTest.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
// SPDX-License-Identifier: MIT
pragma solidity >=0.6.0;

import "./Test.sol";

contract FooTest is Test {
uint256 testNumber;

function setUp() public {
testNumber = 42;
}

function testFail_Subtract43() public {
testNumber -= 43;
}

function wrongFunctionDefinitionName() public {
testNumber -= 43;
}
}
4 changes: 4 additions & 0 deletions e2e/07-foundry-test/test/Test.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
// SPDX-License-Identifier: MIT
pragma solidity >=0.6.0;

contract Test {}
22 changes: 22 additions & 0 deletions e2e/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -147,4 +147,26 @@ describe('e2e', function () {
expect(stdout.trim()).to.not.contain(errorFound)
})
})

describe('Linter - foundry-test-functions with shell', () => {
// Foo contract has 1 warning
// FooTest contract has 1 error
useFixture('07-foundry-test')

it(`should raise error for empty blocks only`, () => {
const { code, stdout } = shell.exec('solhint contracts/Foo.sol')

expect(code).to.equal(0)
expect(stdout.trim()).to.contain('Code contains empty blocks')
})

it(`should raise error for wrongFunctionDefinitionName() only`, () => {
const { code, stdout } = shell.exec('solhint -c test/.solhint.json test/FooTest.sol')

expect(code).to.equal(1)
expect(stdout.trim()).to.contain(
'Function wrongFunctionDefinitionName() must match Foundry test naming convention'
)
})
})
})
5 changes: 5 additions & 0 deletions lib/common/identifier-naming.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,4 +30,9 @@ module.exports = {
hasLeadingUnderscore(text) {
return text && text[0] === '_'
},

isNotFoundryTestCase(text) {
const regex = /^test(Fork)?(Fuzz)?(Fail)?_(Revert(If_|When_){1})?\w{1,}$/
return !match(text, regex)
},
}
6 changes: 6 additions & 0 deletions lib/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,4 +35,10 @@ module.exports = {
getString(ruleName, defaultValue) {
return this.getStringByPath(`rules["${ruleName}"][1]`, defaultValue)
},

getArray(ruleName, defaultValue) {
const path = `rules["${ruleName}"][1]`
const configVal = _.get(this, path)
return _.isArray(configVal) ? configVal : defaultValue
},
}
Loading
Loading