Skip to content

Commit

Permalink
cosmetic before publication
Browse files Browse the repository at this point in the history
  • Loading branch information
Antonboom committed Sep 7, 2023
1 parent 0c35eb7 commit 600ec56
Show file tree
Hide file tree
Showing 8 changed files with 64 additions and 51 deletions.
15 changes: 0 additions & 15 deletions .golangci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -64,13 +64,9 @@ linters:
- asasalint
- asciicheck
- bidichk
- bodyclose
- depguard
- durationcheck
- errcheck
- errchkjson
- errname
- execinquery
- exhaustive
- exportloopref
- forbidigo
Expand All @@ -82,41 +78,30 @@ linters:
- godox
- gofmt
- gofumpt
- goheader
- goimports
- gomoddirectives
- gomodguard
- goprintffuncname
- gosec
- gosimple
- govet
- importas
- ineffassign
- lll
- makezero
- mirror
- misspell
- musttag
- nakedret
- nilerr
- nestif
- noctx
- nolintlint
- nosprintfhostport
- prealloc
- predeclared
- reassign
- revive
- rowserrcheck
- sqlclosecheck
- staticcheck
- stylecheck
- tagliatelle
- tenv
- testableexamples
- testpackage
- thelper
- typecheck
- unconvert
- unparam
- unused
Expand Down
38 changes: 21 additions & 17 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -55,15 +55,15 @@ var registry = checkersRegistry{
}
```

By default, we disabled checker, because it's honestly a matter of taste.
By default, we disable the checker if we doubt its 100% usefulness.

### 4) Create new tests generator in `internal/testgen/gen_{checker_name}.go`

Create a new `ZeroTestsGenerator` in `internal/testgen/gen_zero.go`.
Create new `ZeroTestsGenerator` in `internal/testgen/gen_zero.go`.

See examples in adjacent files.

In the first iteration, these can be a very simple tests for debugging and checker's proof of concept.
In the first iteration, these can be a very simple tests for debugging checker's proof of concept.
And after the implementation of the checker, you can add various cycles, variables, etc. to the template.

`GoldenTemplate` is usually an `ErroredTemplate` with some strings replaced.
Expand All @@ -73,7 +73,7 @@ And after the implementation of the checker, you can add various cycles, variabl
### 6) Generate new tests

```bash
$ task tests:gen
$ task test:gen
Generate analyzer tests...
```

Expand All @@ -82,6 +82,9 @@ Generate analyzer tests...
`Zero` is an example of `checkers.RegularChecker` because it works with "general" assertion call.
For more complex checkers, use the `checkers.AdvancedChecker` interface.

If the checker turns out to be too “fat”, then you can omit some obviously rare combinations,
especially if they are covered by other checkers. Usually these are expressions in `assert.True/False`.

### 8) Improve tests from p.4 if necessary

Pay attention to `Assertion` and `NewAssertionExpander`, but keep your tests as small as possible.
Expand All @@ -104,8 +107,6 @@ Fix linter issues and broken tests (probably related to the checkers registry).

### 10) Update the `Checkers` section in `README.md`, commit the changes and submit a pull request 🔥

---

# Open for contribution

- [elements-match](#elements-match)
Expand Down Expand Up @@ -173,7 +174,7 @@ Fix linter issues and broken tests (probably related to the checkers registry).
```

**Autofix**: true. <br>
**Enabled by default**: false. <br>
**Enabled by default**: maybe? <br>
**Reason**: Code simplification.

