fix(tests): make all the tests pass

When cloning the repo, users expect to (or should expect to) be able to
run `go test ./...` and run all the tests. If running all the tests on a
fresh clone of main doesn't work, then it's hard to contribute. This
ensures that all of the tests run with `go test ./... -short`. The short
flag is used because there are some tests that are "costly" in terms of
time or environment. This is a well-known flag in Go for bypassing these
tests to provide a good onramp for new contributors.

Also, where one test was failing documentation has been improved and
failing tests were fixed.

pkg/controllers/daemon/retinaendpoint/suite_test.go

@@ -37,6 +37,10 @@ var (
 func TestAPIs(t *testing.T) {
 	RegisterFailHandler(Fail)
 
+	if testing.Short() {
+		t.Skip("skipping more involved tests with -short")
+	}
+
 	RunSpecs(t, "Capture Controller Suite")
 }
 

pkg/plugin/linuxutil/netstat_stats_linux.go

@@ -62,43 +62,70 @@ func (nr *NetstatReader) readConnectionStats(path string) error {
 		return err
 	}
 
-	// Split the string into lines
+	// The netstat proc file (typically found at /proc/net/netstat) is composed
+	// of pairs of lines describing various statistics. The reference
+	// implementation for this file is found at
+	// https://sourceforge.net/p/net-tools/code/ci/master/tree/statistics.c.
+	// Given that these statistics are separated across lines the file must first
+	// be divided into lines in order to be processed:
 	lines := strings.Split(string(data), "\n")
 
-	if len(lines) < 2 && len(lines)%2 != 0 {
+	// files often end with a trailing newline. After splitting, this would
+	// present itself as a single empty string at the end. If this is the case,
+	// we want to omit this case from the logic that follows
+	if last := len(lines) - 1; lines[last] == "" {
+		lines = lines[0:last]
+	}
+
+	if len(lines) == 1 {
 		return fmt.Errorf("invalid netstat file")
 	}
 
+	// Each pair of lines must then be considered together to properly extract
+	// statistics:
 	for i := 0; i < len(lines); i += 2 {
-		fields1 := strings.Fields(lines[i])
-		if len(fields1) < 2 {
+		// the format of each stat line pair begins with some signifier like
+		// "TcpExt:" followed by one or more statistics. The first line contains
+		// the headers for these statistics and the second line contains the
+		// corresponding value in the same position. In order to access each
+		// statistic, both of these lines must be processed into sets of
+		// whitespace-delineated fields:
+		headers := strings.Fields(lines[i])
+		if len(headers) < 2 {
 			continue
 		}
 
-		fields2 := strings.Fields(lines[i+1])
-		if len(fields2) < 2 {
+		values := strings.Fields(lines[i+1])
+		if len(values) < 2 {
 			continue
 		}
 
-		if fields1[0] != fields2[0] {
+		// The signifiers for each pair of headers and values must match in order
+		// to be properly considered together.
+		if headers[0] != values[0] {
 			continue
 		}
 
-		if len(fields1) != len(fields2) {
+		// Also, the set of statistics is malformed if there is not a corresponding
+		// header for each value:
+		if len(headers) != len(values) {
 			continue
 		}
 
-		if strings.HasPrefix(fields1[0], "TcpExt") && strings.HasPrefix(fields2[0], "TcpExt") {
+		// knowing that there are two well-formed sets of statistics, it's now
+		// possible to examine the signifier and process the statistics into a
+		// semantic collection:
+		if strings.HasPrefix(headers[0], "TcpExt") && strings.HasPrefix(values[0], "TcpExt") {
 			nr.l.Debug("TcpExt found for netstat ")
-			nr.connStats.TcpExt = nr.processConnFields(fields1, fields2)
-		} else if strings.HasPrefix(fields1[0], "IpExt") && strings.HasPrefix(fields2[0], "IpExt") {
+			nr.connStats.TcpExt = nr.processConnFields(headers, values)
+		} else if strings.HasPrefix(headers[0], "IpExt") && strings.HasPrefix(values[0], "IpExt") {
 			nr.l.Debug("IpExt found for netstat ")
-			nr.connStats.IpExt = nr.processConnFields(fields1, fields2)
-		} else if strings.HasPrefix(fields1[0], "MPTcpExt") && strings.HasPrefix(fields2[0], "MPTcpExt") {
+			nr.connStats.IpExt = nr.processConnFields(headers, values)
+		} else if strings.HasPrefix(headers[0], "MPTcpExt") && strings.HasPrefix(values[0], "MPTcpExt") {
 			nr.l.Debug("MPTcpExt found for netstat ")
-			nr.connStats.MPTcpExt = nr.processConnFields(fields1, fields2)
+			nr.connStats.MPTcpExt = nr.processConnFields(headers, values)
 		} else {
-			nr.l.Info("Unknown field found for netstat ", zap.Any("F1", fields1[0]), zap.Any("F2", fields2[0]))
+			nr.l.Info("Unknown field found for netstat ", zap.Any("F1", headers[0]), zap.Any("F2", values[0]))
 			continue
 		}
 

pkg/plugin/linuxutil/netstat_stats_linux_test.go

@@ -110,43 +110,46 @@ func TestReadConnStats(t *testing.T) {
 	}
 
 	for _, tt := range tests {
-		if tt.addValZero {
-			opts.AddZeroVal = true
-		} else {
-			opts.AddZeroVal = false
-		}
+		tt := tt
+		t.Run(tt.name, func(t *testing.T) {
+			if tt.addValZero {
+				opts.AddZeroVal = true
+			} else {
+				opts.AddZeroVal = false
+			}
 
-		ctrl := gomock.NewController(t)
-		defer ctrl.Finish()
+			ctrl := gomock.NewController(t)
+			defer ctrl.Finish()
 
-		ns := NewMockNetstatInterface(ctrl)
-		nr := NewNetstatReader(opts, ns)
-		InitalizeMetricsForTesting(ctrl)
+			ns := NewMockNetstatInterface(ctrl)
+			nr := NewNetstatReader(opts, ns)
+			InitalizeMetricsForTesting(ctrl)
 
-		testmetric := prometheus.NewGauge(prometheus.GaugeOpts{
-			Name: "testmetric",
-			Help: "testmetric",
-		})
+			testmetric := prometheus.NewGauge(prometheus.GaugeOpts{
+				Name: "testmetric",
+				Help: "testmetric",
+			})
 
-		MockGaugeVec.EXPECT().WithLabelValues(gomock.Any()).Return(testmetric).AnyTimes()
+			MockGaugeVec.EXPECT().WithLabelValues(gomock.Any()).Return(testmetric).AnyTimes()
 
-		assert.NotNil(t, nr)
-		err := nr.readConnectionStats(tt.filePath)
-		if tt.wantErr {
-			assert.NotNil(t, err, "Expected error but got nil tetsname: %s", tt.name)
-		} else {
-			assert.Nil(t, err, "Expected nil but got err tetsname: %s", tt.name)
-			assert.NotNil(t, nr.connStats, "Expected data got nil tetsname: %s", tt.name)
-			if tt.checkVals {
-				assert.Equal(t, tt.result, nr.connStats, "Expected data got nil tetsname: %s", tt.name)
+			assert.NotNil(t, nr)
+			err := nr.readConnectionStats(tt.filePath)
+			if tt.wantErr {
+				assert.NotNil(t, err, "Expected error but got nil")
 			} else {
-				assert.Equal(t, len(nr.connStats.TcpExt), tt.totalsCheck["TcpExt"], "Read values are not equal to expected tetsname: %s", tt.name)
-				assert.Equal(t, len(nr.connStats.IpExt), tt.totalsCheck["IpExt"], "Read values are not equal to expected tetsname: %s", tt.name)
-				assert.Equal(t, len(nr.connStats.MPTcpExt), tt.totalsCheck["MPTcpExt"], "Read values are not equal to expected tetsname: %s", tt.name)
-			}
+				assert.Nil(t, err, "Expected nil but got err")
+				assert.NotNil(t, nr.connStats, "Expected data got nil")
+				if tt.checkVals {
+					assert.Equal(t, tt.result, nr.connStats, "Expected data got nil")
+				} else {
+					assert.Equal(t, len(nr.connStats.TcpExt), tt.totalsCheck["TcpExt"], "Read values are not equal to expected")
+					assert.Equal(t, len(nr.connStats.IpExt), tt.totalsCheck["IpExt"], "Read values are not equal to expected")
+					assert.Equal(t, len(nr.connStats.MPTcpExt), tt.totalsCheck["MPTcpExt"], "Read values are not equal to expected")
+				}
 
-			nr.updateMetrics()
-		}
+				nr.updateMetrics()
+			}
+		})
 	}
 }