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

Suggestion to change test naming best practices #1279

Closed
milosdjurica opened this issue Sep 4, 2024 · 1 comment
Closed

Suggestion to change test naming best practices #1279

milosdjurica opened this issue Sep 4, 2024 · 1 comment
Labels
T-enhancement Type: enhance existing docs

Comments

@milosdjurica
Copy link

In the Foundry book under Best Practices it is suggested that if we "Have a Test contract per contract-under-test", then we should name our unit tests like this :

Citing the docs :

  • contract MyContractTest holds all unit tests for MyContract.
  • function test_add_AddsTwoNumbers() lives within MyContractTest to test the add method.
  • function test_supply_UsersCanSupplyTokens() also lives within MyContractTest to test the supply method.

I came to the conclusion that general approach is: test_FunctionName_Description

But later in the docs, when best practices are mentioned, FunctionName part is not used.

  • test_Description for standard tests.
  • testFuzz_Description for fuzz tests.
  • test_Revert[If|When]_Condition for tests expecting a revert.
  • testFork_Description for tests that fork from a network.
  • testForkFuzz_Revert[If|When]_Condition for a fuzz test that forks and expects a revert.

My suggestion is to change best practices from example above to ->

  • test_FunctionName_Description for standard tests.
  • testFuzz_FunctionName_Description for fuzz tests.
  • test_FunctionName_Revert[If|When]_Condition for tests expecting a revert.
  • testFork_FunctionName_Description for tests that fork from a network.
  • testForkFuzz_FunctionName_Revert[If|When]_Condition for a fuzz test that forks and expects a revert.

When user is testing multiple functions in a single test, he should omit FunctionName part and name the test like this : test_Description (old/current way of naming tests).

I think this would provide more clarity to the test naming conventions and it would make it easier to filter tests.

This way user can filter tests only for a specific function AND/OR tests that RevertIf , etc etc...
forge test --mt FunctionName , forge test --mt RevertIf , forge test --mt FunctionName_RevertIf , all those comands work as expected.

@zerosnacks
Copy link
Member

zerosnacks commented Sep 13, 2024

Thanks for your suggestion!

In practice I've noticed that most non-unit tests cover multiple functions in non-isolation

I think for unit tests it is fine to add the function name, for other types of tests this is not practical

The general recommendations still stand, but consider the function name optional

Marking as not planned

@zerosnacks zerosnacks closed this as not planned Won't fix, can't repro, duplicate, stale Sep 13, 2024
@github-project-automation github-project-automation bot moved this from Todo to Done in Foundry Book Sep 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-enhancement Type: enhance existing docs
Projects
No open projects
Status: Done
Development

No branches or pull requests

2 participants