---
Expand Down Expand Up @@ -215,7 +216,7 @@ type Tx struct {
And similar idea for `assert.InEpsilonSlice` / `assert.InDeltaSlice`.

**Autofix**: false. <br>
**Enabled by default**: true, but configurable. <br>
**Enabled by default**: true. <br>
**Reason**: Work with floating properly.

---
Expand All @@ -232,7 +233,7 @@ And similar idea for `assert.InEpsilonSlice` / `assert.InDeltaSlice`.
```

**Autofix**: true. <br>
**Enabled by default**: false. <br>
**Enabled by default**: true. <br>
**Reason**: Is similar to the [usestdlibvars](https://golangci-lint.run/usage/linters/#usestdlibvars) linter.

---
Expand All @@ -254,7 +255,7 @@ And similar idea for `assert.InEpsilonSlice` / `assert.InDeltaSlice`.
```

**Autofix**: true. <br>
**Enabled by default**: false. <br>
**Enabled by default**: maybe? <br>
**Reason**: Code simplification.

---
Expand All @@ -270,15 +271,15 @@ And similar idea for `assert.InEpsilonSlice` / `assert.InDeltaSlice`.
```

**Autofix**: true. <br>
**Enabled by default**: false. <br>
**Enabled by default**: maybe? <br>
**Reason**: More appropriate `testify` API with clearer failure message.

---

### no-fmt-mess

**Autofix**: true. <br>
**Enabled by default**: maybe.
**Enabled by default**: maybe?

Those who are new to `testify` may be discouraged by the duplicative API:

Expand Down Expand Up @@ -321,13 +322,11 @@ then before that there must be a length constraint through `require`.
```

**Autofix**: false? <br>
**Enabled by default**: true. <br>
**Enabled by default**: maybe? <br>
**Reason**: Similar to [require-error](README.md#require-error). Save you from annoying panics.

Or maybe do something similar for maps? And come up with better name for the checker.

TODO: + require.Empty

### suite-run

```go
Expand All @@ -347,7 +346,7 @@ func (s *Suite) TestSomething() {
```

**Autofix**: true. <br>
**Enabled by default**: probably true. <br>
**Enabled by default**: probably yes. <br>
**Reason**: Code simplification and consistency.

But need to investigate the technical difference and the reasons for the appearance of `s.Run`.
Expand Down Expand Up @@ -405,7 +404,7 @@ func (s *HandlersSuite) Test_UsecaseSuccess()
```

**Autofix**: true. <br>
**Enabled by default**: false.
**Enabled by default**: false. <br>
**Reason**: Just for your reflection and suggestion.

I'm not sure if anyone uses `assert.Zero` – it looks strange and conflicts with `assert.Empty`:
Expand All @@ -420,3 +419,8 @@ I'm not sure if anyone uses `assert.Zero` – it looks strange and conflicts wit

Maybe it's better to make configurable support for other types in the `empty` checker and
vice versa to prohibit the `Zero`?

---

Any other figments of your imagination are welcome 🙏<br>
List of `testify` functions [here](https://pkg.go.dev/github.com/stretchr/testify@master/assert#pkg-functions).
37 changes: 30 additions & 7 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,15 @@

Checks usage of [github.com/stretchr/testify](https://github.com/stretchr/testify).

## Table of Contents

* [Problem statement](#problem-statement)
* [Installation & usage](#installation--usage)
* [Configuring](#configuring)
* [Checkers](#checkers)
* [Chain of warnings](#chain-of-warnings)
* [testify v2](#testify-v2)

## Problem statement

Tests are also program code and the requirements for them should not differ much from the requirements
Expand Down Expand Up @@ -56,7 +65,7 @@ $ testifylint --enable=suite-extra-assert-call --suite-extra-assert-call.mode=re
| [len](#len) |||
| [require-error](#require-error) |||
| [suite-dont-use-pkg](#suite-dont-use-pkg) |||
| [suite-extra-assert-call](#suite-extra-assert-call) | ||
| [suite-extra-assert-call](#suite-extra-assert-call) | ||
| [suite-thelper](#suite-thelper) |||

---
Expand Down Expand Up @@ -238,7 +247,7 @@ This checker is similar to the [floatcompare](https://github.com/golangci/golang

**Autofix**: false. <br>
**Enabled by default**: true. <br>
**Reason**: "Ignoring" errors is not the "Go way" and leads to further panics in the test, making it harder to debug.
**Reason**: Such "ignoring" of errors leads to further panics, making the test harder to debug.

`tesitfy/require` allows to stop test execution when a test fails.

Expand Down Expand Up @@ -288,7 +297,7 @@ func (s *MySuite) TestSomething() {
}
```

You can enable such behaviaor through `--suite-extra-assert-call.mode=require`.
You can enable such behavior through `--suite-extra-assert-call.mode=require`.

**Autofix**: true. <br>
**Enabled by default**: true, in the `remove` mode. <br>
Expand All @@ -313,21 +322,35 @@ func (s *RoomSuite) assertRoomRound(roundID RoundID) {

**Autofix**: true. <br>
**Enabled by default**: false. <br>
**Reason**: Consistency to non-suite test helpers. Explicit markup of helper methods.
**Reason**: Consistency with non-suite test helpers. Explicit markup of helper methods.

`s.T().Helper()` call is not important technically because `testify` prints full `Error Trace`
`s.T().Helper()` call is not important actually because `testify` prints full `Error Trace`
[anyway](https://github.com/stretchr/testify/blob/882382d845cd9780bd93c1acc8e1fa2ffe266ca1/assert/assertions.go#L317).

The checker rather acts as an example of a [checkers.AdvancedChecker](https://github.com/Antonboom/testifylint/blob/676324836555445fded4e9afc004101ec6f597fe/internal/checkers/checker.go#L56).

---

## testify V2
## Chain of warnings

Linter does not automatically handle the "evolution" of changes.
And in some cases may be dissatisfied with your code several times, for example:

```go
assert.True(err == nil) // compares: use assert.Equal
assert.Equal(t, err, nil) // error-nil: use assert.NoError
assert.NoError(t, err) // require-error: for error assertions use require
require.NoError(t, err)
```

Please [contribute](./CONTRIBUTING.md) if you have ideas on how to make this better.

## testify v2

The second version of `testify` [promises](https://github.com/stretchr/testify/issues/1089) more "pleasant" API and
makes some above checkers irrelevant.

It will not be difficult to add `v2` support in the linter in the future.
In this case, the possibility of supporting `v2` in the linter is not excluded.

But at the moment it looks like we are [extremely far](https://github.com/stretchr/testify/pull/1109#issuecomment-1650619745)
from `v2`. Related milestone [here](https://github.com/stretchr/testify/milestone/4).
12 changes: 6 additions & 6 deletions Taskfile.yml
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ tasks:
- task: tidy
- task: fmt
- task: lint
- task: tests
- task: test
- task: install

tools:install:
Expand All @@ -40,18 +40,18 @@ tasks:
- echo "Lint..."
- golangci-lint run --fix ./...

tests:
deps: [ tests:gen ]
test:
deps: [ test:gen ]
cmds:
- echo "Test..."
- go test ./...

tests:coverage:
test:coverage:
cmds:
- echo "Tests with coverage..."
- echo "Test with coverage..."
- go test -coverpkg={{ .COVERED_PKGS | trim | splitLines | join "," }} -coverprofile=coverage.out ./...

tests:gen:
test:gen:
cmds:
- echo "Generate analyzer tests..."
- go run ./internal/testgen
Expand Down
2 changes: 1 addition & 1 deletion internal/testgen/assertion.go
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
package main

// Assertion is a generic view of testify assertion.
// Designed to expand to multiple lines of assertions using AssertionExpander.
// Designed to be expanded (by AssertionExpander) to multiple lines of assertions.
type Assertion struct {
Fn string // "Equal"
Argsf string // "%s, %s"
Expand Down
4 changes: 2 additions & 2 deletions internal/testgen/assertion_expander.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,13 +61,13 @@ func (e *AssertionExpander) NotFmtSingleMode() *AssertionExpander {
// "assert" + "Len" + "t, arr, 0" = "assert.Len(t, arr, 0)"
//
// - if Assertion.ReportMsgf defined then append analysistest.Run diagnostic comment, e.g.
// "assert.Len(t, arr, 0)" + "empty: use %s.%s" = `assert.Len(t, arr, 0) // want "use assert\\.Empty"`
// "assert.Len(t, arr, 0)" + "empty: use %s.%s" = `assert.Len(t, arr, 0) // want "empty: use assert\\.Empty"`
//
// For diagnostic comment formatting Expand uses Assertion.ProposedSelector and Assertion.ProposedFn
// or selector and Assertion.Fn if one of them is not defined. If you do not need formatting use `%.s` (or `%.0s`).
//
// The final string is copied in several variants, depending on the current AssertionExpander mode.
func (e *AssertionExpander) Expand(assrn Assertion, selector, testingTParam string, argValues []any) string { // -> FIXME []string
func (e *AssertionExpander) Expand(assrn Assertion, selector, testingTParam string, argValues []any) string {
fn, args := assrn.Fn, assrn.Argsf

if e.asGolden {
Expand Down
5 changes: 3 additions & 2 deletions internal/testgen/checker_name.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,17 @@ import "strings"
// CheckerName is a simple helper type useful for checker name transformations.
type CheckerName string

// AsPkgName transforms "suite-no-extra-assert-call" into "suitenoextraassertcall".
// AsPkgName transforms "suite-extra-assert-call" into "suiteextraassertcall".
func (n CheckerName) AsPkgName() string {
return strings.ReplaceAll(string(n), "-", "")
}

// AsTestName transforms "suite-no-extra-assert-call" into "TestSuiteNoExtraAssertCallChecker".
// AsTestName transforms "suite-extra-assert-call" into "TestSuiteExtraAssertCallChecker".
func (n CheckerName) AsTestName() string {
return "Test" + n.toCamelCase() + "Checker"
}

// AsSuiteName transforms "suite-extra-assert-call" into "SuiteExtraAssertCallCheckerSuite".
func (n CheckerName) AsSuiteName() string {
return n.toCamelCase() + "CheckerSuite"
}
Expand Down
2 changes: 1 addition & 1 deletion internal/testgen/gen.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ type TestsGenerator interface {
GoldenTemplate() Executor
}

// CheckerTestsGenerator a TestsGenerator for specific Checker.
// CheckerTestsGenerator is a TestsGenerator for specific Checker.
// One checker must be covered by one generator.
type CheckerTestsGenerator interface {
TestsGenerator
Expand Down

0 comments on commit 600ec56

Please sign in to comment.