Operator SDK Test Framework #377

AlexNPavel · 2018-08-01T22:04:38Z

The PR is for the initial version of the Operator SDK test framework which can be used to test operators created with the SDK.

This library is based on the SDK's internal e2e testing framework and has several modifications to make it more applicable for use to test operators (whereas the SDK's tests test SDK functionality, not operators).

fanminshi · 2018-08-02T18:35:11Z

commands/operator-sdk/cmd/test.go

+	dc.Stderr = os.Stderr
+	wd, err := os.Getwd()
+	if err != nil {
+		cmdError.ExitWithError(cmdError.ExitError, fmt.Errorf("Could not determine working directory"))


cmdError.ExitWithError(cmdError.ExitError, fmt.Errorf("Could not determine working directory")) ->
cmdError.ExitWithError(cmdError.ExitError, fmt.Errorf("could not determine working directory: %v", err))

I noticed that we have mixed capitalization of the starting word of a error message. I'd suggest to keep it lower case. And we should have another pr to loop through all the code to make the error message to start with lower case.

fanminshi · 2018-08-02T18:42:06Z

pkg/test/framework.go

+}
+
+func setup() error {
+	kubeconfigEnv, ok := os.LookupEnv("TEST_KUBECONFIG")


I think we should make "TEST_KUBECONFIG", "TEST_NAMESPACE", and etc into constants and save those under pkg/test/constant.go.

fanminshi · 2018-08-02T18:48:41Z

pkg/test/framework.go

+	}
+	kubeconfig, err := clientcmd.BuildConfigFromFlags("", kubeconfigEnv)
+	if err != nil {
+		return err


suggest to be more explicit what's the error is: e.g
return fmt.Errof("failed to build the kubeconfig: %v", err)

fanminshi · 2018-08-02T18:49:30Z

pkg/test/framework.go

+	}
+	kubeclient, err := kubernetes.NewForConfig(kubeconfig)
+	if err != nil {
+		return err


return fmt.Errof("failed to build the kubeclient: %v", err)

fanminshi · 2018-08-02T18:50:08Z

pkg/test/framework.go

+	}
+	extensionsClient, err := extensions.NewForConfig(kubeconfig)
+	if err != nil {
+		return err


return fmt.Errof("failed to build the extensionsClient: %v", err)

fanminshi · 2018-08-02T18:58:33Z

commands/operator-sdk/cmd/test.go

+	}
+	testCmd.Flags().StringVarP(&testLocation, "test-location", "t", "", "Location of test files (e.g. ./test/e2e/)")
+	testCmd.MarkFlagRequired("test-location")
+	testCmd.Flags().StringVarP(&kubeconfig, "kubeconfig", "k", defaultKubeConfig, "Kubeconfig path")


it seems to that most of the flags are required as indicated in Framework.setup() down below. Maybe we should mark those as required?

The flags that are not marked as required have defaults that are used instead.

yeah, you are right.

fanminshi · 2018-08-02T18:59:00Z

pkg/test/main_entry.go

