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

lint: Add testifylint to prevent unnecessary test code issues #1813

Merged
merged 2 commits into from
Nov 25, 2024
+2,698 −2,516
Merged

lint: Add testifylint to prevent unnecessary test code issues #1813

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f305538
lint: Add testifylint to prevent unnecessary test code issues
Matovidlo Jun 4, 2024
f766f90
fix: Add nolint so the assertions do not make test fail
Matovidlo Nov 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
The table of contents is too big for display.
Diff view
Diff view
  •  
  •  
  •  
5 changes: 5 additions & 0 deletions build/ci/golangci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,10 @@ linters-settings:
exhaustive:
check-generated: true
default-signifies-exhaustive: true
testifylint:
disable:
- error-is-as
- go-require

# https://golangci-lint.run/usage/linters
linters:
Expand Down Expand Up @@ -149,6 +153,7 @@ linters:
- staticcheck
- stylecheck
- tagliatelle
- testifylint
- thelper
- tparallel
- paralleltest
Expand Down
47 changes: 24 additions & 23 deletions internal/pkg/diff/diff_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"github.com/keboola/go-client/pkg/keboola"
"github.com/keboola/go-utils/pkg/orderedmap"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

"github.com/keboola/keboola-as-code/internal/pkg/fixtures"
"github.com/keboola/keboola-as-code/internal/pkg/model"
Expand All @@ -23,11 +24,11 @@ func TestDiffOnlyInLocal(t *testing.T) {
BranchManifest: &model.BranchManifest{BranchKey: branchKey},
Local: &model.Branch{BranchKey: branchKey},
}
assert.NoError(t, projectState.Set(branchState))
require.NoError(t, projectState.Set(branchState))

d := NewDiffer(projectState)
results, err := d.Diff()
assert.NoError(t, err)
require.NoError(t, err)
assert.Len(t, results.Results, 1)
result := results.Results[0]
assert.Equal(t, ResultOnlyInLocal, result.State)
Comment on lines -26 to 34
Copy link
Contributor

Choose a reason for hiding this comment

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

Sometimes it makes sense for the test to continue, so you get the overall picture and you don't have to run the test repeatedly.

I prefer to use assert and require only when there is no point in continuing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should you continue when err is not nil? If yes then use if assert.NoError(t, err); err != nil { } Otherwise it makes sense. I don't see anything else just a plain help to prevent unnecessary panics in test code. When you do not care about error, just omit it using _

Expand All @@ -43,11 +44,11 @@ func TestDiffOnlyInRemote(t *testing.T) {
BranchManifest: &model.BranchManifest{BranchKey: branchKey},
Remote: &model.Branch{BranchKey: branchKey},
}
assert.NoError(t, projectState.Set(branchState))
require.NoError(t, projectState.Set(branchState))

d := NewDiffer(projectState)
results, err := d.Diff()
assert.NoError(t, err)
require.NoError(t, err)
assert.Len(t, results.Results, 1)
result := results.Results[0]
assert.Equal(t, ResultOnlyInRemote, result.State)
Expand All @@ -74,11 +75,11 @@ func TestDiffEqual(t *testing.T) {
IsDefault: false,
},
}
assert.NoError(t, projectState.Set(branchState))
require.NoError(t, projectState.Set(branchState))

d := NewDiffer(projectState)
results, err := d.Diff()
assert.NoError(t, err)
require.NoError(t, err)
assert.Len(t, results.Results, 1)
result := results.Results[0]
assert.Equal(t, ResultEqual, result.State)
Expand Down Expand Up @@ -106,11 +107,11 @@ func TestDiffNotEqual(t *testing.T) {
IsDefault: true,
},
}
assert.NoError(t, projectState.Set(branchState))
require.NoError(t, projectState.Set(branchState))

d := NewDiffer(projectState)
results, err := d.Diff()
assert.NoError(t, err)
require.NoError(t, err)
assert.Len(t, results.Results, 1)
result := results.Results[0]
assert.Equal(t, ResultNotEqual, result.State)
Expand Down Expand Up @@ -141,7 +142,7 @@ func TestDiffEqualConfig(t *testing.T) {
IsDefault: false,
},
}
assert.NoError(t, projectState.Set(branchState))
require.NoError(t, projectState.Set(branchState))

