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.

Sign up for GitHub

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

Jump to bottom

debt: deprecate testFail* #4437

Open
Tracked by #8574
rafales opened this issue Feb 27, 2023 · 4 comments · May be fixed by #9574
Open
Tracked by #8574

debt: deprecate testFail* #4437

rafales opened this issue Feb 27, 2023 · 4 comments · May be fixed by #9574
Labels
A-internals Area: internals C-forge Command: forge Cmd-forge-test Command: forge test T-debt Type: code debt T-likely-breaking Type: requires changes that can be breaking
Milestone
v1.0.0

Comments

@rafales
Copy link

rafales commented Feb 27, 2023

Component

Forge

Describe the feature you would like

Hey. I just ran into a problem that wasted more time that I'm willing to admit.

Foundry has a feature where tests named testFailSomething are expected to fail. It's easy to forget about it.

So I found myself in a situation where test is failing for some unknown reason and all I get is this message:

[FAIL. Reason: Assertion failed.]

It would be good if this particular failure had a more user-friendly message, pointing to the reason behind the failure. It's easy to come up with a test name like testFailsWhenCalledContractFailed and not know about the testFailXXX feature.

Message like FAIL. Reason: test was expected to revert, but it did not. or something similar.

Additional context

No response
@rafales rafales added the T-feature Type: feature label Feb 27, 2023
@gakonst gakonst added this to Foundry Feb 27, 2023
@github-project-automation github-project-automation bot moved this to Todo in Foundry Feb 27, 2023
@mds1
Copy link
Collaborator

mds1 commented Feb 27, 2023

@gakonst We should probably just remove testFail at this point, things like this have happened a few times and I don't think we need support for it anymore. Similar to #3153 (comment), we should look for tests named testFail* and print an issue/warning to inform users not to use this test name/format.

@rafales In the meantime I'd suggest following the naming conventions in https://book.getfoundry.sh/tutorials/best-practices, which are test_RevertIf_Condition

@mds1 mds1 added Cmd-forge-test Command: forge test C-forge Command: forge T-debt Type: code debt labels Feb 27, 2023
@abhiram11
Copy link

We can prefer to use vm.expectRevert() in the function itself, rather than naming a function that starts with testFail. I feel it is possible that testFail may be deprecated so it's better to change this developer practice asap.

@palinatolmach palinatolmach mentioned this issue Jan 11, 2024
Merged
@zerosnacks zerosnacks added this to the v1.0.0 milestone Jul 26, 2024
@zerosnacks zerosnacks changed the title Better message when testFailXXX test doesn't fail chore: raise warning upon encountering a test function with the testFail* prefix Sep 16, 2024
@zerosnacks zerosnacks added A-internals Area: internals and removed T-feature Type: feature labels Sep 16, 2024
@zerosnacks
Copy link
Member

Related: foundry-rs/book#813

@JohnGuilding JohnGuilding mentioned this issue Oct 13, 2024
Merged
@grandizzy grandizzy mentioned this issue Oct 30, 2024
Closed
@grandizzy grandizzy added the T-likely-breaking Type: requires changes that can be breaking label Nov 21, 2024
@zerosnacks zerosnacks changed the title chore: raise warning upon encountering a test function with the testFail* prefix chore: deprecate testFail Nov 21, 2024
@zerosnacks
Copy link
Member

Renamed the title, we are intending to deprecate testFail* in a breaking way for v1.0

@zerosnacks zerosnacks changed the title chore: deprecate testFail chore: deprecate testFail* Nov 21, 2024
@zerosnacks zerosnacks changed the title chore: deprecate testFail* debt: deprecate testFail* Nov 21, 2024
@grandizzy grandizzy mentioned this issue Dec 6, 2024
Open
13 tasks
@yash-atreya yash-atreya linked a pull request Dec 18, 2024 that will close this issue
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-internals Area: internals C-forge Command: forge Cmd-forge-test Command: forge test T-debt Type: code debt T-likely-breaking Type: requires changes that can be breaking
Projects
Foundry
Archived in project
Milestone
v1.0.0
Development

Successfully merging a pull request may close this issue.

debt(forge): remove testFail* foundry-rs/foundry
5 participants
@rafales @mds1 @abhiram11 @grandizzy @zerosnacks