fix(tests): make all the tests pass (#34)

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. --------- Signed-off-by: Anubhab Majumdar <[email protected]> Signed-off-by: Evan Baker <[email protected]> Co-authored-by: Anubhab Majumdar <[email protected]> Co-authored-by: Evan Baker <[email protected]>
microsoft · Mar 15, 2024 · 016dce9 · 016dce9
1 parent ed7d9d7
commit 016dce9
Show file tree

Hide file tree

Showing 12 changed files with 96 additions and 71 deletions.
diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml
@@ -29,7 +29,7 @@ jobs:
           GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
           PULL_REQUEST_NUMBER: ${{ github.event.pull_request.number }}
         run: |
-          make retina-test-image IMAGE_NAMESPACE=${{ github.repository }} PLATFORM=linux/amd64
+          make test-image IMAGE_NAMESPACE=${{ github.repository }} PLATFORM=linux/amd64
       - name: Upload Artifacts
         uses: actions/upload-artifact@v4
         with:

diff --git a/Makefile b/Makefile
@@ -188,7 +188,7 @@ retina-capture-workload: ## build the Retina capture workload
 ##@ Containers
 
 IMAGE_REGISTRY	?= ghcr.io
-IMAGE_NAMESPACE ?= $(shell git config --get remote.origin.url | sed -E 's/.*github\.com[\/:]([^\/]+)\/([^\/.]+).git/\1\/\2/') # attempts to extract the upstream for image namespacing
+IMAGE_NAMESPACE ?= $(shell git config --get remote.origin.url | sed -E 's/.*github\.com[\/:]([^\/]+)\/([^\/.]+).git/\1\/\2/')
 
 RETINA_BUILDER_IMAGE			= $(IMAGE_NAMESPACE)/retina-builder
 RETINA_TOOLS_IMAGE				= $(IMAGE_NAMESPACE)/retina-tools
@@ -232,10 +232,9 @@ buildx:
 	fi;
 
 container-docker: buildx # util target to build container images using docker buildx. do not invoke directly.
-	echo "Building for platform $(PLATFORM)"
-	os=$$(echo $(PLATFORM) | cut -d'/' -f1); \
-	arch=$$(echo $(PLATFORM) | cut -d'/' -f2); \
-	echo "Building for $$os/$$arch"; \
+	@os=$$(echo $(PLATFORM) | cut -d'/' -f1); \
+	@arch=$$(echo $(PLATFORM) | cut -d'/' -f2); \
+	@echo "Building for $$os/$$arch"; \
 	docker buildx build \
 		$(ACTION) \
 		--platform $(PLATFORM) \
@@ -360,34 +359,23 @@ manifest:
 # Make sure the layer has only one directory.
 # the test DockerFile needs to build the scratch stage with all the output files 
 # and we will untar the archive and copy the files from scratch stage
-retina-test-image: ## build the retina container image for testing.
+test-image: ## build the retina container image for testing.
 	$(MAKE) container-docker \
 			PLATFORM=$(PLATFORM) \
 			DOCKERFILE=./test/image/Dockerfile \
 			REGISTRY=$(IMAGE_REGISTRY) \
 			IMAGE=$(RETINA_IMAGE) \
 			CONTEXT_DIR=$(REPO_ROOT) \
 			TAG=$(RETINA_PLATFORM_TAG) \
-
-	docker save -o archives.tar $(IMAGE_REGISTRY)/$(RETINA_IMAGE):$(RETINA_PLATFORM_TAG) && \
-	mkdir -p archivelayers && \
-	cp archives.tar archivelayers/archives.tar && \
-	cd archivelayers/ && \
-	pwd && \
-	tar -xvf archives.tar && \
-	cd `ls -d */` && \
-	pwd && \
-	tar -xvf layer.tar && \
-	cp coverage.out ../../
-	$(MAKE) retina-cc
+			ACTION=--load
 
 COVER_PKG ?= .
 
-retina-ut: $(ENVTEST) # Run unit tests.
+test: $(ENVTEST) # Run unit tests.
 	go build -o test-summary ./test/utsummary/main.go
 	CGO_ENABLED=0 KUBEBUILDER_ASSETS="$(shell $(ENVTEST) use -p path)" go test -tags=unit -coverprofile=coverage.out -v -json ./... | ./test-summary --progress --verbose
 
-retina-cc: # Code coverage.
+coverage: # Code coverage.
 #	go generate ./... && go test -tags=unit -coverprofile=coverage.out.tmp ./...
 	cat coverage.out | grep -v "_bpf.go\|_bpfel_x86.go\|_bpfel_arm64.go|_generated.go|mock_" | grep -v mock > coveragenew.out
 	go tool cover -html coveragenew.out -o coverage.html

diff --git a/docs/contributing/readme.md b/docs/contributing/readme.md
@@ -43,8 +43,8 @@ Download [Helm](https://helm.sh/) as well.
 ### Test
 
 ```bash
-make retina-ut # run unit-test locally
-make retina-test-image # run tests in docker container
+make test # run unit-test locally
+make test-image # run tests in docker container
 ```
 
 ### Build

diff --git a/pkg/capture/crd_to_job_test.go b/pkg/capture/crd_to_job_test.go
@@ -40,7 +40,7 @@ func NewCaptureToPodTranslatorForTest(kubeClient kubernetes.Interface) *CaptureT
 	log.SetupZapLogger(log.GetDefaultLogOpts())
 	config := config.CaptureConfig{
 		CaptureDebug:              true,
-		CaptureImageVersion:       "v0.0.6-52-g07caaaf",
+		CaptureImageVersion:       "v0.0.1-pre",
 		CaptureImageVersionSource: captureUtils.VersionSourceOperatorImageVersion,
 		CaptureJobNumLimit:        10,
 	}

diff --git a/pkg/controllers/daemon/retinaendpoint/suite_test.go b/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")
 }
 

diff --git a/pkg/plugin/dropreason/kprobe_bpf.o b/pkg/plugin/dropreason/kprobe_bpf.o
diff --git a/pkg/plugin/linuxutil/netstat_stats_linux.go b/pkg/plugin/linuxutil/netstat_stats_linux.go
@@ -54,6 +54,7 @@ func (nr *NetstatReader) readAndUpdate() error {
 	return nil
 }
 
+//nolint:gomnd // magic numbers are sufficiently explained in this function
 func (nr *NetstatReader) readConnectionStats(path string) error {
 	// Read the contents of the file into a string
 	data, err := os.ReadFile(path)
@@ -62,43 +63,71 @@ 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") {
+		//nolint:gocritic // this should be rewritten, but won't be at time of this writing
+		// 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
 		}
 

diff --git a/pkg/plugin/linuxutil/netstat_stats_linux_test.go b/pkg/plugin/linuxutil/netstat_stats_linux_test.go
@@ -29,6 +29,7 @@ func TestNewNetstatReader(t *testing.T) {
 	assert.NotNil(t, nr)
 }
 
+//nolint:testifylint // not making linter changes to preserve exact behavior
 func TestReadConnStats(t *testing.T) {
 	log.SetupZapLogger(log.GetDefaultLogOpts())
 	opts := &NetstatOpts{
@@ -110,43 +111,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()
+			}
+		})
 	}
 }
 

