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

chore: conform test naming to style guide #433

Merged
merged 6 commits into from
Aug 22, 2023

Conversation

Sabnock01
Copy link
Contributor

This PR puts the forge-std test naming conventions in line with the style guide

Some liberties have been taken with respect to the names in StdAssertions.t.sol but open to any feedback.

I did not add any test names with the Revert[If|When] modifier since none of the tests appear to pass or fail conditionally. Correct me if this is erroneous.

Closes #273

@mds1
Copy link
Collaborator

mds1 commented Aug 14, 2023

I did not add any test names with the Revert[If|When] modifier since none of the tests appear to pass or fail conditionally.

Sorry what do you mean by this bolded part? Also just to clarify, the Revert[If|When] convention doesn't affect execution, it just signals you're testing that something should revert. This is useful because for any time you want to filter happy path vs. failure cases, such as excluding reverting tests when generating gas reports

test/StdAssertions.t.sol Outdated Show resolved Hide resolved
@mds1
Copy link
Collaborator

mds1 commented Aug 17, 2023

This is great, thanks @Sabnock01! Just about ready to merge, but want to bump the question in #433 (comment) first

@Sabnock01
Copy link
Contributor Author

This is great, thanks @Sabnock01! Just about ready to merge, but want to bump the question in #433 (comment) first

Ah, no problem. I was trying to say that specific syntax didn't appear necessary for any of the current tests but I don't recall specifically why I worded it the way I did.

@mds1
Copy link
Collaborator

mds1 commented Aug 17, 2023

Ah got it. Yea of course it’s not strictly necessary, but since we’re updating to follow the best practices doc conventions we may as well use that one too, for tests that are focused on checking revert conditions. Would you mind using that where applicable?

@Sabnock01
Copy link
Contributor Author

Ah got it. Yea of course it’s not strictly necessary, but since we’re updating to follow the best practices doc conventions we may as well use that one too, for tests that are focused on checking revert conditions. Would you mind using that where applicable?

Sure, can do that.

Copy link
Collaborator

@mds1 mds1 left a comment

Choose a reason for hiding this comment

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

Thanks so much!

@mds1 mds1 merged commit 4b8d29e into foundry-rs:master Aug 22, 2023
3 checks passed
@Sabnock01 Sabnock01 deleted the sabnock/273 branch August 22, 2023 15:05
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.

Rename the test functions to adhere to the Foundry best practices
2 participants