configKey := model.ConfigKey{
BranchID: 123,
Expand All @@ -161,11 +162,11 @@ func TestDiffEqualConfig(t *testing.T) {
Description: "description",
},
}
assert.NoError(t, projectState.Set(configState))
require.NoError(t, projectState.Set(configState))

d := NewDiffer(projectState)
results, err := d.Diff()
assert.NoError(t, err)
require.NoError(t, err)
assert.Len(t, results.Results, 2)
result1 := results.Results[0]
assert.Equal(t, ResultEqual, result1.State)
Expand Down Expand Up @@ -199,7 +200,7 @@ func TestDiffNotEqualConfig(t *testing.T) {
IsDefault: false,
},
}
assert.NoError(t, projectState.Set(branchState))
require.NoError(t, projectState.Set(branchState))

configKey := model.ConfigKey{
BranchID: 123,
Expand All @@ -219,11 +220,11 @@ func TestDiffNotEqualConfig(t *testing.T) {
Description: "changed",
},
}
assert.NoError(t, projectState.Set(configState))
require.NoError(t, projectState.Set(configState))

d := NewDiffer(projectState)
results, err := d.Diff()
assert.NoError(t, err)
require.NoError(t, err)
assert.Len(t, results.Results, 2)

result1 := results.Results[0]
Expand Down Expand Up @@ -259,7 +260,7 @@ func TestDiffNotEqualConfigConfiguration(t *testing.T) {
IsDefault: false,
},
}
assert.NoError(t, projectState.Set(branchState))
require.NoError(t, projectState.Set(branchState))

configKey := model.ConfigKey{
BranchID: 123,
Expand Down Expand Up @@ -295,11 +296,11 @@ func TestDiffNotEqualConfigConfiguration(t *testing.T) {
}),
},
}
assert.NoError(t, projectState.Set(configState))
require.NoError(t, projectState.Set(configState))

d := NewDiffer(projectState)
results, err := d.Diff()
assert.NoError(t, err)
require.NoError(t, err)
assert.Len(t, results.Results, 2)

result1 := results.Results[0]
Expand Down Expand Up @@ -330,7 +331,7 @@ func TestDiffRelations(t *testing.T) {
PathValue: `path/to/target`,
},
}
assert.NoError(t, projectState.Set(targetState))
require.NoError(t, projectState.Set(targetState))

objectKey := fixtures.MockedKey{
ID: `345`,
Expand Down Expand Up @@ -369,7 +370,7 @@ func TestDiffRelations(t *testing.T) {
},
},
}
assert.NoError(t, projectState.Set(objectState))
require.NoError(t, projectState.Set(objectState))

differ := NewDiffer(projectState)
reporter := differ.diffValues(objectState, objectState.Remote.Relations, objectState.Local.Relations, differ.newOptions)
Expand Down Expand Up @@ -451,7 +452,7 @@ func TestDiffTransformation(t *testing.T) {
},
},
}
assert.NoError(t, projectState.Set(configState))
require.NoError(t, projectState.Set(configState))

// Transformation
differ := NewDiffer(projectState)
Expand Down Expand Up @@ -514,7 +515,7 @@ func TestDiffSharedCode(t *testing.T) {
},
},
}
assert.NoError(t, projectState.Set(configRowState))
require.NoError(t, projectState.Set(configRowState))

// Transformation
differ := NewDiffer(projectState)
Expand Down Expand Up @@ -679,7 +680,7 @@ func TestDiffOrchestration(t *testing.T) {
},
},
}
assert.NoError(t, projectState.Set(configState))
require.NoError(t, projectState.Set(configState))

differ := NewDiffer(projectState)
reporter := differ.diffValues(configState, configState.Remote.Orchestration, configState.Local.Orchestration, differ.newOptions)
Expand Down Expand Up @@ -763,7 +764,7 @@ func TestDiffMap(t *testing.T) {
}),
},
}
assert.NoError(t, projectState.Set(configState))
require.NoError(t, projectState.Set(configState))

