Code coverage (kserve#2351)

* # This is a combination of 6 commits. # This is the 1st commit message: Modify agent test case for code coverage (kserve#1849) * Modifies the test case for sync models config Signed-off-by: Andrews Arokiam <[email protected]> # This is the commit message #2: Add test cases for agent storage utils (kserve#1849) * Add test case for FileExists function * Add test case for RemoveDir function Signed-off-by: Andrews Arokiam <[email protected]> # This is the commit message #3: Add test case for agent storage utils * Add test case for GetProvider function Signed-off-by: Andrews Arokiam <[email protected]> # This is the commit message #4: Add test case for gcs model downloader (kserve#1849) * Add test case for gcs model downloader in agent Signed-off-by: Andrews Arokiam <[email protected]> # This is the commit message #5: Add test cases for agent downloader Signed-off-by: Andrews Arokiam <[email protected]> # This is the commit message #6: Add test cases for configmap (kserve#1849) * Add test cases for v1beta1 configmap Signed-off-by: Andrews Arokiam <[email protected]> * Add test cases for inference service defaults (kserve#1849) * Add test cases for all model runtimes * Add test cases for all runtime defaults Signed-off-by: Andrews Arokiam <[email protected]> * fmt (kserve#1849) Signed-off-by: Andrews Arokiam <[email protected]> * Modify agent test case for code coverage (kserve#1849) * Modifies the test case for sync models config Signed-off-by: Andrews Arokiam <[email protected]> Add test cases for agent storage utils (kserve#1849) * Add test case for FileExists function * Add test case for RemoveDir function Signed-off-by: Andrews Arokiam <[email protected]> Add test case for agent storage utils * Add test case for GetProvider function Signed-off-by: Andrews Arokiam <[email protected]> Add test case for gcs model downloader (kserve#1849) * Add test case for gcs model downloader in agent Signed-off-by: Andrews Arokiam <[email protected]> Add test cases for agent downloader Signed-off-by: Andrews Arokiam <[email protected]> Add test cases for configmap (kserve#1849) * Add test cases for v1beta1 configmap Signed-off-by: Andrews Arokiam <[email protected]> Add test cases for inference service defaults (kserve#1849) * Add test cases for all model runtimes * Add test cases for all runtime defaults Signed-off-by: Andrews Arokiam <[email protected]> Add test cases for predictor model (kserve#1849) * Add test cases for isFrameworkSupported function Signed-off-by: Andrews Arokiam <[email protected]> Add test cases for sklearn predictor (kserve#1849) * Add test cases for GetProtocol function Signed-off-by: Andrews Arokiam <[email protected]> Add test cases for configmap (kserve#1849) * Add test case for creating empty model config Signed-off-by: Andrews Arokiam <[email protected]> Add test cases for utils (kserve#1849) * Add test cases for IncludesArg function * Add test cases for IsGPUEnabled function * Add test cases for FirstNonNilError function * Add test cases for RemoveString function * Add test cases for IsPrefixSupported function Signed-off-by: Andrews Arokiam <[email protected]> * Add test cases for creds_utils (kserve#1849) * Add test cases for set_gcs_credentials function * Add test cases for create_secret function * Add test cases for set_service_account function * Add test cases for create_service_account function * Add test cases for patch_service_account function * Add test cases for get_creds_name_from_config_map function Signed-off-by: Andrews Arokiam <[email protected]> * Add test cases for creds_utils (kserve#1849) * Add test cases for set_gcs_credentials function * Add test cases for create_secret function * Add test cases for set_service_account function * Add test cases for create_service_account function * Add test cases for patch_service_account function * Add test cases for get_creds_name_from_config_map function Signed-off-by: Andrews Arokiam <[email protected]> Add test cases for creds_utils (kserve#1849) * Add test cases for set_s3_credentials function * Add test cases for set_azure_credentials function Signed-off-by: Andrews Arokiam <[email protected]> Add test cases for v1beta1 component (kserve#1849) * Add test cases for validateStorageSpec function * Add test cases for validateLogger function * Add test cases for FirstNonNilComponent function Signed-off-by: Andrews Arokiam <[email protected]> Add test cases for inference service status (kserve#1849) * Add test cases for PropagateRawStatus function * Add test cases for PropagateStatus function Signed-off-by: Andrews Arokiam <[email protected]> Add test cases for predictor (kserve#1849) * Add test cases for GetPredictorImplementations function * Add test cases for GetPredictorImplementation function Signed-off-by: Andrews Arokiam <[email protected]> Add test cases for agent_injector (kserve#1849) * Add test cases for getLoggerConfigs function * Add test cases for getAgentConfigs function Signed-off-by: Andrews Arokiam <[email protected]> Add test cases for batcher_injector (kserve#1849) * Add test cases for getBatcherConfigs function Signed-off-by: Andrews Arokiam <[email protected]> * Add test cases for storage initializer injector (kserve#1849) * Add test cases for getStorageInitializerConfigs function * Add test cases for parsePvcUri function Signed-off-by: Andrews Arokiam <[email protected]> Add test cases for storage initializer injector (kserve#1849) * Add test cases for parsePvcUri function Signed-off-by: Andrews Arokiam <[email protected]> fmt (kserve#1849) Signed-off-by: Andrews Arokiam <[email protected]> Add test cases for controller utils (kserve#1849) * Add test cases for GetDeploymentMode function Signed-off-by: Andrews Arokiam <[email protected]> Remove double import of same package (kserve#1849) Signed-off-by: Andrews Arokiam <[email protected]> temp commit Signed-off-by: Andrews Arokiam <[email protected]> Updated coverage for inference_service_default_test Signed-off-by: Andrews Arokiam <[email protected]> Added scripts for code coverage Signed-off-by: Andrews Arokiam <[email protected]> Updated make to track coverage including subpackages Added more coverage Added ignore to client package - generated code Added coverage script to workflow Signed-off-by: Andrews Arokiam <[email protected]> * Temporarily commenting couple of test cases Signed-off-by: Andrews Arokiam <[email protected]> Temporary changes to debug e2e Signed-off-by: Andrews Arokiam <[email protected]> Commented configmap test Reverted accidental commit of generated code. Signed-off-by: Andrews Arokiam <[email protected]> Added -v to debug failing tests Signed-off-by: Andrews Arokiam <[email protected]> Updated tests to remove dependency on k8s cluster Signed-off-by: Andrews Arokiam <[email protected]> * Updated readme to show coverage
sivanantha321 · Oct 13, 2022 · 4082ec3 · 4082ec3
1 parent aa293a8
commit 4082ec3
Show file tree

Hide file tree

Showing 40 changed files with 6,811 additions and 21 deletions.
diff --git a/.cov-ignore b/.cov-ignore
@@ -0,0 +1,4 @@
+zz_generated.deepcopy.go
+openapi_generated.go
+pkg\\/client
+testing
diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml
@@ -12,7 +12,7 @@ jobs:
     name: Build
     runs-on: ubuntu-latest
     steps:
-
+    
     - name: Set up Go 1.x
       uses: actions/setup-go@v2
       with:
@@ -31,9 +31,27 @@ jobs:
         make fmt
 
     - name: Test
+      id: test
       run: |
         export GOPATH=/home/runner/go
         export PATH=$PATH:/usr/local/kubebuilder/bin:/home/runner/go/bin
         wget -O $GOPATH/bin/yq https://github.com/mikefarah/yq/releases/download/3.3.2/yq_linux_amd64
         chmod +x $GOPATH/bin/yq
         make test
+        ./coverage.sh
+        echo ::set-output name=coverage::$(./coverage.sh | tr -s '\t' | cut -d$'\t' -f 3)
+
+    - name: Print coverage
+      run: |
+        echo "Coverage output is ${{ steps.test.outputs.coverage }}"
+    
+    - name: Update coverage badge
+      if: github.ref == 'refs/heads/master'
+      uses: schneegans/[email protected]
+      with:
+        auth: ${{ secrets.GIST_SECRET }}
+        gistID: 5174bd748ac63a6e4803afea902e9810
+        filename: coverage.json
+        label: coverage
+        message: ${{ steps.test.outputs.coverage }}
+        color: green
diff --git a/Makefile b/Makefile
@@ -41,7 +41,7 @@ all: test manager agent router
 
 # Run tests
 test: fmt vet manifests envtest
-	KUBEBUILDER_ASSETS="$(shell $(ENVTEST) use $(ENVTEST_K8S_VERSION) -p path)" go test $$(go list ./pkg/...) ./cmd/... -coverprofile coverage.out
+	KUBEBUILDER_ASSETS="$(shell $(ENVTEST) use $(ENVTEST_K8S_VERSION) -p path)" go test -v $$(go list ./pkg/...) ./cmd/... -coverprofile coverage.out -coverpkg ./pkg/... ./cmd...
 
 # Build manager binary
 manager: generate fmt vet lint

diff --git a/README.md b/README.md
@@ -1,6 +1,6 @@
 # KServe
 [![go.dev reference](https://img.shields.io/badge/go.dev-reference-007d9c?logo=go&logoColor=white)](https://pkg.go.dev/github.com/kserve/kserve)
-[![Coverage Status](https://coveralls.io/repos/github/kserve/kserve/badge.svg?branch=master)](https://coveralls.io/github/kserve/kserve?branch=master)
+[![Coverage Status](https://img.shields.io/endpoint?url=https://gist.githubusercontent.com/andyi2it/5174bd748ac63a6e4803afea902e9810/raw/coverage.json)](https://github.com/kserve/kserve/actions/workflows/go.yml)
 [![Go Report Card](https://goreportcard.com/badge/github.com/kserve/kserve)](https://goreportcard.com/report/github.com/kserve/kserve)
 [![Releases](https://img.shields.io/github/release-pre/kserve/kserve.svg?sort=semver)](https://github.com/kserve/kserve/releases)
 [![LICENSE](https://img.shields.io/github/license/kserve/kserve.svg)](https://github.com/kserve/kserve/blob/master/LICENSE)

diff --git a/coverage.sh b/coverage.sh
@@ -0,0 +1,10 @@
+#!/bin/sh
+
+while read p || [ -n "$p" ] 
+do
+sed -i "/${p}/d" ./coverage.out 
+done < ./.cov-ignore
+
+go tool cover -func coverage.out > coverage.cov
+
+tail -1 coverage.cov
diff --git a/pkg/agent/downloader_test.go b/pkg/agent/downloader_test.go
@@ -0,0 +1,102 @@
+/*
+Copyright 2022 The KServe Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package agent
+
+import (
+	"io/ioutil"
+	logger "log"
+	"os"
+
+	"github.com/kserve/kserve/pkg/agent/mocks"
+	"github.com/kserve/kserve/pkg/agent/storage"
+	"github.com/kserve/kserve/pkg/apis/serving/v1alpha1"
+	"github.com/kserve/kserve/pkg/modelconfig"
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+	"go.uber.org/zap"
+)
+
+var _ = Describe("Downloader", func() {
+	var modelDir string
+	var downloader *Downloader
+	BeforeEach(func() {
+		dir, err := ioutil.TempDir("", "example")
+		if err != nil {
+			logger.Fatal(err)
+		}
+		modelDir = dir
+		logger.Printf("Creating temp dir %v\n", modelDir)
+		zapLogger, _ := zap.NewProduction()
+		sugar := zapLogger.Sugar()
+		downloader = &Downloader{
+			ModelDir: modelDir + "/test",
+			Providers: map[storage.Protocol]storage.Provider{
+				storage.S3: &storage.S3Provider{
+					Client:     &mocks.MockS3Client{},
+					Downloader: &mocks.MockS3Downloader{},
+				},
+			},
+			Logger: sugar,
+		}
+	})
+	AfterEach(func() {
+		os.RemoveAll(modelDir)
+		logger.Printf("Deleted temp dir %v\n", modelDir)
+	})
+
+	Context("When protocol is invalid", func() {
+		It("Should fail out and return error", func() {
+			modelConfig := modelconfig.ModelConfig{
+				Name: "model1",
+				Spec: v1alpha1.ModelSpec{
+					StorageURI: "sss://models/model1",
+					Framework:  "sklearn",
+				},
+			}
+			err := downloader.DownloadModel(modelConfig.Name, &modelConfig.Spec)
+			Expect(err).ShouldNot(BeNil())
+		})
+	})
+
+	Context("When storage uri is empty", func() {
+		It("Should fail out and return error", func() {
+			modelConfig := modelconfig.ModelConfig{
+				Name: "model1",
+				Spec: v1alpha1.ModelSpec{
+					StorageURI: "",
+					Framework:  "sklearn",
+				},
+			}
+			err := downloader.DownloadModel(modelConfig.Name, &modelConfig.Spec)
+			Expect(err).ShouldNot(BeNil())
+		})
+	})
+
+	Context("When storage uri is invalid", func() {
+		It("Should fail out and return error", func() {
+			modelConfig := modelconfig.ModelConfig{
+				Name: "model1",
+				Spec: v1alpha1.ModelSpec{
+					StorageURI: "s3:://models/model1",
+					Framework:  "sklearn",
+				},
+			}
+			err := downloader.DownloadModel(modelConfig.Name, &modelConfig.Spec)
+			Expect(err).ShouldNot(BeNil())
+		})
+	})
+})
diff --git a/pkg/agent/storage/utils_test.go b/pkg/agent/storage/utils_test.go
@@ -20,9 +20,11 @@ import (
 	"io/ioutil"
 	"os"
 	"path"
+	"path/filepath"
 	"syscall"
 	"testing"
 
+	"github.com/kserve/kserve/pkg/agent/mocks"
 	"github.com/onsi/gomega"
 )
 
@@ -48,3 +50,63 @@ func TestCreate(t *testing.T) {
 	expectedMode := os.FileMode(0777)
 	g.Expect(mode.Perm()).To(gomega.Equal(expectedMode))
 }
+
+func TestFileExists(t *testing.T) {
+	g := gomega.NewGomegaWithT(t)
+	syscall.Umask(0)
+	tmpDir, _ := ioutil.TempDir("", "test")
+	defer os.RemoveAll(tmpDir)
+
+	// Test case for existing file
+	f, err := os.CreateTemp(tmpDir, "tmpfile")
+	g.Expect(err).To(gomega.BeNil())
+	g.Expect(FileExists(f.Name())).To(gomega.BeTrue())
+	f.Close()
+
+	// Test case for not existing file
+	path := filepath.Join(tmpDir, "fileNotExist")
+	g.Expect(FileExists(path)).To(gomega.BeFalse())
+
+	// Test case for directory as filename
+	g.Expect(FileExists(tmpDir)).To(gomega.BeFalse())
+}
+
+func TestRemoveDir(t *testing.T) {
+	g := gomega.NewGomegaWithT(t)
+	syscall.Umask(0)
+	tmpDir, _ := ioutil.TempDir("", "test")
+	subDir, _ := ioutil.TempDir(tmpDir, "test")
+	os.CreateTemp(subDir, "tmp")
+	os.CreateTemp(tmpDir, "tmp")
+
+	err := RemoveDir(tmpDir)
+	g.Expect(err).To(gomega.BeNil())
+	_, err = os.Stat(tmpDir)
+	g.Expect(os.IsNotExist(err)).To(gomega.BeTrue())
+
+	// Test case for non existing directory
+	err = RemoveDir("directoryNotExist")
+	g.Expect(err).NotTo(gomega.BeNil())
+}
+
+func TestGetProvider(t *testing.T) {
+	g := gomega.NewGomegaWithT(t)
+
+	// When providers map already have specified provider
+	mockProviders := map[Protocol]Provider{
+		S3: &S3Provider{
+			Client:     &mocks.MockS3Client{},
+			Downloader: &mocks.MockS3Downloader{},
+		},
+	}
+	provider, err := GetProvider(mockProviders, S3)
+	g.Expect(err).To(gomega.BeNil())
+	g.Expect(provider).Should(gomega.Equal(mockProviders[S3]))
+
+	// When providers map does not have specified provider
+	for _, protocol := range SupportedProtocols {
+		provider, err = GetProvider(map[Protocol]Provider{}, protocol)
+		g.Expect(err).To(gomega.BeNil())
+		g.Expect(provider).ShouldNot(gomega.BeNil())
+	}
+}
diff --git a/pkg/agent/watcher_test.go b/pkg/agent/watcher_test.go
@@ -18,6 +18,7 @@ package agent
 
 import (
 	"context"
+	"encoding/json"
 	"fmt"
 	"io/ioutil"
 	logger "log"
@@ -33,6 +34,7 @@ import (
 	"github.com/kserve/kserve/pkg/agent/mocks"
 	"github.com/kserve/kserve/pkg/agent/storage"
 	"github.com/kserve/kserve/pkg/apis/serving/v1alpha1"
+	"github.com/kserve/kserve/pkg/constants"
 	"github.com/kserve/kserve/pkg/modelconfig"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
@@ -63,7 +65,6 @@ var _ = Describe("Watcher", func() {
 			It("should download and load the new models", func() {
 				defer GinkgoRecover()
 				logger.Printf("Sync model config using temp dir %v\n", modelDir)
-				watcher := NewWatcher("/tmp/configs", modelDir, sugar)
 				modelConfigs := modelconfig.ModelConfigs{
 					{
 						Name: "model1",
@@ -82,7 +83,19 @@ var _ = Describe("Watcher", func() {
 						},
 					},
 				}
-				watcher.parseConfig(modelConfigs, false)
+				_, err := os.Stat("/tmp/configs")
+				if os.IsNotExist(err) {
+					if err := os.MkdirAll("/tmp/configs", os.ModePerm); err != nil {
+						logger.Fatal(err, " Failed to create configs directory")
+					}
+				}
+
+				file, _ := json.MarshalIndent(modelConfigs, "", " ")
+				if err := ioutil.WriteFile("/tmp/configs/"+constants.ModelConfigFileName, file, os.ModePerm); err != nil {
+					logger.Fatal(err, " Failed to write config files")
+				}
+				watcher := NewWatcher("/tmp/configs", modelDir, sugar)
+
 				puller := Puller{
 					channelMap:  make(map[string]*ModelChannel),
 					completions: make(chan *ModelOp, 4),
@@ -100,12 +113,18 @@ var _ = Describe("Watcher", func() {
 					},
 					logger: sugar,
 				}
+				puller.waitGroup.wg.Add(len(watcher.ModelEvents))
 				go puller.processCommands(watcher.ModelEvents)
+
 				Eventually(func() int { return len(puller.channelMap) }).Should(Equal(0))
 				Eventually(func() int { return puller.opStats["model1"][Add] }).Should(Equal(1))
 				Eventually(func() int { return puller.opStats["model2"][Add] }).Should(Equal(1))
 				modelSpecMap, _ := SyncModelDir(modelDir+"/test1", watcher.logger)
 				Expect(watcher.ModelTracker).Should(Equal(modelSpecMap))
+
+				DeferCleanup(func() {
+					os.RemoveAll("/tmp/configs")
+				})
 			})
 		})
 	})

diff --git a/pkg/apis/serving/v1alpha1/inference_graph_validation.go b/pkg/apis/serving/v1alpha1/inference_graph_validation.go
@@ -30,6 +30,18 @@ import (
 const (
 	// InvalidGraphNameFormatError defines the error message for invalid inference graph name
 	InvalidGraphNameFormatError = "The InferenceGraph \"%s\" is invalid: a InferenceGraph name must consist of lower case alphanumeric characters or '-', and must start with alphabetical character. (e.g. \"my-name\" or \"abc-123\", regex used for validation is '%s')"
+	// RootNodeNotFoundError defines the error message for root node not found
+	RootNodeNotFoundError = "root node not found, InferenceGraph needs a node with name 'root' as the root node of the graph"
+	// WeightNotProvidedError defines the error message for traffic weight is nil for inference step
+	WeightNotProvidedError = "InferenceGraph[%s] Node[%s] Route[%s] missing the 'Weight'"
+	// InvalidWeightError defines the error message for sum of traffic weight is not 100
+	InvalidWeightError = "InferenceGraph[%s] Node[%s] splitter node: the sum of traffic weights for all routing targets should be 100"
+	// DuplicateStepNameError defines the error message for more than one step contains same name
+	DuplicateStepNameError = "Node \"%s\" of InferenceGraph \"%s\" contains more than one step with name \"%s\""
+	// TargetNotProvidedError defines the error message for inference graph target not specified
+	TargetNotProvidedError = "Step %d (\"%s\") in node \"%s\" of InferenceGraph \"%s\" does not specify an inference target"
+	// InvalidTargetError defines the error message for inference graph target specifies more than one of nodeName, serviceName, serviceUrl
+	InvalidTargetError = "Step %d (\"%s\") in node \"%s\" of InferenceGraph \"%s\" specifies more than one of nodeName, serviceName, serviceUrl"
 )
 
 const (
@@ -95,7 +107,7 @@ func validateInferenceGraphStepNameUniqueness(ig *InferenceGraph) error {
 		for _, route := range node.Steps {
 			if route.StepName != "" {
 				if nameSet.Has(route.StepName) {
-					return fmt.Errorf("Node \"%s\" of InferenceGraph \"%s\" contains more than one step with name \"%s\"",
+					return fmt.Errorf(DuplicateStepNameError,
 						nodeName, ig.Name, route.StepName)
 				}
 				nameSet.Insert(route.StepName)
@@ -122,12 +134,10 @@ func validateInferenceGraphSingleStepTargets(ig *InferenceGraph) error {
 				count += 1
 			}
 			if count == 0 {
-				return fmt.Errorf("Step %d (\"%s\") in node \"%s\" of InferenceGraph \"%s\" does not specify an inference target",
-					i, route.StepName, nodeName, ig.Name)
+				return fmt.Errorf(TargetNotProvidedError, i, route.StepName, nodeName, ig.Name)
 			}
 			if count != 1 {
-				return fmt.Errorf("Step %d (\"%s\") in node \"%s\" of InferenceGraph \"%s\" specifies more than one of nodeName, serviceName, serviceUrl",
-					i, route.StepName, nodeName, ig.Name)
+				return fmt.Errorf(InvalidTargetError, i, route.StepName, nodeName, ig.Name)
 			}
 		}
 	}
@@ -150,7 +160,7 @@ func validateInferenceGraphRouterRoot(ig *InferenceGraph) error {
 			return nil
 		}
 	}
-	return fmt.Errorf("root node not found, InferenceGraph needs a node with name 'root' as the root node of the graph")
+	return fmt.Errorf(RootNodeNotFoundError)
 }
 
 // Validation of inference graph router type
@@ -161,12 +171,12 @@ func validateInferenceGraphSplitterWeight(ig *InferenceGraph) error {
 		if node.RouterType == Splitter {
 			for _, route := range node.Steps {
 				if route.Weight == nil {
-					return fmt.Errorf("InferenceGraph[%s] Node[%s] Route[%s] missing the 'Weight'", ig.Name, name, route.ServiceName)
+					return fmt.Errorf(WeightNotProvidedError, ig.Name, name, route.ServiceName)
 				}
 				weight += int(*route.Weight)
 			}
 			if weight != 100 {
-				return fmt.Errorf("InferenceGraph[%s] Node[%s] splitter node: the sum of traffic weights for all routing targets should be 100", ig.Name, name)
+				return fmt.Errorf(InvalidWeightError, ig.Name, name)
 			}
 		}
 	}