Switch to table driven tests, fix bug in inline comments causing sort…

…ing to be off, fixes #21

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

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 

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:], " ")
 	}

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 (

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 {

testdata/import_with_lint_comment.txt

@@ -0,0 +1,6 @@
+package main
+
+import (
+	"crypto/md5" //nolint:gosec
+	"fmt"
+)

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"
 )

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"
 )