From 0c97187fab585d3d4dc8be13a74796c7e90f6614 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=BD=97=E6=B3=BD=E8=BD=A9?= Date: Tue, 27 Feb 2024 10:04:57 +0800 Subject: [PATCH] ci: add more coverage (#332) Signed-off-by: spacewander --- .github/codecov.yml | 8 +- .github/workflows/test.yml | 46 +++++++-- Makefile | 4 +- README.md | 1 + controller/Makefile | 2 +- pkg/plugins/mock.go | 36 +++++++ pkg/plugins/plugins.go | 22 +++-- pkg/plugins/plugins_test.go | 93 +++++++++++++++++-- pkg/plugins/type.go | 2 +- .../control_plane/control_plane.go | 24 +++++ .../integration/data_plane/data_plane.go | 40 +++++++- plugins/tests/integration/helper/helper.go | 5 + plugins/tests/integration/test_plugins.go | 34 +++++++ 13 files changed, 287 insertions(+), 30 deletions(-) diff --git a/.github/codecov.yml b/.github/codecov.yml index 20a89af3..f651b518 100644 --- a/.github/codecov.yml +++ b/.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' diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 3ca48af5..c2f36810 100644 --- a/.github/workflows/test.yml +++ b/.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/codecov-action@v4.0.1 + - 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/codecov-action@v4.0.1 + with: + fail_ci_if_error: true + file: ./cover.out,plugins/tests/integration/cover.out,./controller/cover.out + token: ${{ secrets.CODECOV_TOKEN }} + verbose: true diff --git a/Makefile b/Makefile index e7ab9bef..2e070453 100644 --- a/Makefile +++ b/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; \ ) diff --git a/README.md b/README.md index a520ac82..8eb213c4 100644 --- a/README.md +++ b/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... diff --git a/controller/Makefile b/controller/Makefile index f33c2b60..4121cc61 100644 --- a/controller/Makefile +++ b/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 diff --git a/pkg/plugins/mock.go b/pkg/plugins/mock.go index d628d716..1b939dc7 100644 --- a/pkg/plugins/mock.go +++ b/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 +} diff --git a/pkg/plugins/plugins.go b/pkg/plugins/plugins.go index 0cfdd484..6fe1b224 100644 --- a/pkg/plugins/plugins.go +++ b/pkg/plugins/plugins.go @@ -55,9 +55,17 @@ 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) @@ -65,22 +73,24 @@ func RegisterHttpPlugin(name string, plugin Plugin) { 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) } } diff --git a/pkg/plugins/plugins_test.go b/pkg/plugins/plugins_test.go index db8bea4f..41b7a79a 100644 --- a/pkg/plugins/plugins_test.go +++ b/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) + }) + }) + } +} diff --git a/pkg/plugins/type.go b/pkg/plugins/type.go index 981d87e0..49645e95 100644 --- a/pkg/plugins/type.go +++ b/pkg/plugins/type.go @@ -111,7 +111,7 @@ type GoPlugin interface { } type ConsumerPlugin interface { - Plugin + GoPlugin ConsumerConfig() api.PluginConsumerConfig } diff --git a/plugins/tests/integration/control_plane/control_plane.go b/plugins/tests/integration/control_plane/control_plane.go index 3085c78d..51f133fe 100644 --- a/plugins/tests/integration/control_plane/control_plane.go +++ b/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{ diff --git a/plugins/tests/integration/data_plane/data_plane.go b/plugins/tests/integration/data_plane/data_plane.go index 1e7e072d..d13c8c1a 100644 --- a/plugins/tests/integration/data_plane/data_plane.go +++ b/plugins/tests/integration/data_plane/data_plane.go @@ -18,6 +18,7 @@ import ( "bufio" "context" "crypto/md5" + "fmt" "io" "net" "net/http" @@ -124,6 +125,19 @@ func StartDataPlane(t *testing.T, opt *Option) (*DataPlane, error) { } } + coverDir := helper.CoverDir() + _, err = os.Stat(coverDir) + if err != nil { + if !os.IsNotExist(err) { + return nil, err + } + if err := os.MkdirAll(coverDir, 0755); err != nil { + return nil, err + } + // When the integration test is run with `go test ...`, the previous coverage files are not removed. + // Since we only care about the coverage in CI, it is fine so far. + } + // This is the envoyproxy/envoy:contrib-dev fetched in 2024-02-08 // Use docker inspect --format='{{index .RepoDigests 0}}' envoyproxy/envoy:contrib-dev // to get the sha256 ID @@ -139,6 +153,7 @@ func StartDataPlane(t *testing.T, opt *Option) (*DataPlane, error) { projectRoot + "/plugins/tests/integration/libgolang.so:/etc/libgolang.so" + " -v /tmp:/tmp" + + " -e GOCOVERDIR=" + coverDir + " -p 10000:10000 -p 9998:9998 " + hostAddr + " " + image @@ -230,9 +245,14 @@ func (dp *DataPlane) cleanup(t *testing.T) error { } func (dp *DataPlane) Stop() { + err := dp.FlushCoverage() + if err != nil { + logger.Error(err, "failed to flush coverage") + } + logger.Info("stop envoy") cmd := exec.Command("docker", "stop", containerName) - err := cmd.Run() + err = cmd.Run() if err != nil { logger.Error(err, "failed to terminate envoy") return @@ -305,6 +325,20 @@ func (dp *DataPlane) do(method string, path string, header http.Header, body io. } func (dp *DataPlane) Configured() bool { - _, err := dp.Head("/detect_if_the_rds_takes_effect", nil) - return err == nil + resp, err := dp.Head("/detect_if_the_rds_takes_effect", nil) + if err != nil { + return false + } + return resp.StatusCode == 200 +} + +func (dp *DataPlane) FlushCoverage() error { + resp, err := dp.Post("/flush_coverage", nil, nil) + if err != nil { + return err + } + if resp.StatusCode != 200 { + return fmt.Errorf("unexpected status code: %d", resp.StatusCode) + } + return nil } diff --git a/plugins/tests/integration/helper/helper.go b/plugins/tests/integration/helper/helper.go index b3ef34ea..6930836b 100644 --- a/plugins/tests/integration/helper/helper.go +++ b/plugins/tests/integration/helper/helper.go @@ -18,6 +18,7 @@ import ( "fmt" "net" "os" + "path/filepath" "testing" "time" @@ -44,3 +45,7 @@ func WaitServiceUp(t *testing.T, port string, service string) { return true }, 10*time.Second, 50*time.Millisecond, msg) } + +func CoverDir() string { + return filepath.Join("/tmp", "htnn_coverage") +} diff --git a/plugins/tests/integration/test_plugins.go b/plugins/tests/integration/test_plugins.go index b489b150..38ffcb4b 100644 --- a/plugins/tests/integration/test_plugins.go +++ b/plugins/tests/integration/test_plugins.go @@ -16,11 +16,13 @@ package integration import ( "net/http" + "runtime/coverage" "runtime/debug" "strings" "mosn.io/htnn/pkg/filtermanager/api" "mosn.io/htnn/pkg/plugins" + "mosn.io/htnn/plugins/tests/integration/helper" ) type basePlugin struct { @@ -254,8 +256,40 @@ func (p *localReplyPlugin) Factory() api.FilterFactory { return localReplyFactory } +type coveragePlugin struct { + plugins.PluginMethodDefaultImpl + basePlugin +} + +func (p *coveragePlugin) Factory() api.FilterFactory { + return coverageFactory +} + +func coverageFactory(c interface{}, callbacks api.FilterCallbackHandler) api.Filter { + return &coverageFilter{ + callbacks: callbacks, + } +} + +type coverageFilter struct { + api.PassThroughFilter + + callbacks api.FilterCallbackHandler +} + +func (f *coverageFilter) DecodeHeaders(headers api.RequestHeaderMap, endStream bool) api.ResultAction { + err := coverage.WriteCountersDir(helper.CoverDir()) + if err != nil { + api.LogErrorf("failed to write coverage: %v", err) + return &api.LocalResponse{Code: 500} + } + return &api.LocalResponse{Code: 200} +} + func init() { plugins.RegisterHttpPlugin("stream", &streamPlugin{}) plugins.RegisterHttpPlugin("buffer", &bufferPlugin{}) plugins.RegisterHttpPlugin("localReply", &localReplyPlugin{}) + + plugins.RegisterHttpPlugin("coverage", &coveragePlugin{}) }