differ := NewDiffer(projectState)
reporter := differ.diffValues(configState, configState.Remote.Content, configState.Local.Content, differ.newOptions)
Expand Down
3 changes: 1 addition & 2 deletions internal/pkg/diff/reporter_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package diff

import (
"fmt"
"reflect"
"strings"
"testing"
Expand Down Expand Up @@ -76,7 +75,7 @@ func TestReporterStringsDiff(t *testing.T) {
}
for i, c := range cases {
result := stringsDiff(c.remote, c.local)
assert.Equal(t, c.result, result, fmt.Sprintf(`case "%d"`, i))
assert.Equal(t, c.result, result, `case "%d"`, i)
}
}

Expand Down
5 changes: 3 additions & 2 deletions internal/pkg/encoding/json/schema/generate_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

"github.com/keboola/keboola-as-code/internal/pkg/encoding/json"
)
Expand All @@ -13,7 +14,7 @@ func TestGenerateDocument(t *testing.T) {
t.Parallel()
document, err := GenerateDocument(getSampleSchema())
documentJSON := json.MustEncodeString(document, true)
assert.NoError(t, err)
require.NoError(t, err)

expected := `
{
Expand Down Expand Up @@ -50,7 +51,7 @@ func TestGenerateDocumentEmptySchema(t *testing.T) {
t.Parallel()
document, err := GenerateDocument([]byte(`{}`))
documentJSON := json.MustEncodeString(document, true)
assert.NoError(t, err)
require.NoError(t, err)
assert.Equal(t, "{}\n", documentJSON)
}

Expand Down
17 changes: 9 additions & 8 deletions internal/pkg/encoding/json/schema/meta_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (

"github.com/keboola/go-utils/pkg/orderedmap"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

"github.com/keboola/keboola-as-code/internal/pkg/encoding/json/schema"
)
Expand All @@ -14,7 +15,7 @@ func TestFieldMeta_Empty(t *testing.T) {
meta, found, err := schema.FieldMeta([]byte(""), orderedmap.Path{})
assert.Empty(t, meta)
assert.False(t, found)
assert.Nil(t, err)
require.NoError(t, err)
}

func TestFieldMeta_Complex(t *testing.T) {
Expand Down Expand Up @@ -54,39 +55,39 @@ func TestFieldMeta_Complex(t *testing.T) {
meta, found, err := schema.FieldMeta([]byte(componentSchema), orderedmap.Path{})
assert.Empty(t, meta)
assert.False(t, found)
assert.Nil(t, err)
require.NoError(t, err)

// Not found
meta, found, err = schema.FieldMeta([]byte(componentSchema), orderedmap.PathFromStr("foo.bar"))
assert.Empty(t, meta)
assert.False(t, found)
assert.Nil(t, err)
require.NoError(t, err)

// Found object
meta, found, err = schema.FieldMeta([]byte(componentSchema), orderedmap.PathFromStr("parameters.db"))
assert.NotEmpty(t, meta)
assert.True(t, found)
assert.Nil(t, err)
require.NoError(t, err)
assert.Equal(t, "Database", meta.Title)
assert.Equal(t, "", meta.Description)
assert.Equal(t, nil, meta.Default)
assert.Nil(t, meta.Default)
assert.False(t, meta.Required)

// Found string, required field
meta, found, err = schema.FieldMeta([]byte(componentSchema), orderedmap.PathFromStr("parameters.db.#connectionString"))
assert.NotEmpty(t, meta)
assert.True(t, found)
assert.Nil(t, err)
require.NoError(t, err)
assert.Equal(t, "Connection String", meta.Title)
assert.Equal(t, `Eg. "DefaultEndpointsProtocol=https;...". The value will be encrypted when saved.`, meta.Description)
assert.Equal(t, nil, meta.Default)
assert.Nil(t, meta.Default)
assert.True(t, meta.Required)

// Found int, default field
meta, found, err = schema.FieldMeta([]byte(componentSchema), orderedmap.PathFromStr("parameters.db.limit"))
assert.NotEmpty(t, meta)
assert.True(t, found)
assert.Nil(t, err)
require.NoError(t, err)
assert.Equal(t, "Query Limit", meta.Title)
assert.Equal(t, "", meta.Description)
assert.Equal(t, "1234", meta.Default)
Expand Down
19 changes: 10 additions & 9 deletions internal/pkg/encoding/json/schema/schema_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"github.com/keboola/go-client/pkg/keboola"
"github.com/keboola/go-utils/pkg/orderedmap"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

. "github.com/keboola/keboola-as-code/internal/pkg/encoding/json/schema"
"github.com/keboola/keboola-as-code/internal/pkg/filesystem/knownpaths"
Expand All @@ -27,7 +28,7 @@ func TestValidateObjects_Ok(t *testing.T) {
})
content := orderedmap.New()
content.Set(`parameters`, parameters)
assert.NoError(t, ValidateContent(schema, content))
require.NoError(t, ValidateContent(schema, content))
}

func TestValidateObjects_Error(t *testing.T) {
Expand All @@ -46,7 +47,7 @@ func TestValidateObjects_Error(t *testing.T) {
content := orderedmap.New()
content.Set(`parameters`, parameters)
err := ValidateContent(schema, content)
assert.Error(t, err)
require.Error(t, err)
expectedErr := `
- missing properties: "firstName"
- "address": missing properties: "street"
Expand All @@ -68,7 +69,7 @@ func TestValidateObjects_InvalidSchema_InvalidJSON(t *testing.T) {
}),
},
}))
assert.Error(t, err)
require.Error(t, err)
expected := `
invalid JSON schema:
- invalid character '.' looking for beginning of object key string, offset: 2
Expand Down Expand Up @@ -209,7 +210,7 @@ func TestValidateObjects_InvalidSchema_InvalidType(t *testing.T) {
Value: orderedmap.FromPairs([]orderedmap.Pair{{Key: "key", Value: "value"}}),
},
}))
assert.Error(t, err)
require.Error(t, err)
expected := `
invalid JSON schema:
- allOf failed:
Expand All @@ -227,7 +228,7 @@ func TestValidateObjects_BooleanRequired(t *testing.T) {
// But, for historical reasons, in Keboola components, "required: true" is also used.
// In the UI, this causes the drop-down list to not have an empty value.
// For this reason,the error should be ignored.
assert.NoError(t, ValidateContent(invalidSchema, orderedmap.FromPairs([]orderedmap.Pair{
require.NoError(t, ValidateContent(invalidSchema, orderedmap.FromPairs([]orderedmap.Pair{
{
Key: "parameters",
Value: orderedmap.FromPairs([]orderedmap.Pair{
Expand All @@ -242,7 +243,7 @@ func TestValidateObjects_SkipEmpty(t *testing.T) {
t.Parallel()
schema := getTestSchema()
content := orderedmap.New()
assert.NoError(t, ValidateContent(schema, content))
require.NoError(t, ValidateContent(schema, content))
}

func TestValidateObjects_InvalidSchema_Warning1(t *testing.T) {
Expand Down Expand Up @@ -321,19 +322,19 @@ func testInvalidComponentSchema(t *testing.T, invalidSchema []byte, expectedLogs
},
})
registry := state.NewRegistry(knownpaths.NewNop(context.Background()), naming.NewRegistry(), components, model.SortByID)
assert.NoError(t, registry.Set(&model.ConfigState{
require.NoError(t, registry.Set(&model.ConfigState{
ConfigManifest: &model.ConfigManifest{ConfigKey: model.ConfigKey{ComponentID: componentID}},
Local: &model.Config{Content: someContent},
}))
assert.NoError(t, registry.Set(&model.ConfigRowState{
require.NoError(t, registry.Set(&model.ConfigRowState{
ConfigRowManifest: &model.ConfigRowManifest{ConfigRowKey: model.ConfigRowKey{ComponentID: componentID}},
Local: &model.ConfigRow{Content: someContent},
}))

// Validate, no error
content := orderedmap.New()
content.Set(`parameters`, orderedmap.New())
assert.NoError(t, ValidateObjects(context.Background(), logger, registry))
require.NoError(t, ValidateObjects(context.Background(), logger, registry))
assert.Equal(t, strings.TrimLeft(expectedLogs, "\n"), logger.AllMessagesTxt())
}

Expand Down
Loading