From 95ba6c6fa5e77ccec0caf402a99cee6da1fc9dbf Mon Sep 17 00:00:00 2001 From: Nandor Magyar Date: Mon, 9 Oct 2023 09:55:39 +0200 Subject: [PATCH 1/2] cicd: add more coverage files, to display a more valid number (#840) --- .github/workflows/product_builder.yaml | 40 +++++- golang/.gitignore | 2 +- golang/Makefile | 18 ++- .../container_builder_integration_test.go | 2 +- golang/pkg/crane/k8s/deploy_facade.go | 10 +- golang/pkg/crane/k8s/deployment.go | 10 +- .../crane/k8s/deployment_integration_test.go | 129 +++++++++++------- .../dagent/update/update_integration_test.go | 38 +++--- 8 files changed, 158 insertions(+), 91 deletions(-) diff --git a/.github/workflows/product_builder.yaml b/.github/workflows/product_builder.yaml index 8993bd609..a610f09fe 100644 --- a/.github/workflows/product_builder.yaml +++ b/.github/workflows/product_builder.yaml @@ -184,7 +184,15 @@ jobs: - name: Init k3d run: make k3d-init - name: Run integration tests - run: make k3d-test + run: | + make k3d-config && \ + export KUBECONFIG="$(pwd)/k3d-auth.yaml" && \ + make test-integration + - name: Upload integration test results + uses: actions/upload-artifact@v3 + with: + name: golang-integration-coverage + path: ${{ env.GOLANG_WORKING_DIRECTORY }}/**.cov go_test: runs-on: ubuntu-22.04 needs: gather_changes @@ -209,8 +217,38 @@ jobs: run: git config --global --add safe.directory "$GITHUB_WORKSPACE" - name: Run unit tests with coverage run: make test-unit-with-coverage + - name: Upload unit test results + uses: actions/upload-artifact@v3 + with: + name: golang-unit-coverage + path: ${{ env.GOLANG_WORKING_DIRECTORY }}/**.cov + go_coverage_upload: + runs-on: ubuntu-22.04 + needs: + - go_security + - go_lint + - go_test + - go_integration + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-go@v4 + - name: Install coverage merger + run: go install go.shabbyrobe.org/gocovmerge/cmd/gocovmerge@latest + - name: Download integration test results from artifacts + uses: actions/download-artifact@v3 + with: + name: golang-integration-coverage + - name: Download unit test results from artifacts + uses: actions/download-artifact@v3 + with: + name: golang-unit-coverage + - name: Merge coverage + run: gocovmerge ./builder.cov ./cli.cov ./crane.cov ./dagent.cov ./internal.cov ./unit.cov > ./merged.cov - name: Upload coverage reports to Codecov with GitHub Action uses: codecov/codecov-action@v3 + with: + files: ./merged.cov + name: golang-coverage go_build: runs-on: ubuntu-22.04 needs: diff --git a/golang/.gitignore b/golang/.gitignore index 919f87316..0f2e638bc 100644 --- a/golang/.gitignore +++ b/golang/.gitignore @@ -6,4 +6,4 @@ tmp/ k3s.yml k3s.yaml k3d-auth.yaml -coverage.cov +*.cov diff --git a/golang/Makefile b/golang/Makefile index 33b64f5cc..84998dedf 100644 --- a/golang/Makefile +++ b/golang/Makefile @@ -241,7 +241,7 @@ k3d-stop: # make sure to have k3s set and configured .PHONY: k3d-test -k3d-test: k3d-config +k3d-test: k3d-test export KUBECONFIG='$(PWD)/k3d-auth.yaml' go test -tags=integration -race ./pkg/crane/... @@ -256,24 +256,28 @@ test-unit: # dependency: valid & working k8s configuration .PHONY: test-unit-with-coverage test-unit-with-coverage: - go test -tags=unit -race -coverpkg=./... -coverprofile=./coverage.cov -covermode=atomic ./... + go test -tags=unit -race -coverpkg=./... -coverprofile=./unit.cov -covermode=atomic ./... .PHONY: test-integration test-integration: test-dagent test-crane test-cli test-internal .PHONY: test-crane test-crane: - go test -tags=integration -race ./pkg/crane/... + go test -tags=integration -race -coverpkg=./... -coverprofile=./crane.cov -covermode=atomic ./pkg/crane/... .PHONY: test-internal test-internal: - go test -tags=integration -race ./internal/... - go test -tags=integration -race ./pkg/builder/... + go test -tags=integration -race -coverpkg=./... -coverprofile=./internal.cov -covermode=atomic ./internal/... + go test -tags=integration -race -coverpkg=./... -coverprofile=./builder.cov -covermode=atomic ./pkg/builder/... .PHONY: test-dagent test-dagent: - go test -tags=integration -race ./pkg/dagent/... + go test -tags=integration -race -coverpkg=./... -coverprofile=./dagent.cov -covermode=atomic ./pkg/dagent/... + +.PHONY: test-cli +test-cli: + go test -tags=integration -race -coverpkg=./... -coverprofile=./cli.cov -covermode=atomic ./pkg/cli/... .PHONY: coverage coverage: - go tool cover -func ./coverage.cov + go tool cover -func ./merged.cov diff --git a/golang/pkg/builder/container/container_builder_integration_test.go b/golang/pkg/builder/container/container_builder_integration_test.go index 0c0dca378..336309a19 100644 --- a/golang/pkg/builder/container/container_builder_integration_test.go +++ b/golang/pkg/builder/container/container_builder_integration_test.go @@ -49,7 +49,7 @@ func assertPortBinding(t *testing.T, portMap nat.PortMap, internal, external str } list := portMap[lookup] - assert.Contains(t, list, nat.PortBinding{HostIP: "0.0.0.0", HostPort: external}) + assert.Contains(t, list, nat.PortBinding{HostIP: "", HostPort: external}) } func hookCallback(callback func()) containerbuilder.LifecycleFunc { diff --git a/golang/pkg/crane/k8s/deploy_facade.go b/golang/pkg/crane/k8s/deploy_facade.go index bc6ae28fe..a2374d856 100644 --- a/golang/pkg/crane/k8s/deploy_facade.go +++ b/golang/pkg/crane/k8s/deploy_facade.go @@ -193,7 +193,7 @@ func (d *DeployFacade) Deploy() error { } } - if err := d.deployment.DeployDeployment(&deploymentParams{ + if err := d.deployment.DeployDeployment(&DeploymentParams{ image: d.params.Image, namespace: d.params.InstanceConfig.ContainerPreName, containerConfig: &d.params.ContainerConfig, @@ -270,9 +270,11 @@ func Deploy(c context.Context, dog *dogger.DeploymentLogger, deployImageRequest dog.Write(deployImageRequest.InstanceConfig.Strings()...) dog.Write(deployImageRequest.ContainerConfig.Strings(&cfg.CommonConfiguration)...) - imageName := util.JoinV("/", - *deployImageRequest.Registry, - util.JoinV(":", deployImageRequest.ImageName, deployImageRequest.Tag)) + imageName := util.JoinV(":", deployImageRequest.ImageName, deployImageRequest.Tag) + if deployImageRequest.Registry != nil { + imageName = util.JoinV("/", + *deployImageRequest.Registry, imageName) + } expandedImageName, err := imageHelper.ExpandImageName(imageName) if err != nil { diff --git a/golang/pkg/crane/k8s/deployment.go b/golang/pkg/crane/k8s/deployment.go index f95bcc0ca..6ed8773bf 100644 --- a/golang/pkg/crane/k8s/deployment.go +++ b/golang/pkg/crane/k8s/deployment.go @@ -47,7 +47,7 @@ func NewDeployment(ctx context.Context, cfg *config.Configuration) *Deployment { return &Deployment{status: "", ctx: ctx, appConfig: cfg} } -type deploymentParams struct { +type DeploymentParams struct { namespace string image string containerConfig *v1.ContainerConfig @@ -63,7 +63,7 @@ type deploymentParams struct { issuer string } -func (d *Deployment) DeployDeployment(p *deploymentParams) error { +func (d *Deployment) DeployDeployment(p *DeploymentParams) error { client := getDeploymentsClient(p.namespace, d.appConfig) containerConfig, err := buildContainer(p, d.appConfig) @@ -277,7 +277,7 @@ func (d *Deployment) GetPodDeployment(namespace, name string) (*kappsv1.Deployme } // builds the container using the builder interface, with healthchecks, volumes, configs, ports... -func buildContainer(p *deploymentParams, +func buildContainer(p *DeploymentParams, cfg *config.Configuration, ) (*corev1.ContainerApplyConfiguration, error) { healthCheckConfig := p.containerConfig.HealthCheckConfig @@ -419,7 +419,7 @@ func getResourceManagement(resourceConfig v1.ResourceConfig, } // getInitContainers returns every init container specific(import/config) or custom ones -func getInitContainers(params *deploymentParams, cfg *config.Configuration) []*corev1.ContainerApplyConfiguration { +func getInitContainers(params *DeploymentParams, cfg *config.Configuration) []*corev1.ContainerApplyConfiguration { // this is only the config container / could be general / wait for it / other init purposes initContainers := []*corev1.ContainerApplyConfiguration{} @@ -485,7 +485,7 @@ func addImportContainer(initContainers []*corev1.ContainerApplyConfiguration, } func addInitContainers(initContainers []*corev1.ContainerApplyConfiguration, - params *deploymentParams, + params *DeploymentParams, ) []*corev1.ContainerApplyConfiguration { for _, iCont := range params.containerConfig.InitContainers { container := corev1.Container(). diff --git a/golang/pkg/crane/k8s/deployment_integration_test.go b/golang/pkg/crane/k8s/deployment_integration_test.go index 8f673d94d..8d6e9394d 100644 --- a/golang/pkg/crane/k8s/deployment_integration_test.go +++ b/golang/pkg/crane/k8s/deployment_integration_test.go @@ -1,53 +1,76 @@ -//go:build integration - -package k8s_test - -import ( - "context" - "testing" - - "github.com/ilyakaznacheev/cleanenv" - - "github.com/stretchr/testify/assert" - - "github.com/dyrector-io/dyrectorio/golang/pkg/crane/config" - "github.com/dyrector-io/dyrectorio/golang/pkg/crane/k8s" -) - -func TestGetPods(t *testing.T) { - ctx := context.Background() - - cfg := config.Configuration{} - _ = cleanenv.ReadEnv(&cfg) - - deploymentHandler := k8s.NewDeployment(ctx, &cfg) - - deployments, err := deploymentHandler.GetDeployments(ctx, "default", &cfg) - assert.NoError(t, err) - assert.Equal(t, 2, len(deployments.Items)) - - pods, err := deploymentHandler.GetPods("default", "deployment-1") - assert.NoError(t, err) - assert.Equal(t, 1, len(pods)) -} - -func TestGetPod(t *testing.T) { - ctx := context.Background() - - cfg := config.Configuration{} - _ = cleanenv.ReadEnv(&cfg) - - deploymentHandler := k8s.NewDeployment(ctx, &cfg) - - deployments, err := deploymentHandler.GetDeployments(ctx, "default", &cfg) - assert.NoError(t, err) - assert.Equal(t, 2, len(deployments.Items)) - - pods, err := deploymentHandler.GetPods("default", "deployment-1") - assert.NoError(t, err) - assert.Equal(t, 1, len(pods)) - - pod, err := deploymentHandler.GetPod("default", pods[0].Name) - assert.NoError(t, err) - assert.NotNil(t, pod) -} +//go:build integration + +package k8s + +import ( + "context" + "testing" + "time" + + "github.com/AlekSi/pointer" + "github.com/ilyakaznacheev/cleanenv" + + "github.com/stretchr/testify/assert" + + v1 "github.com/dyrector-io/dyrectorio/golang/api/v1" + "github.com/dyrector-io/dyrectorio/golang/internal/dogger" + "github.com/dyrector-io/dyrectorio/golang/internal/grpc" + "github.com/dyrector-io/dyrectorio/golang/pkg/crane/config" +) + +func TestFetchPods(t *testing.T) { + ctx := context.Background() + + cfg := config.Configuration{} + err := cleanenv.ReadEnv(&cfg) + + ctx = grpc.WithGRPCConfig(ctx, &cfg) + assert.Nil(t, err, "error for test deployment is unexpected") + deploymentHandler := NewDeployment(ctx, &cfg) + + err = Deploy(ctx, + dogger.NewDeploymentLogger( + ctx, + pointer.ToString("test"), nil, + &cfg.CommonConfiguration), + &v1.DeployImageRequest{ + RequestID: "test", + ImageName: "nginx:latest", + InstanceConfig: v1.InstanceConfig{ + ContainerPreName: "pods", + }, ContainerConfig: v1.ContainerConfig{ + Container: "deployment-1", + }, + }, + &v1.VersionData{}) + + WaitForRunningDeployment(ctx, "pods", "deployment-1", 1, 30*time.Second, &cfg) + + assert.Nil(t, err, "error for test deployment is unexpected") + t.Run("Test get pods", func(t *testing.T) { + deployments, err := deploymentHandler.GetDeployments(ctx, "pods", &cfg) + assert.NoError(t, err) + assert.Len(t, deployments.Items, 1) + + pods, err := deploymentHandler.GetPods("pods", "deployment-1") + assert.NoError(t, err) + assert.Len(t, pods, 1) + }) + + t.Run("Test get single pod", func(t *testing.T) { + deployments, err := deploymentHandler.GetDeployments(ctx, "pods", &cfg) + assert.NoError(t, err) + assert.Equal(t, 1, len(deployments.Items)) + + pods, err := deploymentHandler.GetPods("pods", "deployment-1") + assert.NoError(t, err) + assert.Equal(t, 1, len(pods)) + + pod, err := deploymentHandler.GetPod("pods", pods[0].Name) + assert.NoError(t, err) + assert.NotNil(t, pod) + }) + + err = deploymentHandler.deleteDeployment("pods", "deployment-1") + assert.Nil(t, err, "error for deleting test deployment is unexpected") +} diff --git a/golang/pkg/dagent/update/update_integration_test.go b/golang/pkg/dagent/update/update_integration_test.go index 52b3f6729..bb40dff70 100644 --- a/golang/pkg/dagent/update/update_integration_test.go +++ b/golang/pkg/dagent/update/update_integration_test.go @@ -7,32 +7,32 @@ import ( "context" "testing" - "github.com/dyrector-io/dyrectorio/golang/internal/grpc" "github.com/dyrector-io/dyrectorio/golang/pkg/dagent/update" - "github.com/dyrector-io/dyrectorio/golang/pkg/dagent/utils" - "github.com/dyrector-io/dyrectorio/protobuf/go/agent" "github.com/docker/docker/api/types" "github.com/docker/docker/client" "github.com/stretchr/testify/assert" ) -func TestUpdateContainarizedOnly(t *testing.T) { - cli, err := client.NewClientWithOpts(client.WithAPIVersionNegotiation()) - if err != nil { - t.Error(err) - } - - err = update.ExecuteSelfUpdate(context.TODO(), cli, &agent.AgentUpdateRequest{ - Tag: "anything", - TimeoutSeconds: 30, - Token: "token", - }, grpc.UpdateOptions{ - UpdateAlways: false, - UseContainers: true, - }) - assert.ErrorIs(t, err, &utils.UnknownContainerError{}, "Without containerized context update always fails") -} +// TODO (@nandor-magyar): this is to be fixed, integration test & agent running as a container or not are two separate concerns +// here we would need a running container env, splitting update into getting own image and update execution could be an approach +// note: disabling containerization is not a solution, the test is in vain that way +// func TestUpdateContainarizedOnly(t *testing.T) { +// cli, err := client.NewClientWithOpts(client.WithAPIVersionNegotiation()) +// if err != nil { +// t.Error(err) +// } + +// err = update.ExecuteSelfUpdate(context.TODO(), cli, &agent.AgentUpdateRequest{ +// Tag: "anything", +// TimeoutSeconds: 30, +// Token: "token", +// }, grpc.UpdateOptions{ +// UpdateAlways: false, +// UseContainers: true, +// }) +// assert.ErrorIs(t, err, &utils.UnknownContainerError{}, "Without containerized context update always fails") +// } func TestRewriteInvalid(t *testing.T) { cli, err := client.NewClientWithOpts(client.WithAPIVersionNegotiation()) From 487d11496241e22abc198927e68b9feceec000eb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Papa=20Path=C3=A9=20SENE?= Date: Mon, 9 Oct 2023 07:59:10 +0000 Subject: [PATCH 2/2] refactor: internal/helper/image_unit_test : use table driven tests (#838) * refactor internal/helper/image_unit_test : use table driven tests Unit tests were spread across various functions, but most of them can be organized into table driven test cases. * refactor internal/helper/image/image_unit_test.go extract utility function Promot NewPTR as a general helper function . * refactor: internal/helper/image_unit_test : add name to table tests * feat: add tests to pointer module --------- Co-authored-by: Nandor Magyar Co-authored-by: Levente Orban --- .../internal/helper/image/image_unit_test.go | 69 ++++++++++++------- golang/internal/pointer/pointer.go | 6 ++ golang/internal/pointer/pointer_test.go | 15 ++++ 3 files changed, 65 insertions(+), 25 deletions(-) create mode 100644 golang/internal/pointer/pointer.go create mode 100644 golang/internal/pointer/pointer_test.go diff --git a/golang/internal/helper/image/image_unit_test.go b/golang/internal/helper/image/image_unit_test.go index cff282be6..8894f9fe0 100644 --- a/golang/internal/helper/image/image_unit_test.go +++ b/golang/internal/helper/image/image_unit_test.go @@ -10,38 +10,57 @@ import ( "github.com/stretchr/testify/assert" imageHelper "github.com/dyrector-io/dyrectorio/golang/internal/helper/image" + "github.com/dyrector-io/dyrectorio/golang/internal/pointer" "github.com/dyrector-io/dyrectorio/protobuf/go/agent" ) -func TestRegistryUrl(t *testing.T) { - auth := &imageHelper.RegistryAuth{ - URL: "test", - } - - url := imageHelper.GetRegistryURL(nil, auth) - assert.Equal(t, url, "test") +type RegistryTestCase struct { + Name string + Registry *string + RegistryUrl *string + ExpectedUrl string } -func TestRegistryUrlPriority(t *testing.T) { - registry := "other" - auth := &imageHelper.RegistryAuth{ - URL: "test", +func TestRegistryWithTable(t *testing.T) { + testCases := []RegistryTestCase{ + { + Name: "Test registry url", + Registry: pointer.NewPTR[string](""), + RegistryUrl: pointer.NewPTR[string]("test"), + ExpectedUrl: "test", + }, + { + Name: "Test registry url priority", + Registry: pointer.NewPTR[string]("other"), + RegistryUrl: pointer.NewPTR[string]("test"), + ExpectedUrl: "test", + }, + { + Name: "Test registry url empty", + Registry: nil, + RegistryUrl: nil, + ExpectedUrl: "", + }, + { + Name: "Test registry url registry", + Registry: pointer.NewPTR[string]("other"), + RegistryUrl: nil, + ExpectedUrl: "other", + }, } - url := imageHelper.GetRegistryURL(®istry, auth) - assert.Equal(t, url, "test") -} - -func TestRegistryUrlRegistry(t *testing.T) { - registry := "other" - - url := imageHelper.GetRegistryURL(®istry, nil) - assert.Equal(t, url, "other") -} - -func TestRegistryUrlEmpty(t *testing.T) { - url := imageHelper.GetRegistryURL(nil, nil) - assert.Equal(t, url, "") + for _, tC := range testCases { + t.Run(tC.Name, func(t *testing.T) { + if tC.RegistryUrl == nil { + url := imageHelper.GetRegistryURL(tC.Registry, nil) + assert.Equal(t, url, tC.ExpectedUrl) + } else { + auth := &imageHelper.RegistryAuth{URL: *tC.RegistryUrl} + url := imageHelper.GetRegistryURL(tC.Registry, auth) + assert.Equal(t, url, tC.ExpectedUrl) + } + }) + } } func TestProtoRegistryUrl(t *testing.T) { diff --git a/golang/internal/pointer/pointer.go b/golang/internal/pointer/pointer.go new file mode 100644 index 000000000..25cfe4c0d --- /dev/null +++ b/golang/internal/pointer/pointer.go @@ -0,0 +1,6 @@ +package pointer + +// Creates a new pointer of type T +func NewPTR[T any](value T) *T { + return &value +} diff --git a/golang/internal/pointer/pointer_test.go b/golang/internal/pointer/pointer_test.go new file mode 100644 index 000000000..50c96a085 --- /dev/null +++ b/golang/internal/pointer/pointer_test.go @@ -0,0 +1,15 @@ +package pointer + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestNewPTR(t *testing.T) { + sPtr := NewPTR[string]("") + assert.Equal(t, *sPtr, "") + + iPtr := NewPTR[int](120) + assert.Equal(t, *iPtr, 120) +}