+
+func MainEntry(m *testing.M) {
+	// go test always runs from the test directory; change to project root
+	root, ok := os.LookupEnv("TEST_PROJROOT")


use constant.go

fanminshi · 2018-08-02T19:00:27Z

pkg/test/main_entry.go

+	// go test always runs from the test directory; change to project root
+	root, ok := os.LookupEnv("TEST_PROJROOT")
+	if !ok {
+		log.Fatalf("Failed to get project root dir environment variable")


log.Fatalf -> log.Fatal? and Failed -> failed?

fanminshi · 2018-08-02T19:00:45Z

pkg/test/main_entry.go

+	}
+	os.Chdir(root)
+	if err := setup(); err != nil {
+		log.Fatalf("Failed to set up framework: %v", err)


Failed -> failed?

fanminshi · 2018-08-02T19:01:41Z

pkg/util/e2eutil/wait_util.go

+
+var retryInterval = time.Second * 5
+
+func WaitForDeployment(t *testing.T, kubeclient kubernetes.Interface, namespace, name string, replicas, retries int) error {


doc string?

shawn-hurley

I think that this will work with the ansible operator as well

/cc @fabianvf @mhrivnak

shawn-hurley · 2018-08-03T16:23:57Z

commands/operator-sdk/cmd/test.go

+		cmdError.ExitWithError(cmdError.ExitError, fmt.Errorf("could not determine working directory: %v", err))
+	}
+	dc.Env = append(os.Environ(),
+		fmt.Sprintf("%v=%v", "TEST_KUBECONFIG", kubeconfig),


If you make these values constants I think you may want to use them here

shawn-hurley · 2018-08-03T17:15:23Z

pkg/test/resource_creator.go

+	groupVersion := strings.Split(yamlMap["apiVersion"].(string), "/")
+	crGV := schema.GroupVersion{Group: groupVersion[0], Version: groupVersion[1]}
+	crConfig.GroupVersion = &crGV
+	crConfig.APIPath = "/apis"


Do we need to handle the case when the Group is "". I believe that means the APIPath should be /api.

Custom resources should not be part of the core group (or group ""), so we shouldn't need to worry about that case. The GetCRClient function is only for custom resources.

At the moment, that function gets called as a catch-all if it's something we did not specify in the CreateFromYAML function, so that could occur. I plan to support all resources in that function soon though.

hasbro17 · 2018-08-06T20:15:00Z

commands/operator-sdk/cmd/test.go

+
+func NewTestCmd() *cobra.Command {
+	testCmd := &cobra.Command{
+		Use:   "test --kubeconfig <path to kubeconfig> --image <name of operator image>",


We don't need the operator --image flag anymore right?
Since we just use the input manifest to deploy the operator.

hasbro17 · 2018-08-06T21:18:44Z

pkg/test/main_entry.go

+)
+
+func MainEntry(m *testing.M) {
+	projRoot := flag.String("root", "", "path to project root")


We should probably declare the flag names as constants here and then import them in commands/operator-sdk/cmd/test.go so we don't have to change them in two places.

hasbro17 · 2018-08-06T21:20:04Z

pkg/test/main_entry.go

+	rbacManPath := flag.String("rbac", "", "path to rbac manifest")
+	flag.Parse()
+	// go test always runs from the test directory; change to project root
+	os.Chdir(*projRoot)


Check the returned error.

hasbro17 · 2018-08-06T21:22:16Z

pkg/test/resource_creator.go

+)
+
+var (
+	filemode = int(0664)


This isn't being used anywhere.

hasbro17 · 2018-08-06T21:33:53Z

pkg/test/main_entry.go

+func MainEntry(m *testing.M) {
+	projRoot := flag.String("root", "", "path to project root")
+	kubeconfigPath := flag.String("kubeconfig", "", "path to kubeconfig")
+	namespace := flag.String("namespace", "", "namespace to tests to run in")


namespace to tests to run in ==> namespace where the tests will run

AlexNPavel · 2018-08-06T22:40:17Z

I will be moving the namespace and other global resources into the framework initialization

AlexNPavel · 2018-08-06T22:58:29Z

We're moving all tests into their own namespaces again

GetCRClient was not storing the CRClient it creates in ctx

Cobra prints the defaults in the help message for you

Was using github url instead of sig.k8s.io url

AlexNPavel · 2018-08-07T21:55:43Z

We are now using the controller-runtime's dynamic client to handle creation and deletion of all resources except for custom resources

fanminshi · 2018-08-07T22:07:45Z

commands/operator-sdk/cmd/test.go

+}
+
+func testFunc(cmd *cobra.Command, args []string) {
+	wd, err := os.Getwd()


we probably can use mustGetwd from https://github.com/operator-framework/operator-sdk/blob/master/commands/operator-sdk/cmd/new.go#L148 here.

fanminshi · 2018-08-07T22:08:50Z

commands/operator-sdk/cmd/test.go

+	if err != nil {
+		cmdError.ExitWithError(cmdError.ExitError, fmt.Errorf("could not determine working directory: %v", err))
+	}
+	testArgs := []string{"test", testLocation + "/..."}


we probably can use execCmd from https://github.com/operator-framework/operator-sdk/blob/master/commands/operator-sdk/cmd/new.go#L156 to help us executing external command.

fanminshi · 2018-08-07T22:09:37Z

pkg/test/context.go

+// GetObjID returns an ascending ID based on the length of cleanUpFns. It is
+// based on the premise that every new object also appends a new finalizerFn on
+// cleanUpFns.
+func (ctx *TestCtx) GetObjID() string {


is this being used anywhere? if not, we probably don't need it.

fanminshi · 2018-08-07T22:17:04Z

pkg/test/context.go

+
+type finalizerFn func() error
+
+func (f *Framework) NewTestCtx(t *testing.T) TestCtx {


is there a reason to tie NewTestCtx to Framework object? it seems me that NewTestCtx can just be a static factory function.

fanminshi · 2018-08-07T22:32:11Z

pkg/test/context.go

+	return ctx.ID + "-" + strconv.Itoa(len(ctx.CleanUpFns))
+}
+
+func (ctx *TestCtx) Cleanup(t *testing.T) {


Can you have make Cleanup to take in f func(format string, v ...interface{})?
then you can pass t.Errorf or log.Fatalf to it. So that we don't have to have a CleanupNoT function.

e.g:

func (ctx *TestCtx) Cleanup(f func(format string, v ...interface{})) { for i := len(ctx.CleanUpFns) - 1; i >= 0; i-- { err := ctx.CleanUpFns[i]() if err != nil { f("a cleanup function failed with error: %v\n", err) } } } func TestBla(t *testing.T) { ctx := Global.NewTestCtx(nil) defer ctx.Cleanup(log.Fatalf) ctx = Global.NewTestCtx(t) defer ctx.Cleanup(t.Errof) }

@AlexNPavel any thought to the above approach?

I like it. What I'm not sure about is how to handle failure. We don't want to use log.Fatalf as our error reporting function, because that could cause the function to end before all clean up functions have finished. The way that CleanupNoT handles this is with a failed boolean that only runs log.Fatalf after all functions have been executed. We could use that design again, but there's still the issue about what happens if a user give us something like log.Fatalf. We could document the behavior, but it would still be easy for people to make this mistake.

I see. Let's just keep it is at the moment. we can revisit this later.

ironcladlou · 2018-08-08T18:08:47Z

commands/operator-sdk/cmd/test.go

+func testFunc(cmd *cobra.Command, args []string) {
+	testArgs := []string{"test", testLocation + "/..."}
+	if verbose {
+		testArgs = append(testArgs, "-v")


Consider allowing arbitrary passthrough of test flags, perhaps following the typical convention (e.g. go test itself) of "everything after [--|-args] is passed through".

I feel like that would be quite difficult to do; I'm currently thinking of ways to implement that in a simple manner. The way go test does it is quite complicated and they don't use the flag package for go test, instead opting for separate code: https://github.com/golang/go/blob/master/src/cmd/go/internal/test/testflag.go#L21

As long as the test framework is implemented as a library and doesn't impose a dependency on this wrapper command (which I THINK is true) then maybe it's not a big deal. For example, it looks like I should be able to do this if I wanted:

go test <go-test-args...> ./... -args <framework-args...>

Which would be fine for me in special/advanced cases. That would also be consistent with the way the build/run cycle of the SDK currently works for me (e.g. I compile with the standard go build toolchain during iteration, use the SDK to do iterative testing, and sometimes run my binary without the SDK in very special cases).

I'll add that as a TODO for now. It would be nice to be able to do it with cobra, but if that's too complicated, we may just implement it as a string that the user passes in (e.g. operator-sdk test -t ./test/e2e/ --passthrough "-v -parallel=2".

Sure... it may be useful to help me understand the design goals. For example, if a goal is to provide a simple wrapper which works for most cases but consistently provides an escape hatch for advanced uses (via the standard Go toolchain) then just keeping what you have here seems reasonable to me. If the wrapper is intended to be the only reasonable way to use the test framework, then things get a little trickier in deciding how much of the standard toolchain to re-expose, and how.

@ironcladlou It's the former. The wrapper is meant to be a simple way for users to run their e2e tests in an SDK based project. I don't think there's any restriction for it to be the only way.
For more advanced use cases go test could be used.

ironcladlou · 2018-08-08T18:21:56Z

pkg/util/e2eutil/wait_util.go

@@ -0,0 +1,54 @@
+// Copyright 2018 The Operator-SDK Authors


Not sure why the apimachinery utils for waiting/polling would be insufficient here, and using them would reduce the scope of changes/maintenance.

@ironcladlou could you link those utils. I wasn't aware of those.

Sure- check out the wait package.

hasbro17 · 2018-08-08T23:34:31Z

pkg/util/e2eutil/wait_util.go

+// If the deployment does not have the required number of replicas after 5 * retries seconds, the function returns an error
+// This can be used in multiple ways, like verifying that a required resource is ready before trying to use it, or to test
+// failure handling, like simulated in SimulatePodFail.
+func WaitForDeployment(t *testing.T, kubeclient kubernetes.Interface, namespace, name string, replicas, retries int) error {


Let's just pass in the retry interval and total timeout and keep it the same as wait.Poll() expects it. Also that way we don't need to set the global vars for retryInterval and retries on lines 28-29

In addition to taking the very good advice to make this stateless, I'd also recommend just keeping it a private function of the example test file until enough copypasta is observed in the wild to justify building up a util package (which tend to get gorpy very fast).

hasbro17 · 2018-08-09T17:57:26Z

LGTM

ironcladlou · 2018-08-09T18:00:10Z

Should have squashed this

hasbro17 · 2018-08-09T18:02:47Z

@ironcladlou We used the squash-and-merge button on github to squash it here instead of doing it locally. So it'll just be one commit.
b19e56f

ironcladlou · 2018-08-09T18:04:22Z

@hasbro17 oh, nice! Didn't know about that- thanks!

AlexNPavel requested review from fanminshi and hasbro17 August 1, 2018 22:04

fanminshi reviewed Aug 2, 2018

View reviewed changes

AlexNPavel force-pushed the pkg-test branch 2 times, most recently from 6086ce9 to ce9e21b Compare August 3, 2018 17:20

shawn-hurley reviewed Aug 3, 2018

View reviewed changes

AlexNPavel changed the title ~~[WIP] Operator SDK Test Framework~~ Operator SDK Test Framework Aug 6, 2018

hasbro17 reviewed Aug 6, 2018

View reviewed changes

pkg/test/resource_creator.go Outdated

)

var (

filemode = int(0664)

Copy link

Contributor

hasbro17 Aug 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't being used anywhere.

hasbro17 reviewed Aug 6, 2018

View reviewed changes

AlexNPavel changed the title ~~Operator SDK Test Framework~~ [WIP] Operator SDK Test Framework Aug 6, 2018

AlexNPavel added 15 commits August 7, 2018 10:15

pkg/test: add initial e2e framework and operator-sdk test subcommand

e589d19

pkg/test/resource_creator.go: create InitializeClusterResources function

4b455f4

pkg/test,commands/.../test.go: make namespace settable and global

9c32234

pkg/test: add license header to files

f6524e6

pkg/util/e2eutil: change DeploymentReplicaCheck to WaitForDeployment

7f13e2b

pkg/util/e2eutil: rename check_util to wait_util

e0d06ba

commands/operator-sdk/cmd/test.go: add cli args for manifest paths

267bed5

pkg/test: handle new cli args

476f951

pkg/test/resource_creator.go: add UpdateCR

6f85f67

pkg/test/resource_creator.go: fix bug in GetCRClient

3c0c94c

GetCRClient was not storing the CRClient it creates in ctx

test/test-framework: add tests for the test framework

438ddeb

pkg/test,commands/.../test.go: add project root env var

6198721

pkg/test/framework.go: use !ok instead of ok != true

0d83f5b

commands/.../test.go: don't specify flag defaults

9b174ba

Cobra prints the defaults in the help message for you

pkg/test/resource_creator.go: delete just the necessary CR, not all

2ce24f7

AlexNPavel added 5 commits August 7, 2018 10:15

pkg/test,commands/.../test.go: remove namespace flags

64b431a

commands/.../test.go: remove namespace variable

ff4a96f

pkg/test/resource_creator.go: return error if namespace already exists

2cbf475

pkg/test/framework.go: add dynamic client from controller-runtime

46300e7

pkg/test: add dynamic client using controller-runtime

145e6a2

AlexNPavel force-pushed the pkg-test branch from e4b3188 to 145e6a2 Compare August 7, 2018 17:19

AlexNPavel changed the title ~~[WIP] Operator SDK Test Framework~~ Operator SDK Test Framework Aug 7, 2018

AlexNPavel added 5 commits August 7, 2018 10:34

Gopkg.lock: correct controller-runtime revision in lock

b3ecd38

Gpokg.*: change controller-runtime name

491f339

Was using github url instead of sig.k8s.io url

pkg/test/resource_creator.go: support multispec files

1cc1a6c

pkg/test/resource_creator.go: add finalizer funcs in CreateFromYAML

61e7943

pkg/test/resource_creator.go: add TODO

5b85f1e

fanminshi reviewed Aug 7, 2018

View reviewed changes

AlexNPavel added 2 commits August 7, 2018 15:52

commands/.../test.go: simplify testFunc with existing helper funcs

2c8d7f1

pkg/test/context.go: remove unused function

0a930dd

hasbro17 mentioned this pull request Aug 7, 2018

SDK provides a test-framework library to let operators run e2e tests #387

Closed

pkg/test: separate NewTestCtx from the framework struct

90b8170

ironcladlou reviewed Aug 8, 2018

View reviewed changes

AlexNPavel added 2 commits August 8, 2018 13:41

commands/.../test.go: add todo

b74f40c

pkg/util/e2eutil: use apimachinery's wail util

a792207

hasbro17 reviewed Aug 8, 2018

View reviewed changes

pkg/util/e2eutil: make wait_util stateless

3066a44

AlexNPavel merged commit b19e56f into operator-framework:master Aug 9, 2018

AlexNPavel deleted the pkg-test branch August 9, 2018 17:59


		var retryInterval = time.Second * 5

		func WaitForDeployment(t *testing.T, kubeclient kubernetes.Interface, namespace, name string, replicas, retries int) error {


		type finalizerFn func() error

		func (f Framework) NewTestCtx(t testing.T) TestCtx {

Operator SDK Test Framework #377

Operator SDK Test Framework #377

Conversation

AlexNPavel commented Aug 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shawn-hurley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexNPavel commented Aug 6, 2018

AlexNPavel commented Aug 6, 2018

AlexNPavel commented Aug 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fanminshi Aug 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hasbro17 commented Aug 9, 2018

ironcladlou commented Aug 9, 2018

hasbro17 commented Aug 9, 2018

ironcladlou commented Aug 9, 2018

fanminshi Aug 7, 2018 •

edited

Loading