Cosmetic fixes

- satisfy updated golangci-lint
- simplify code
- simplify func signatures
- enhance documentation a bit

Signed-off-by: apostasie <[email protected]>

Author: apostasie

mod/tigron/.golangci.yml

@@ -25,15 +25,13 @@ linters:
           files:
             - $all
           allow:
+
             - $gostd
             - github.com/containerd/nerdctl/mod/tigron
             - github.com/creack/pty
-            - github.com/rs/zerolog
-            - github.com/rs/zerolog/log
-            - go.uber.org/goleak
-            - golang.org/x/sync/errgroup
+            - golang.org/x/sync
             - golang.org/x/term
-            - gotest.tools
+            - gotest.tools/v3
     staticcheck:
       checks:
         - all

mod/tigron/Makefile

@@ -181,7 +181,7 @@ install-dev-tools:
 ##########################
 test-unit:
 	$(call title, $@)
-	@EXPERIMENTAL_HIGHK_FD=true go test $(VERBOSE_FLAG) $(MAKEFILE_DIR)/...
+	@go test $(VERBOSE_FLAG) $(MAKEFILE_DIR)/...
 	$(call footer, $@)
 
 test-unit-bench:
@@ -191,7 +191,7 @@ test-unit-bench:
 
 test-unit-race:
 	$(call title, $@)
-	@EXPERIMENTAL_HIGHK_FD=true go test $(VERBOSE_FLAG) $(MAKEFILE_DIR)/... -race
+	@CGO_ENABLED=1 go test $(VERBOSE_FLAG) $(MAKEFILE_DIR)/... -race
 	$(call footer, $@)
 
 .PHONY: \

mod/tigron/expect/comparators.go