diff --git a/pkg/plugin/packetforward/packetforward_bpf.o b/pkg/plugin/packetforward/packetforward_bpf.o
diff --git a/pkg/plugin/packetparser/packetparser_bpf.o b/pkg/plugin/packetparser/packetparser_bpf.o
diff --git a/scripts/coverage/README.md b/scripts/coverage/README.md
@@ -12,4 +12,4 @@ We use GITHUB_TOKEN to get the following:
 https: //docs.github.com/en/authentication/keeping-your-account-and-data-secure/creating-a-personal-access-token#creating-a-personal-access-token-classic
 2. Set a test PR number as `PULL_REQUEST_NUMBER` env variable
 3. generate coverage.out for local branch with `go test -coverprofile=coverage.out ./...`
-4. run `make retina-cc` on your local branch and not main.
+4. run `make coverage` on your local branch and not main.
diff --git a/test/image/Dockerfile b/test/image/Dockerfile
@@ -19,7 +19,7 @@ WORKDIR /go/src/github.com/microsoft/retina
 # RUN go mod edit -module retina
 # RUN make all generate
 #RUN go generate ./...
-RUN make retina-ut
+RUN make test
 #RUN go test -covermode count -coverprofile /home/runner/work/_temp/go.cov -coverpkg ./... ./...
 
 RUN cat coverage.out