From 2c5afc367fa13603ee2dd62a72be397983f7f73d Mon Sep 17 00:00:00 2001 From: Cameron McAvoy Date: Wed, 14 Oct 2020 15:44:07 -0500 Subject: [PATCH] Switch to table driven tests, fix bug in inline comments causing sorting to be off, fixes #21 --- CHANGELOG.md | 4 + README.md | 2 +- import.go | 5 + main.go | 2 +- main_test.go | 198 ++++++++++------------ testdata/import_with_lint_comment.txt | 6 + testdata/sort_external_groups.txt | 14 +- testdata/sort_external_groups_invalid.txt | 14 +- 8 files changed, 121 insertions(+), 124 deletions(-) create mode 100644 testdata/import_with_lint_comment.txt diff --git a/CHANGELOG.md b/CHANGELOG.md index d12ef4e..2c9c2c6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [1.1.3] - 2020-10-14 +### Fixed +- go-groups correctly sorts imports with inline comments + ## [1.1.2] - 2020-08-06 ### Fixed - go-groups correctly detects generated code and ignores generated code by default diff --git a/README.md b/README.md index 1400981..f4a6d4b 100644 --- a/README.md +++ b/README.md @@ -9,7 +9,7 @@ go-groups # Project Overview -Command `go-groups` is a CLI tool to parse go import blocks, sort, and re-group +Command `go-groups` is a CLI tool to deterministically rewrite go import blocks to sort and re-group the imports. `go-groups` is similar to `goimports`, but in addition to sorting imports diff --git a/import.go b/import.go index ad2ad2f..e2d8ce2 100644 --- a/import.go +++ b/import.go @@ -1,10 +1,13 @@ package main import ( + "regexp" "sort" "strings" ) +var commentRegex = regexp.MustCompile(`(//.*)|(/\*.*\*\/)`) + // Imports represents the list of imports in a given go file. // This helper encapsulates the logic for sorting imports based on go-groups. type Imports []importLine @@ -21,10 +24,12 @@ func (s Imports) Swap(i, j int) { func (s Imports) Less(i, j int) bool { s1 := strings.TrimSpace(s[i].line) + s1 = commentRegex.ReplaceAllString(s1, "") if strings.ContainsAny(s1, " ") { s1 = strings.Join(strings.Split(s1, " ")[1:], " ") } s2 := strings.TrimSpace(s[j].line) + s2 = commentRegex.ReplaceAllString(s2, "") if strings.ContainsAny(s2, " ") { s2 = strings.Join(strings.Split(s2, " ")[1:], " ") } diff --git a/main.go b/main.go index ff14ad0..1e3d050 100644 --- a/main.go +++ b/main.go @@ -16,7 +16,7 @@ const ( exitBadStdin = 2 exitInternalError = 3 - versionStr = "go-groups version 1.1.2 (2020-08-06)" + versionStr = "go-groups version 1.1.3 (2020-10-14)" ) var ( diff --git a/main_test.go b/main_test.go index fc96c14..db03741 100644 --- a/main_test.go +++ b/main_test.go @@ -14,115 +14,97 @@ func TestParse_NoImports(t *testing.T) { require.False(t, rewritten) } -func TestParse_ValidGroupingSortImports(t *testing.T) { - bytes := testdata(t, "valid_imports.txt") - result, rewritten := parse(bytes) - - require.True(t, rewritten) - require.Equal(t, string(bytes), string(result)) -} - -func TestParse_ExtraGroups(t *testing.T) { - bytes := testdata(t, "extra_groups.txt") - result, rewritten := parse(bytes) - - require.True(t, rewritten) - require.Equal(t, testdata(t, "valid_imports.txt"), result) -} - -func TestParse_ExternalGroups(t *testing.T) { - bytes := testdata(t, "external_groups_invalid.txt") - expected := testdata(t, "external_groups.txt") - result, rewritten := parse(bytes) - - require.True(t, rewritten) - require.Equal(t, string(expected), string(result)) -} - -func TestParse_SortingExternalGroups(t *testing.T) { - bytes := testdata(t, "sort_external_groups_invalid.txt") - expected := testdata(t, "sort_external_groups.txt") - result, rewritten := parse(bytes) - - require.True(t, rewritten) - require.Equal(t, expected, result) -} - -func TestParse_PrefixedGroups(t *testing.T) { - bytes := testdata(t, "prefixed_groups_invalid.txt") - expected := testdata(t, "prefixed_groups.txt") - result, rewritten := parse(bytes) - - require.True(t, rewritten) - require.Equal(t, expected, result) -} - -func TestParse_SubdomainImports(t *testing.T) { - bytes := testdata(t, "subdomain_imports_invalid.txt") - expected := testdata(t, "subdomain_imports.txt") - result, rewritten := parse(bytes) - - require.True(t, rewritten) - require.Equal(t, string(expected), string(result)) -} - -func TestParse_CommentedImports(t *testing.T) { - bytes := testdata(t, "commented_imports.txt") - result, rewritten := parse(bytes) - require.True(t, rewritten) - require.Equal(t, string(bytes), string(result)) -} - -func TestParse_Multiline_CommentedImports(t *testing.T) { - bytes := testdata(t, "multiline_comments_invalid.txt") - expected := testdata(t, "multiline_comments.txt") - result, rewritten := parse(bytes) - require.True(t, rewritten) - require.Equal(t, string(expected), string(result)) -} - -func TestParse_gofmt(t *testing.T) { - b := testdata(t, "gofmt_invalid.txt") - expected := testdata(t, "gofmt.txt") - +func TestParse(t *testing.T) { + type testcase struct { + Description string + ActualFixture string + ExpectedFixture string + NoGoFmt bool + GenCode bool + } + testcases := []testcase{ + { + Description: "go-groups should not modify sorted import blocks", + ActualFixture: "valid_imports.txt", + ExpectedFixture: "valid_imports.txt", + }, + { + Description: "go-groups should merge multiple import groups", + ActualFixture: "extra_groups.txt", + ExpectedFixture: "valid_imports.txt", + }, + { + Description: "go-groups should group external imports", + ActualFixture: "external_groups_invalid.txt", + ExpectedFixture: "external_groups.txt", + }, + { + Description: "go-groups should sort external imports", + ActualFixture: "sort_external_groups_invalid.txt", + ExpectedFixture: "sort_external_groups.txt", + }, + { + Description: "go-groups should handle import prefixes", + ActualFixture: "prefixed_groups_invalid.txt", + ExpectedFixture: "prefixed_groups.txt", + }, + { + Description: "go-groups should group subdomain external imports", + ActualFixture: "subdomain_imports_invalid.txt", + ExpectedFixture: "subdomain_imports.txt", + }, + { + Description: "go-groups should preserve comments in imports", + ActualFixture: "commented_imports.txt", + ExpectedFixture: "commented_imports.txt", + }, + { + Description: "go-groups should group multi-line comments in imports", + ActualFixture: "multiline_comments_invalid.txt", + ExpectedFixture: "multiline_comments.txt", + NoGoFmt: true, + }, + { + Description: "go-groups should run gofmt over source code", + ActualFixture: "gofmt_invalid.txt", + ExpectedFixture: "gofmt.txt", + }, + { + Description: "go-groups should not run gofmt over source code", + ActualFixture: "gofmt_invalid.txt", + ExpectedFixture: "gofmt_invalid.txt", + NoGoFmt: true, + }, + { + Description: "go-groups should not run over generated code", + ActualFixture: "code_generated.txt", + ExpectedFixture: "code_generated.txt", + }, + { + Description: "go-groups should run over generated code", + ActualFixture: "code_generated.txt", + ExpectedFixture: "code_generated_valid.txt", + GenCode: true, + }, + { + Description: "go-groups order imports with lint comments", + ActualFixture: "import_with_lint_comment.txt", + ExpectedFixture: "import_with_lint_comment.txt", + }, + } var buf bytes.Buffer - err := processFile("", strings.NewReader(string(b)), &buf, true, false) - require.NoError(t, err) - - require.Equal(t, string(expected), buf.String()) -} - -func TestParse_gofmt_disabled(t *testing.T) { - b := testdata(t, "gofmt_invalid.txt") - expected := testdata(t, "gofmt_invalid.txt") - - var buf bytes.Buffer - err := processFile("", strings.NewReader(string(b)), &buf, false, false) - require.NoError(t, err) - - require.Equal(t, string(expected), buf.String()) -} - -func TestParse_generated_code(t *testing.T) { - b := testdata(t, "code_generated.txt") - expected := testdata(t, "code_generated.txt") - - var buf bytes.Buffer - err := processFile("", strings.NewReader(string(b)), &buf, true, false) - require.NoError(t, err) - - require.Equal(t, string(expected), buf.String()) -} - -func TestParse_generated_code_enabled(t *testing.T) { - b := testdata(t, "code_generated.txt") - expected := testdata(t, "code_generated_valid.txt") - - var buf bytes.Buffer - err := processFile("", strings.NewReader(string(b)), &buf, true, true) - require.NoError(t, err) - - require.Equal(t, string(expected), buf.String()) + for _, testcase := range testcases { + bytes := testdata(t, testcase.ActualFixture) + expected := testdata(t, testcase.ExpectedFixture) + err := processFile("", strings.NewReader(string(bytes)), &buf, !testcase.NoGoFmt, testcase.GenCode) + + if buf.String() != string(expected) { + t.Logf("Input: \n%s\n\nOutput: \n%s\n\nExpected: \n%s\n\n", string(bytes), buf.String(), string(expected)) + } + require.NoError(t, err) + require.Equal(t, string(expected), buf.String(), testcase.Description) + buf.Reset() + } } func testdata(t *testing.T, str string) []byte { diff --git a/testdata/import_with_lint_comment.txt b/testdata/import_with_lint_comment.txt new file mode 100644 index 0000000..8fe3b53 --- /dev/null +++ b/testdata/import_with_lint_comment.txt @@ -0,0 +1,6 @@ +package main + +import ( + "crypto/md5" //nolint:gosec + "fmt" +) diff --git a/testdata/sort_external_groups.txt b/testdata/sort_external_groups.txt index c37ca6c..8e12c8b 100644 --- a/testdata/sort_external_groups.txt +++ b/testdata/sort_external_groups.txt @@ -1,14 +1,14 @@ package foo import ( - "net/http" - "testing" + "net/http" + "testing" - "github.com/gin-gonic/gin" + "github.com/gin-gonic/gin" - "github.com/stretchr/testify/suite" + "github.com/stretchr/testify/suite" - "indeed.com/devops/libmarvin/gin/ginhandler" - "indeed.com/devops/libmarvin/spec" - "indeed.com/devops/marvlet/api/v2" + "indeed.com/devops/libmarvin/gin/ginhandler" + "indeed.com/devops/libmarvin/spec" + "indeed.com/devops/marvlet/api/v2" ) diff --git a/testdata/sort_external_groups_invalid.txt b/testdata/sort_external_groups_invalid.txt index 777def7..0471cda 100644 --- a/testdata/sort_external_groups_invalid.txt +++ b/testdata/sort_external_groups_invalid.txt @@ -1,13 +1,13 @@ package foo import ( - "net/http" - "testing" + "net/http" + "testing" - "indeed.com/devops/libmarvin/gin/ginhandler" - "indeed.com/devops/libmarvin/spec" - "indeed.com/devops/marvlet/api/v2" + "indeed.com/devops/libmarvin/gin/ginhandler" + "indeed.com/devops/libmarvin/spec" + "indeed.com/devops/marvlet/api/v2" - "github.com/gin-gonic/gin" - "github.com/stretchr/testify/suite" + "github.com/gin-gonic/gin" + "github.com/stretchr/testify/suite" )