Rule for test function names

[PaulRBerg]

With the advent of Foundry, more and more Solidity projects are now tested in Solidity itself. This has led to the community coalescing around several conventions, such as using the following regex for test function names:

test(Fork)?(Fuzz)?_(Revert(If_|When_){1})?\w{1,}

This is recommended in the Best Practices tutorial of the Foundry Book.

Here are a few examples:

• test_Description
• test_RevertWhen_Condition
• testFuzz_Description
• testForkFuzz_RevertWhen_Condition

It would be nice if there were a Solhint rule for this - and even nicer if the rule could be configured to apply only to contracts in specific directories (e.g. in test).

[dbale-altoros]

@PaulRBerg you mean a rule to do what ?
check that the prefix of any function is with one of those for testing ?

[PaulRBerg]

check that the prefix of any function is with one of those for testing ?

Yes. A rule that would activate the regex above.

[fvictorio]

Some thoughts on this.

First, I guess the behavior of the new rule should be: "If a (public?) function starts with test, then check that it follows the naming convention". Not sure about the public part, but I guess it makes sense.

Second, having some way to only apply the rule for files in a given directory makes a lot of sense, but I'm not sure what's the best way to accomplish it. Possible solutions:

1. The best way IMO would be to have a separate .solhint.json file inside the test directory, that extends the config file in the parent directory. This is a useful pattern, and the way this is done in eslint-land. But I'm pretty sure that solhint doesn't support this (it should).
2. Alternatively, the rule could have a testDirectory option, or something like that. Since I think the previous alternative is way better (and useful in other contexts), I believe adding this option would eventually become dead weight. But it's certainly the easiest way to accomplish this. Idk.

A middle ground would be this: have two solhint files .solhint.json and .solhint-tests.json, and make the second one extend the first (this is also not supported, but it's easier to do). Then have two npm scripts: lint and lint-tests (or whatever) and make the second one explicitly use the second config file and the tests directory.

[PaulRBerg]

Foundry considers only public or external functions tests, so I agree about the public qualifier.

I agree that the best way to add support for this rule would be to be able to nested Solhint configurations in folders, but I would smile upon having one of the interim solutions you mentioned (e.g. .solhint.test.json).

[dbale-altoros]

@PaulRBerg
Shouldn't be a regex like this
test(Fork)?(Fuzz)?(Fail)_(Revert(If_|When_){1})?\w{1,}
instead of
test(Fork)?(Fuzz)?_(Revert(If_|When_){1})?\w{1,}

Seems like testFail is missing
https://book.getfoundry.sh/forge/writing-tests

[PaulRBerg]

@dbale-altoros testFail is supposed to be deprecated IIRC

But yes I think for backward compatibility, it would be helpful to add it in the regex.

btw thanks for shipping so many features lately!

[dbale-altoros]

oh, did not know it was supposed to be deprecated
thanks for your answer

[dbale-altoros]

I read all your comments.
This pr #476
Will implement a rule like this one
But the configuration of such a rule needs to be improved with one of the approaches you mentioned

For now, to speed up the process and in order to use it, two .solhint.json should be used to avoid false positives in regular contracts (not foundry tests)
Other .solhint.json should be used in the Foundry TEST directory implementing this rule
By default will be off

Rule configuration is as specified in readme

Feedback appreciate

[dbale-altoros]

new release this Friday !!

[pcaversaccio]

Just as feedback, I always use the naming scheme testFunctionName (without the _), which leads to warnings in the current version. Also, for fuzzing, I use testFuzzFunctionName. Furthermore, I want to emphasise that there are also so-called invariant tests that will throw warnings currently, see e.g. here. Eventually, the terminology invariant will be aliased (and later replaced) via statefulFuzz (see foundry-rs/foundry#4922).

[dbale-altoros]

@pcaversaccio thanks a lot for your feedback, I really appreciate
I will try to fix this...
So seems like the underscore is optional on every case, right ?

And from what I read... the invariant tests and future statefulFuzz will not start with the prefix test at all... Is that correct ?

[pcaversaccio]

So seems like the underscore is optional on every case, right ?

Correct, that's at least how I personally use it.

And from what I read... the invariant tests and future statefulFuzz will not start with the prefix test at all... Is that correct ?

Also correct, from the docs:

[dbale-altoros]

Thanks again @pcaversaccio

What is kind of confusing is that on every foundry doc, there's always an underscore before the description (or function name)...
Even in that invariant_A() there is an underscore as well
So disregarding the way you use it (not trying to be harsh at all, just want to understand)
Seems that the prefix for foundry to evaluate a function as a test is just the word test (or invariant) without the underscore

[pcaversaccio]

Thanks again @pcaversaccio

What is kind of confusing is that on every foundry doc, there's always an underscore before the description (or function name)... Even in that invariant_A() there is an underscore as well So disregarding the way you use it (not trying to be harsh at all, just want to understand) Seems that the prefix for foundry to evaluate a function as a test is just the word test (or invariant) without the underscore

Well not only me :), e.g. OpenZeppelin uses the same naming scheme: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/test/utils/math/Math.t.sol

[dbale-altoros]

Of course!
Sorry if I did not explained myself well
I meant that I want to make a rule that can fulfill foundry requirement to be taken as a test
And seems like the underscore it is not a mandatory thing... just the prefix keyword test and that's it

[pcaversaccio]

Of course!
Sorry if I did not explained myself well
I meant that I want to make a rule that can fulfill foundry requirement to be taken as a test
And seems like the underscore it is not a mandatory thing... just the prefix keyword test and that's it

Ah yes, only test is required. The origin can be traced back to dapptools which introduced this semantics:

Every function prefixed with test is expected to succeed, while functions prefixed by testFail are expected to fail.

[dbale-altoros]

THANKS!!!

[dbale-altoros]

Fixed here
#484
feedback appreciated
Friday or Monday, next release with this fix

[PaulRBerg]

having some way to only apply the rule for files in a given directory makes a lot of sense, but I'm not sure what's the best way to accomplish it.

Any updates on this feature? I've just upgraded to the latest Solhint and there are a bunch of rules that I would like activated in src but not in test, for instance:

• custom-errors
• one-contract-per-file
• immutable-vars-naming

For the full terminal log, see this.

[pcaversaccio]

having some way to only apply the rule for files in a given directory makes a lot of sense, but I'm not sure what's the best way to accomplish it.

Any updates on this feature? I've just upgraded to the latest Solhint and there are a bunch of rules that I would like activated in src but not in test, for instance:

• custom-errors
• one-contract-per-file
• immutable-vars-naming

For the full terminal log, see this.

My repo is not public so can't share it but I use two solhint files: one in the src directory with the name .solhint.json and another one in the test directory with the name .solhint-tests.json and I've configured the following npm scripts:

"solhint:check": "npx solhint -c src/.solhint.json src/**/*.sol && npx solhint -c test/.solhint-tests.json test/**/*.sol",
"solhint:fix": "npx solhint -c src/.solhint.json src/**/*.sol --fix && npx solhint -c test/.solhint-tests.json test/**/*.sol --fix",

This does the job for me now.

[dbale-altoros]

@PaulRBerg
For now I would recommend to use two .solhint.json
We are now studying a new set of rules to implement
Not sure when can we add this feature given the fact it can be accomplished in another way....
Or maybe I'm missing something you need ?