refactor: rename tests with error assertions #161
Conversation
postmeback
requested review from
alexfertel,
bidzyyys and
qalisander
as code owners
✅ Deploy Preview for contracts-stylus canceled.
|
There was a problem hiding this comment.
Hey @postmeback!
Appreciate your contribution!
Here are few comments I have:
| @@ -1141,21 +1141,18 @@ mod tests { | |||
| } | |||
|
|
|||
| #[motsu::test] | |||
| fn balance_of_zero_balance(contract: Erc721) { | |||
| fn error_when_contract_balance_is_zero_balance(contract: Erc721) { | |||
There was a problem hiding this comment.
I can see this test is not checking any error. I think we are not expecting to change its name. cc @bidzyyys
There was a problem hiding this comment.
Yep, we should rename only when we check errors in tests.
| contract | ||
| ._mint(alice, token_id) | ||
| .expect("should mint the token a first time"); | ||
| .expect("should mint the token the first time"); |
| @@ -1165,14 +1162,13 @@ mod tests { | |||
| } | |||
|
|
|||
| #[motsu::test] | |||
| fn mints(contract: Erc721) { | |||
| fn when_minting_token_balance_and_owner_are_correct(contract: Erc721) { | |||
There was a problem hiding this comment.
In this test we're also not checking for any error. Seems make sense to leave it as it is. cc @bidzyyys
There was a problem hiding this comment.
mints is fine here. The task is to rename tests that checks errors.
qalisander
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files
|
|
Hey @postmeback! We're really sorry about this, but after some discussion, we found that renaming the tests this way hides away the intent of the test, so we don't think it's worth the trade-off. As such, I'll be closing the PR, since we won't be proceeding with these changes anymore. Hopefully this doesn't deter you from contributing to our library! Thanks. |
Changes as per the mentioned issue. It's a work in progress, requesting a quick review. Will continue working on it, if the initial changes are fine.
Resolves #155
PR Checklist
The below items are not applicable as such, because it is more about renaming tests to a particular naming convention.