PR: docs, comments, naming

Signed-off-by: Hamza El-Saawy <[email protected]>

test/cri-containerd/logging_binary_test.go

@@ -12,9 +12,8 @@ import (
 	"testing"
 	"time"
 
-	runtime "k8s.io/cri-api/pkg/apis/runtime/v1alpha2"
-
 	"github.com/Microsoft/hcsshim/test/pkg/require"
+	runtime "k8s.io/cri-api/pkg/apis/runtime/v1alpha2"
 )
 
 // This test requires compiling a helper logging binary which can be found

test/cri-containerd/main_test.go

@@ -145,6 +145,10 @@ func requireFeatures(tb testing.TB, features ...string) {
 	require.Features(tb, flagFeatures, features...)
 }
 
+// requireAnyFeatures checks in flagFeatures if at least one of the required features
+// was enabled, and skips the test if all are missing.
+//
+// See: [requireFeatures]
 func requireAnyFeature(tb testing.TB, features ...string) {
 	tb.Helper()
 	require.AnyFeature(tb, flagFeatures, features...)

test/functional/uvm_mem_backingtype_test.go

@@ -6,24 +6,26 @@ package functional
 
 import (
 	"context"
+	"io"
 	"testing"
 
 	"github.com/Microsoft/hcsshim/internal/uvm"
 	"github.com/Microsoft/hcsshim/osversion"
 	"github.com/Microsoft/hcsshim/test/pkg/require"
+	"github.com/sirupsen/logrus"
 
-	testuvm "github.com/Microsoft/hcsshim/test/pkg/uvm"
+	tuvm "github.com/Microsoft/hcsshim/test/pkg/uvm"
 )
 
 func runMemStartLCOWTest(t *testing.T, opts *uvm.OptionsLCOW) {
 	t.Helper()
-	u := testuvm.CreateAndStartLCOWFromOpts(context.Background(), t, opts)
+	u := tuvm.CreateAndStartLCOWFromOpts(context.Background(), t, opts)
 	u.Close()
 }
 
 func runMemStartWCOWTest(t *testing.T, opts *uvm.OptionsWCOW) {
 	t.Helper()
-	u, _, _ := testuvm.CreateWCOWUVMFromOptsWithImage(context.Background(), t, opts, "microsoft/nanoserver")
+	u, _, _ := tuvm.CreateWCOWUVMFromOptsWithImage(context.Background(), t, opts, "microsoft/nanoserver")
 	u.Close()
 }
 
@@ -102,6 +104,7 @@ func BenchmarkMemBackingTypeVirtualLCOW(b *testing.B) {
 
 	require.Build(b, osversion.RS5)
 	requireFeatures(b, featureLCOW)
+	logrus.SetOutput(io.Discard)
 
 	runBenchMemStartLcowTest(b, true, false)
 }
@@ -111,6 +114,7 @@ func BenchmarkMemBackingTypeVirtualDeferredLCOW(b *testing.B) {
 
 	require.Build(b, osversion.RS5)
 	requireFeatures(b, featureLCOW)
+	logrus.SetOutput(io.Discard)
 
 	runBenchMemStartLcowTest(b, true, true)
 }
@@ -120,6 +124,7 @@ func BenchmarkMemBackingTypePhyscialLCOW(b *testing.B) {
 
 	require.Build(b, osversion.RS5)
 	requireFeatures(b, featureLCOW)
+	logrus.SetOutput(io.Discard)
 
 	runBenchMemStartLcowTest(b, false, false)
 }

test/pkg/flag/flag.go

@@ -14,21 +14,21 @@ const (
 	ExcludeFeatureFlagName = "exclude"
 )
 
-// NewFeatureFlag defines two flags, [FeatureFlagName] and [ExcludeFeatureFlagName], to all
-// setting and excluding certain features.
-func NewFeatureFlag(features []string) *ExcludeStringSet {
+// NewFeatureFlag defines two flags, [FeatureFlagName] and [ExcludeFeatureFlagName], to
+// allow setting and excluding certain features.
+func NewFeatureFlag(features []string) *IncludeExcludeStringSet {
 	fs := NewStringSet(FeatureFlagName,
 		"`features` to test; can be set multiple times, with a comma-separated list, or both. "+
 			"Leave empty to enable all features. "+
 			"(supported features: "+strings.Join(features, ", ")+")", false)
 
-	return NewExcludeStringSet(fs, ExcludeFeatureFlagName,
+	return NewIncludeExcludeStringSet(fs, ExcludeFeatureFlagName,
 		"`features` to exclude from tests (see "+FeatureFlagName+" for more details)",
 		features)
 }
 
-// ExcludeStringSet allows unsetting strings seen in a [StringSet].
-type ExcludeStringSet struct {
+// IncludeExcludeStringSet allows unsetting strings seen in a [StringSet].
+type IncludeExcludeStringSet struct {
 	// flags explicitly included
 	inc *StringSet
 	// flags explicitly excluded
@@ -38,9 +38,9 @@ type ExcludeStringSet struct {
 	def []string
 }
 
-// NewStringSet returns a new ExcludeStringSet.
-func NewExcludeStringSet(include *StringSet, name, usage string, all []string) *ExcludeStringSet {
-	es := &ExcludeStringSet{
+// NewIncludeExcludeStringSet returns a new NewIncludeExcludeStringSet.
+func NewIncludeExcludeStringSet(include *StringSet, name, usage string, all []string) *IncludeExcludeStringSet {
+	es := &IncludeExcludeStringSet{
 		inc: include,
 		exc: &StringSet{
 			s:  make(map[string]struct{}),
@@ -52,11 +52,11 @@ func NewExcludeStringSet(include *StringSet, name, usage string, all []string) *
 	return es
 }
 
-var _ flag.Value = &ExcludeStringSet{}
+var _ flag.Value = &IncludeExcludeStringSet{}
 
-func (es *ExcludeStringSet) Set(s string) error { return es.exc.Set(s) }
+func (es *IncludeExcludeStringSet) Set(s string) error { return es.exc.Set(s) }
 
-func (es *ExcludeStringSet) String() string {
+func (es *IncludeExcludeStringSet) String() string {
 	if es == nil { // may be called by flag package on nil receiver
 		return ""
 	}
@@ -67,10 +67,10 @@ func (es *ExcludeStringSet) String() string {
 	return "[" + strings.Join(ss, ", ") + "]"
 }
 
-func (es *ExcludeStringSet) Strings() []string { return es.strings() }
-func (es *ExcludeStringSet) Len() int          { return len(es.strings()) }
+func (es *IncludeExcludeStringSet) Strings() []string { return es.strings() }
+func (es *IncludeExcludeStringSet) Len() int          { return len(es.strings()) }
 
-func (es *ExcludeStringSet) strings() []string {
+func (es *IncludeExcludeStringSet) strings() []string {
 	ss := es.def
 	set := make([]string, 0, len(ss))
 	if es.inc != nil && es.inc.Len() > 0 {
@@ -85,7 +85,7 @@ func (es *ExcludeStringSet) strings() []string {
 	return set
 }
 
-func (es *ExcludeStringSet) IsSet(s string) bool {
+func (es *IncludeExcludeStringSet) IsSet(s string) bool {
 	if es.inc == nil || es.inc.Len() == 0 || es.inc.IsSet(s) {
 		// either no values were included, or value was explicitly provided
 		return !es.exc.IsSet(s)
@@ -147,7 +147,7 @@ func (ss *StringSet) standardize(s string) string {
 	return s
 }
 
-// an actual (mathematical?) set
+// stringSet is a set of strings.
 type stringSet map[string]struct{}
 
 func (ss stringSet) set(s string) { ss[s] = struct{}{} }
@@ -156,7 +156,7 @@ func (ss stringSet) isSet(s string) bool {
 	return ok
 }
 
-// LogrusLevel is a flag that accepts logrus logging levels, as strings.
+// LogrusLevel is a flag that accepts logrus logging levels as strings.
 type LogrusLevel struct {
 	Level logrus.Level
 }

test/pkg/flag/flag_test.go

@@ -9,11 +9,12 @@ import (
 
 // tests for testing fixtures ...
 
-// calling New(Exclude)?StringSet will add it to the default flag set, which may cause problems since [testing] already defines flags
+// calling New(IncludeExclude)?StringSet will add it to the default flag set,
+// which may cause problems since [testing] already defines flags
 
 func Test_ExcludeStringSetFlag(t *testing.T) {
 	all := []string{"one", "two", "three", "four", "zero"}
-	es := &ExcludeStringSet{
+	es := &IncludeExcludeStringSet{
 		inc: &StringSet{
 			s:  make(map[string]struct{}),
 			cs: false,
@@ -27,7 +28,7 @@ func Test_ExcludeStringSetFlag(t *testing.T) {
 
 	orderlessEq(t, all, es.Strings())
 	for _, s := range all {
-		assert(t, es.IsSet(s), s+" is not set")
+		assert(t, es.IsSet(s), s+" is expected to be set, but is not")
 	}
 
 	for i, tc := range []struct {
@@ -64,9 +65,9 @@ func Test_ExcludeStringSetFlag(t *testing.T) {
 
 			for _, s := range all {
 				if slices.Contains(tc.exp, s) {
-					assert(t, es.IsSet(s), s+" is not set")
+					assert(t, es.IsSet(s), s+" is expected to be set, but is not")
 				} else {
-					assert(t, !es.IsSet(s), s+" is set")
+					assert(t, !es.IsSet(s), s+" is not expected to be set, but is")
 				}
 			}
 		})
@@ -86,13 +87,13 @@ func Test_StringSetFlag(t *testing.T) {
 
 	orderlessEq(t, exp, ss.Strings())
 	for _, s := range exp {
-		assert(t, ss.IsSet(s), s+"is not set")
+		assert(t, ss.IsSet(s), s+"is expected to be set, but is not")
 	}
 	for _, s := range []string{"HI", "bYe", "BYE", "  not A wOrD"} {
-		assert(t, ss.IsSet(s), s+"is not set")
+		assert(t, ss.IsSet(s), s+"is expected to be set, but is not")
 	}
 	for _, s := range []string{"hello", "goodbye", "also not a word"} {
-		assert(t, !ss.IsSet(s), s+"is set")
+		assert(t, !ss.IsSet(s), s+"is not expected to be set, but is")
 	}
 }
 

test/pkg/require/requires.go

@@ -9,9 +9,12 @@ import (
 )
 
 // Features checks the wanted features are present in given,
-// and skips the test if any are missing. If the given set is empty,
-// the function returns (by default all features are enabled).
-func Features(tb testing.TB, given *flag.ExcludeStringSet, want ...string) {
+// and skips the test if any are missing or explicitly excluded.
+// If the given set is empty, the function returns
+// (by default, all features are enabled).
+//
+// See [flag.NewFeatureFlag] and [flag.IncludeExcludeStringSet] for more details.
+func Features(tb testing.TB, given *flag.IncludeExcludeStringSet, want ...string) {
 	tb.Helper()
 	if given.Len() == 0 {
 		return
@@ -26,7 +29,7 @@ func Features(tb testing.TB, given *flag.ExcludeStringSet, want ...string) {
 // AnyFeature checks if at least one of the features are enabled.
 //
 // See [Features] for more information.
-func AnyFeature(tb testing.TB, given *flag.ExcludeStringSet, want ...string) {
+func AnyFeature(tb testing.TB, given *flag.IncludeExcludeStringSet, want ...string) {
 	tb.Helper()
 	if given.Len() == 0 {
 		return