@@ -30,7 +30,7 @@ import (
 // All can be used as a parameter for expected.Output to group a set of comparators.
 func All(comparators ...test.Comparator) test.Comparator {
 	//nolint:thelper
-	return func(stdout string, info string, t *testing.T) {
+	return func(stdout, info string, t *testing.T) {
 		t.Helper()
 
 		for _, comparator := range comparators {
@@ -43,17 +43,18 @@ func All(comparators ...test.Comparator) test.Comparator {
 // is found contained in the output.
 func Contains(compare string) test.Comparator {
 	//nolint:thelper
-	return func(stdout string, info string, t *testing.T) {
+	return func(stdout, info string, t *testing.T) {
 		t.Helper()
 		assert.Check(t, strings.Contains(stdout, compare),
 			fmt.Sprintf("Output does not contain: %q", compare)+info)
 	}
 }
 
-// DoesNotContain is to be used for expected.Output to ensure a comparison string is NOT found in the output.
+// DoesNotContain is to be used for expected.Output to ensure a comparison string is NOT found in
+// the output.
 func DoesNotContain(compare string) test.Comparator {
 	//nolint:thelper
-	return func(stdout string, info string, t *testing.T) {
+	return func(stdout, info string, t *testing.T) {
 		t.Helper()
 		assert.Check(t, !strings.Contains(stdout, compare),
 			fmt.Sprintf("Output does contain: %q", compare)+info)
@@ -63,7 +64,7 @@ func DoesNotContain(compare string) test.Comparator {
 // Equals is to be used for expected.Output to ensure it is exactly the output.
 func Equals(compare string) test.Comparator {
 	//nolint:thelper
-	return func(stdout string, info string, t *testing.T) {
+	return func(stdout, info string, t *testing.T) {
 		t.Helper()
 		assert.Equal(t, compare, stdout, info)
 	}
@@ -73,7 +74,7 @@ func Equals(compare string) test.Comparator {
 // Provisional - expected use, but have not seen it so far.
 func Match(reg *regexp.Regexp) test.Comparator {
 	//nolint:thelper
-	return func(stdout string, info string, t *testing.T) {
+	return func(stdout, info string, t *testing.T) {
 		t.Helper()
 		assert.Check(t, reg.MatchString(stdout), "Output does not match: "+reg.String(), info)
 	}

mod/tigron/expect/consts.go

@@ -17,8 +17,13 @@
 package expect
 
 const (
-	ExitCodeSuccess     = 0
+	// ExitCodeSuccess will ensure that the command effectively ran returned with exit code zero.
+	ExitCodeSuccess = 0
+	// ExitCodeGenericFail will verify that the command ran and exited with a non-zero error code.
+	// This does NOT include timeouts, cancellation, or signals.
 	ExitCodeGenericFail = -1
-	ExitCodeNoCheck     = -2
-	ExitCodeTimeout     = -3
+	// ExitCodeNoCheck does not enforce any check at all on the function.
+	ExitCodeNoCheck = -2
+	// ExitCodeTimeout verifies that the command was cancelled on timeout.
+	ExitCodeTimeout = -3
 )

mod/tigron/expect/doc.go

@@ -0,0 +1,19 @@
+/*
+   Copyright The containerd Authors.
+
+   Licensed under the Apache License, Version 2.0 (the "License");
+   you may not use this file except in compliance with the License.
+   You may obtain a copy of the License at
+
+       http://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+*/
+
+// Package expect provides a set of simple concrete test.Comparator implementations to use by tests
+// on stdout, along with exit code expectations.
+package expect

mod/tigron/require/doc.go

@@ -0,0 +1,19 @@
+/*
+   Copyright The containerd Authors.
+
+   Licensed under the Apache License, Version 2.0 (the "License");
+   you may not use this file except in compliance with the License.
+   You may obtain a copy of the License at
+
+       http://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+*/
+
+// Package require provides a set of concrete test.Requirements to express the need for a specific
+// architecture, OS, or binary, along with Not() and All() which allow Requirements composition.
+package require

mod/tigron/require/requirement.go

@@ -24,6 +24,7 @@ import (
 	"github.com/containerd/nerdctl/mod/tigron/test"
 )
 
+// Binary requires a certain binary to be present in the PATH.
 func Binary(name string) *test.Requirement {
 	return &test.Requirement{
 		Check: func(_ test.Data, _ test.Helpers) (bool, string) {
@@ -39,45 +40,40 @@ func Binary(name string) *test.Requirement {
 	}
 }
 
+// OS requires a specific operating system (matched against runtime.GOOS).
 func OS(os string) *test.Requirement {
 	return &test.Requirement{
 		Check: func(_ test.Data, _ test.Helpers) (bool, string) {
-			mess := fmt.Sprintf("current operating system is %q", runtime.GOOS)
-			ret := true
-			if runtime.GOOS != os {
-				ret = false
-			}
-
-			return ret, mess
+			return runtime.GOOS == os, fmt.Sprintf("current operating system is %q", runtime.GOOS)
 		},
 	}
 }
 
+// Arch requires a specific CPU architecture (matched against runtime.GOARCH).
 func Arch(arch string) *test.Requirement {
 	return &test.Requirement{
 		Check: func(_ test.Data, _ test.Helpers) (bool, string) {
-			mess := fmt.Sprintf("current architecture is %q", runtime.GOARCH)
-			ret := true
-			if runtime.GOARCH != arch {
-				ret = false
-			}
-
-			return ret, mess
+			return runtime.GOARCH == arch, fmt.Sprintf("current architecture is %q", runtime.GOARCH)
 		},
 	}
 }
 
 //nolint:gochecknoglobals
 var (
-	Amd64   = Arch("amd64")
-	Arm64   = Arch("arm64")
+	// Amd64 requires an intel x86_64 CPU.
+	Amd64 = Arch("amd64")
+	// Arm64 requires an ARM CPU.
+	Arm64 = Arch("arm64")
+	// Windows requires the OS to be Windows.
 	Windows = OS("windows")
-	Linux   = OS("linux")
-	Darwin  = OS("darwin")
+	// Linux requires the OS to be Linux.
+	Linux = OS("linux")
+	// Darwin requires the OS to be macOS.
+	Darwin = OS("darwin")
 )
 
-// NOTE: Not will always ignore any setup and cleanup inside the wrapped requirement.
-
+// Not will negate another requirement
+// Note that it will always *ignore* any setup and cleanup inside the wrapped requirement.
 func Not(requirement *test.Requirement) *test.Requirement {
 	return &test.Requirement{
 		Check: func(data test.Data, helpers test.Helpers) (bool, string) {
@@ -88,6 +84,7 @@ func Not(requirement *test.Requirement) *test.Requirement {
 	}
 }
 
+// All combines several other requirements together.
 func All(requirements ...*test.Requirement) *test.Requirement {
 	return &test.Requirement{
 		Check: func(data test.Data, helpers test.Helpers) (bool, string) {

mod/tigron/test/case.go

@@ -23,20 +23,23 @@ import (
 	"gotest.tools/v3/assert"
 )
 
-// Case describes an entire test-case, including data, setup and cleanup routines, command and expectations.
+// Case describes an entire test-case, including data, setup and cleanup routines, command and
+// expectations.
 type Case struct {
-	// Description contains a human-readable short desc, used as a seed for the identifier and as a title for the test
+	// Description contains a human-readable short desc, used as a seed for the identifier and as a
+	// title for the test.
 	Description string
-	// NoParallel disables parallel execution if set to true
+	// NoParallel disables parallel execution if set to true.
 	// This obviously implies that all tests run in parallel, by default. This is a design choice.
 	NoParallel bool
-	// Env contains a map of environment variables to use as a base for all commands run in Setup, Command and Cleanup
-	// Note that the environment is inherited by subtests
+	// Env contains a map of environment variables to use as a base for all commands run in Setup,
+	// Command and Cleanup.
+	// Note that the environment is inherited by subtests.
 	Env map[string]string
-	// Data contains test specific data, accessible to all operations, also inherited by subtests
+	// Data contains test specific data, accessible to all operations, also inherited by subtests.
 	Data Data
 	// Config contains specific information meaningful to the binary being tested.
-	// It is also inherited by subtests
+	// It is also inherited by subtests.
 	Config Config
 
 	// Requirement
@@ -73,7 +76,11 @@ func (test *Case) Run(t *testing.T) {
 
 		// Attach testing.T
 		test.t = subT
-		assert.Assert(test.t, test.Description != "" || test.parent == nil, "A test description cannot be empty")
+		assert.Assert(
+			test.t,
+			test.Description != "" || test.parent == nil,
+			"A test description cannot be empty",
+		)
 		assert.Assert(test.t, test.Command == nil || test.Expected != nil,
 			"Expectations for a test command cannot be nil. You may want to use Setup instead.")
 

mod/tigron/test/command.go

@@ -14,6 +14,7 @@
    limitations under the License.
 */
 
+//nolint:revive
 package test
 
 import (
@@ -34,7 +35,8 @@ import (
 	"github.com/containerd/nerdctl/mod/tigron/test/internal/pty"
 )
 
-// CustomizableCommand is an interface meant for people who want to heavily customize the base command
+// CustomizableCommand is an interface meant for people who want to heavily customize the base
+// command
 // of their test case.
 type CustomizableCommand interface {
 	TestableCommand
@@ -52,8 +54,9 @@ type CustomizableCommand interface {
 	// WithConfig allows passing custom config properties from the test to the base command
 	withConfig(config Config)
 	withT(t *testing.T)
-	// Clear does a clone, but will clear binary and arguments, but retain the env, or any other custom properties
-	// Gotcha: if GenericCommand is embedded with a custom Run and an overridden clear to return the embedding type
+	// Clear does a clone, but will clear binary and arguments, but retain the env, or any other
+	// custom properties Gotcha: if GenericCommand is embedded with a custom Run and an overridden
+	// clear to return the embedding type
 	// the result will be the embedding command, no longer the GenericCommand
 	clear() TestableCommand
 
@@ -127,12 +130,13 @@ func (gc *GenericCommand) Run(expect *Expected) {
 		env    []string
 		tty    *os.File
 		psty   *os.File
+		stdout string
 	)
 
 	output := &bytes.Buffer{}
-	stdout := ""
 	copyGroup := &errgroup.Group{}
 
+	//nolint:nestif
 	if !gc.async {
 		iCmdCmd := gc.boot()
 
@@ -158,6 +162,7 @@ func (gc *GenericCommand) Run(expect *Expected) {
 					gc.t.Log("start command failed")
 					gc.t.Log(gc.result.ExitCode)
 					gc.t.Log(gc.result.Error)
+
 					return gc.result.Error
 				}
 
@@ -166,6 +171,7 @@ func (gc *GenericCommand) Run(expect *Expected) {
 					if err != nil {
 						gc.t.Log("writing to the pty failed")
 						gc.t.Log(err)
+
 						return err
 					}
 				}
@@ -205,7 +211,8 @@ func (gc *GenericCommand) Run(expect *Expected) {
 		// ExitCode goes first
 		switch expect.ExitCode {
 		case internal.ExitCodeNoCheck:
-			// ExitCodeNoCheck means we do not care at all about exit code. It can be a failure, a success, or a timeout.
+			// ExitCodeNoCheck means we do not care at all about exit code. It can be a failure, a
+			// success, or a timeout.
 		case internal.ExitCodeGenericFail:
 			// ExitCodeGenericFail means we expect an error (excluding timeout).
 			assert.Assert(gc.t, result.ExitCode != 0,
@@ -367,7 +374,8 @@ func (gc *GenericCommand) boot() icmd.Cmd {
 		}
 	}
 
-	// Ensure the subprocess gets executed in a temporary directory unless explicitly instructed otherwise
+	// Ensure the subprocess gets executed in a temporary directory unless explicitly instructed
+	// otherwise
 	iCmdCmd.Dir = gc.workingDir
 
 	if gc.stdin != nil {

mod/tigron/test/config.go

@@ -29,7 +29,7 @@ func WithConfig(key ConfigKey, value ConfigValue) Config {
 // Contains the implementation of the Config interface
 
 //nolint:ireturn
-func configureConfig(cfg Config, parent Config) Config {
+func configureConfig(cfg, parent Config) Config {
 	if cfg == nil {
 		cfg = &config{
 			config: make(map[ConfigKey]ConfigValue),