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

doc: update error-compare and define unwrapped-error #162

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

doc: update error-compare and define unwrapped-error #162

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 22 additions & 3 deletions CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ Describe a new checker in [checkers section](./README.md#checkers).
- [require-len](#require-len)
- [suite-test-name](#suite-test-name)
- [useless-assert](#useless-assert)

- [unwrapped-error](#unwrapped-error)
---

### elements-match
Expand All @@ -169,22 +169,41 @@ Describe a new checker in [checkers section](./README.md#checkers).

---

### unwrapped-error

```go
❌ assert.Contains(t, err.Error(), "not found")
assert.Equal(t, err.Error(), "user not found")

✅ assert.ErrorContains(t, err, "not found")
assert.EqualError(t, err, "user not found")
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit afraid this one is valid

Copy link
Contributor Author

@mmorel-35 mmorel-35 Jul 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

You probably need a rule that says

If I have

var ErrUserNotFound = errors.New("user not found")
...
assert.EqualError(t, err, "user not found")

Then you shall replace it with

var ErrUserNotFound = errors.New("user not found")
...
assert.ErrorIs(t, err, ErrUserNotFound)

Copy link
Contributor

@ccoVeille ccoVeille Jul 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

My point is that ErrorEqual should be part of the same checker than ErrorContains.

It should be discouraged

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, we probably need to change namings then . Maybe error-compare for those two and something else for the use of Error()

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe, let's wait for Anton's feedback

Copy link
Contributor

Choose a reason for hiding this comment

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

@Antonboom maybe you missed this old thread

4cb531e#r1661489366

You said ErrorContains is an antipattern, what is your point of view about ErrorEqual ?

assert.ErrorIs(t, err, ErrUserNotFound)
assert.NotErrorIs(t, err, errSentinel)
```

**Autofix**: true. <br>
**Enabled by default**: true. <br>
**Reason**: Using testify methods will avoid the risk of nil pointer exception that might happen with the use of the `Error()` method on the `error` interface. <br>
**Related issues**: [#47](https://github.com/Antonboom/testifylint/issues/47)

---

### error-compare

```go
❌ assert.ErrorContains(t, err, "not found")
assert.EqualError(t, err, "user not found")
assert.Equal(t, err.Error(), "user not found")
assert.Equal(t, err, errSentinel) // Through `reflect.DeepEqual` causes error strings to be compared.
assert.NotEqual(t, err, errSentinel)
// etc.

✅ assert.ErrorIs(t, err, ErrUserNotFound)
assert.ErrorAs(t, err, &target)
```

**Autofix**: false. <br>
**Enabled by default**: true. <br>
**Reason**: The `Error()` method on the `error` interface exists for humans, not code. <br>
**Reason**: The `assert.ErrorContains()` is commonly used as an anti-pattern. The code and tests often needs to be adapted to be able to use `assert.ErrorIs()` or `assert.ErrorAs()` methods. <br>
**Related issues**: [#47](https://github.com/Antonboom/testifylint/issues/47)

---
Expand Down