ci: add more coverage (#332)

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

.github/codecov.yml

@@ -3,18 +3,18 @@ coverage:
     project:
       default:
         target: 80%
-        threshold: 5%
+        threshold: 5% # 75%~80% is the yellow zone - ok to merge but should increase it
         if_ci_failed: error
     patch:
       default:
-        target: 80%
-        threshold: 20% # higher threshold for single change
+        target: 70% # lower target for single change
         if_ci_failed: error
 ignore:
   # test files
   - "plugins/tests"
   - "controller/tests"
   - "e2e/"
+  - "**/mock.go"
   # examples
   - "examples/"
   # tools
@@ -23,4 +23,4 @@ ignore:
   # generated code
   - "**/*.pb.go"
   - "**/*.pb.validate.go"
-  - 'zz_generated.deepcopy.go'
+  - '**/zz_generated.deepcopy.go'

.github/workflows/test.yml

@@ -18,7 +18,6 @@ concurrency:
 
 env:
   IN_CI: true
-  COVERAGE_OPTION: "-covermode=atomic -coverprofile=coverage.out"
 
 jobs:
   unit-test:
@@ -34,14 +33,12 @@ jobs:
 
     - name: Test
       run: make unit-test
-    - name: Upload to codecov
-      uses: codecov/[email protected]
+    - name: Upload artifact
+      if: always() # always upload coverage, so the coverage percents won't affect by the failed tests
+      uses: actions/upload-artifact@v4
       with:
-        fail_ci_if_error: false # let CI build pass if Codecov runs into an error during upload
-        file: ./coverage.out
-        flags: unittests
-        token: ${{ secrets.CODECOV_TOKEN }}
-        verbose: true
+        name: unit-test-cover
+        path: ./cover.out
 
   plugins-integration-test:
     timeout-minutes: 10
@@ -70,6 +67,16 @@ jobs:
         # upload artifact can be found in https://github.com/mosn/htnn/actions/runs/$id
         name: ci-logs
         path: ./test-envoy
+    - name: Generate coverage
+      if: always()
+      run: |
+        go tool covdata textfmt -i=/tmp/htnn_coverage -o plugins/tests/integration/cover.out -v 2
+    - name: Upload artifact
+      if: always()
+      uses: actions/upload-artifact@v4
+      with:
+        name: plugins-integration-test-cover
+        path: plugins/tests/integration/cover.out
 
   controller-test:
     timeout-minutes: 10
@@ -97,3 +104,26 @@ jobs:
 
     - name: Test
       run: make test
+    - name: Upload artifact
+      if: always()
+      uses: actions/upload-artifact@v4
+      with:
+        name: controller-test-cover
+        path: ./controller/cover.out
+
+  coverage:
+    timeout-minutes: 10
+    runs-on: ubuntu-latest
+    needs: [unit-test, plugins-integration-test, controller-test]
+    if: always()
+    steps:
+    - uses: actions/checkout@v4
+    - name: Download artifact
+      uses: actions/download-artifact@v4
+    - name: Upload to codecov
+      uses: codecov/[email protected]
+      with:
+        fail_ci_if_error: true
+        file: ./cover.out,plugins/tests/integration/cover.out,./controller/cover.out
+        token: ${{ secrets.CODECOV_TOKEN }}
+        verbose: true

Makefile

@@ -39,7 +39,7 @@ PROTOC = protoc
 PROTO_FILES = $(call rwildcard,./,*.proto)
 GO_TARGETS = $(patsubst %.proto,%.pb.go,$(PROTO_FILES))
 
-TEST_OPTION ?= -gcflags="all=-N -l" -race $(COVERAGE_OPTION)
+TEST_OPTION ?= -gcflags="all=-N -l" -race -covermode=atomic -coverprofile=cover.out -coverpkg=./...
 
 MOUNT_GOMOD_CACHE = -v $(shell go env GOPATH):/go
 ifeq ($(IN_CI), true)
@@ -92,6 +92,7 @@ build-test-so-local:
 	CGO_ENABLED=1 go build -tags so \
 		-ldflags "-B 0x$(shell head -c20 /dev/urandom|od -An -tx1|tr -d ' \n') -X main.Version=${VERSION}(${GIT_VERSION})" \
 		--buildmode=c-shared \
+		-cover -covermode=atomic -coverpkg=./... \
 		-v -o plugins/tests/integration/${TARGET_SO} \
 		${PROJECT_NAME}/plugins/tests/integration/libgolang
 
@@ -109,6 +110,7 @@ plugins-integration-test:
 	if ! docker images ${PROXY_IMAGE} | grep envoyproxy/envoy > /dev/null; then \
 		docker pull ${PROXY_IMAGE}; \
 	fi
+	test -d /tmp/htnn_coverage && rm -rf /tmp/htnn_coverage || true
 	$(foreach PKG, $(shell go list ./plugins/tests/integration/...), \
 		go test -v ${PKG} || exit 1; \
 	)

README.md

@@ -1,6 +1,7 @@
 # HTNN
 
 [![test](https://github.com/mosn/htnn/actions/workflows/test.yml/badge.svg)](https://github.com/mosn/htnn/actions/workflows/test.yml)
+[![codecov](https://codecov.io/gh/mosn/htnn/branch/main/graph/badge.svg)](https://codecov.io/gh/mosn/htnn)
 
 HTNN is...
 

controller/Makefile

@@ -78,7 +78,7 @@ generate: controller-gen ## Generate code containing DeepCopy, DeepCopyInto, and
 .PHONY: test
 test: manifests generate envtest ## Run tests.
 	KUBEBUILDER_ASSETS="$(shell $(ENVTEST) use $(ENVTEST_K8S_VERSION) --bin-dir $(LOCALBIN) -p path)" go test -gcflags="all=-N -l" -race \
-		./... -coverprofile cover.out
+		./... -coverprofile cover.out -covermode=atomic -coverpkg=./...
 
 .PHONY: benchmark
 benchmark: manifests generate envtest ## Run benchmarks

pkg/plugins/mock.go

@@ -37,3 +37,39 @@ func (m *MockPlugin) Merge(parent interface{}, child interface{}) interface{} {
 type MockPluginConfig struct {
 	Config
 }
+
+type MockConsumerPlugin struct {
+	MockPlugin
+}
+
+func (m *MockConsumerPlugin) Order() PluginOrder {
+	return PluginOrder{
+		Position: OrderPositionAuthn,
+	}
+}
+
+func (m *MockConsumerPlugin) ConsumerConfig() api.PluginConsumerConfig {
+	return nil
+}
+
+type MockNativePlugin struct {
+	PluginMethodDefaultImpl
+}
+
+func (m *MockNativePlugin) Config() api.PluginConfig {
+	return &MockPluginConfig{}
+}
+
+func (m *MockNativePlugin) Order() PluginOrder {
+	return PluginOrder{
+		Position: OrderPositionOuter,
+	}
+}
+
+func (m *MockNativePlugin) RouteConfigTypeURL() string {
+	return ""
+}
+
+func (m *MockNativePlugin) HTTPFilterConfigPlaceholder() map[string]interface{} {
+	return nil
+}

pkg/plugins/plugins.go

@@ -55,32 +55,42 @@ func LoadHttpFilterFactoryAndParser(name string) *FilterFactoryAndParser {
 	return httpFilterFactoryAndParser[name]
 }
 
+const (
+	errNilPlugin                  = "plugin should not be nil"
+	errUnknownPluginType          = "a plugin should be either Go plugin or Native plugin"
+	errInvalidGoPluginOrder       = "invalid plugin order position: Go plugin should not use OrderPositionOuter or OrderPositionInner"
+	errInvalidNativePluginOrder   = "invalid plugin order position: Native plugin should use OrderPositionOuter or OrderPositionInner"
+	errInvalidConsumerPluginOrder = "invalid plugin order position: Consumer plugin should use OrderPositionAuthn"
+)
+
 func RegisterHttpPlugin(name string, plugin Plugin) {
 	if plugin == nil {
-		panic("plugin should not be nil")
+		panic(errNilPlugin)
 	}
 
 	logger.Info("register plugin", "name", name)
 
 	if goPlugin, ok := plugin.(GoPlugin); ok {
 		order := plugin.Order()
 		if order.Position == OrderPositionOuter || order.Position == OrderPositionInner {
-			panic("invalid plugin order position: Go plugin should not use OrderPositionOuter or OrderPositionInner")
+			panic(errInvalidGoPluginOrder)
 		}
 		RegisterHttpFilterFactoryAndParser(name,
 			goPlugin.Factory(),
 			NewPluginConfigParser(goPlugin))
-	}
-	if _, ok := plugin.(NativePlugin); ok {
+	} else if _, ok := plugin.(NativePlugin); ok {
 		order := plugin.Order()
 		if order.Position != OrderPositionOuter && order.Position != OrderPositionInner {
-			panic("invalid plugin order position: Native plugin should use OrderPositionOuter or OrderPositionInner")
+			panic(errInvalidNativePluginOrder)
 		}
+	} else {
+		panic(errUnknownPluginType)
 	}
+
 	if _, ok := plugin.(ConsumerPlugin); ok {
 		order := plugin.Order()
 		if order.Position != OrderPositionAuthn {
-			panic("invalid plugin order position: Consumer plugin should use OrderPositionAuthn")
+			panic(errInvalidConsumerPluginOrder)
 		}
 	}
 

pkg/plugins/plugins_test.go

@@ -113,13 +113,33 @@ func TestMerge(t *testing.T) {
 	assert.Equal(t, "parent", res)
 }
 
-type pluginOrderWrapper struct {
-	Plugin
+type goPluginOrderWrapper struct {
+	GoPlugin
 
 	order PluginOrder
 }
 
-func (p *pluginOrderWrapper) Order() PluginOrder {
+func (p *goPluginOrderWrapper) Order() PluginOrder {
+	return p.order
+}
+
+type consumerPluginWrapper struct {
+	ConsumerPlugin
+
+	order PluginOrder
+}
+
+func (p *consumerPluginWrapper) Order() PluginOrder {
+	return p.order
+}
+
+type nativePluginWrapper struct {
+	NativePlugin
+
+	order PluginOrder
+}
+
+func (p *nativePluginWrapper) Order() PluginOrder {
 	return p.order
 }
 
@@ -146,9 +166,9 @@ func TestComparePluginOrder(t *testing.T) {
 		},
 	}
 	for name, po := range pluginOrders {
-		RegisterHttpPlugin(name, &pluginOrderWrapper{
-			Plugin: plugin,
-			order:  po,
+		RegisterHttpPlugin(name, &goPluginOrderWrapper{
+			GoPlugin: plugin,
+			order:    po,
 		})
 	}
 
@@ -170,3 +190,64 @@ func TestComparePluginOrder(t *testing.T) {
 		"authz_last",
 	}, plugins)
 }
+
+func TestRejectBadPluginDef(t *testing.T) {
+	type pluginWrapper struct {
+		Plugin
+	}
+
+	cases := []struct {
+		name  string
+		input Plugin
+		err   string
+	}{
+		{
+			name: "unknown type",
+			input: &pluginWrapper{
+				Plugin: &MockPlugin{},
+			},
+			err: errUnknownPluginType,
+		},
+		{
+			name: "nil plugin",
+			err:  errNilPlugin,
+		},
+		{
+			name: "invalid Go plugin order",
+			input: &goPluginOrderWrapper{
+				GoPlugin: &MockPlugin{},
+				order: PluginOrder{
+					Position: OrderPositionInner,
+				},
+			},
+			err: errInvalidGoPluginOrder,
+		},
+		{
+			name: "invalid Native plugin order",
+			input: &nativePluginWrapper{
+				NativePlugin: &MockNativePlugin{},
+				order: PluginOrder{
+					Position: OrderPositionAuthz,
+				},
+			},
+			err: errInvalidNativePluginOrder,
+		},
+		{
+			name: "invalid Consumer plugin order",
+			input: &consumerPluginWrapper{
+				ConsumerPlugin: &MockConsumerPlugin{},
+				order: PluginOrder{
+					Position: OrderPositionAuthz,
+				},
+			},
+			err: errInvalidConsumerPluginOrder,
+		},
+	}
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			assert.PanicsWithValue(t, c.err, func() {
+				RegisterHttpPlugin(c.name, c.input)
+			})
+		})
+	}
+}

pkg/plugins/type.go

@@ -111,7 +111,7 @@ type GoPlugin interface {
 }
 
 type ConsumerPlugin interface {
-	Plugin
+	GoPlugin
 
 	ConsumerConfig() api.PluginConsumerConfig
 }

plugins/tests/integration/control_plane/control_plane.go

@@ -182,6 +182,30 @@ func (cp *ControlPlane) UseGoPluginConfig(config *filtermanager.FilterManagerCon
 									},
 								},
 							},
+							{
+								Match: &route.RouteMatch{
+									PathSpecifier: &route.RouteMatch_Path{
+										Path: "/flush_coverage",
+									},
+								},
+								Action: &route.Route_DirectResponse{
+									DirectResponse: &route.DirectResponseAction{
+										Status: 200,
+									},
+								},
+								TypedPerFilterConfig: map[string]*any1.Any{
+									"envoy.filters.http.golang": proto.MessageToAny(&golang.ConfigsPerRoute{
+										PluginsConfig: map[string]*golang.RouterPlugin{
+											"fm": {
+												Override: &golang.RouterPlugin_Config{
+													Config: proto.MessageToAny(
+														FilterManagerConfigToTypedStruct(NewSinglePluinConfig("coverage", nil))),
+												},
+											},
+										},
+									}),
+								},
+							},
 							{
 								Match: &route.RouteMatch{
 									PathSpecifier: &route.RouteMatch_Prefix{