initial commit for tests #50

lina-is-here · 2019-11-18T14:45:58Z

Output locally

$ go test ./test/integration
ok      github.com/openshift/insights-operator/test/integration 10.722s

openshift-ci-robot · 2019-11-18T14:46:13Z

Hi @lina-nikiforova. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

tisnik

@lina-nikiforova it looks very good so far, thank you a lot! I just added some nitpicks, we might discuss about them f2f

tisnik · 2019-11-21T19:27:33Z

test/integration/bugs_test.go

+	err := kubeClient.CoreV1().Secrets("openshift-config").Delete("support", &metav1.DeleteOptions{})
+
+	// if the secret is not found, continue, not a problem
+	if (err != nil) && (err.Error() != "secrets \"support\" not found") {


!= operator has higher precedence that &&, so strictly speaking parenthesis are not needed there

(https://golang.org/ref/spec#Operators)

you can write `secrets "support" not found` instead of escaping quote (but it depends what you prefer)

tisnik · 2019-11-21T19:29:28Z

test/integration/bugs_test.go

+	// restart insights-operator (delete pods)
+	pods, err := kubeClient.CoreV1().Pods("openshift-insights").List(metav1.ListOptions{})
+	if err != nil {
+		panic(err.Error())


t.Fatal(err.Error())

tisnik · 2019-11-21T19:29:49Z

test/integration/bugs_test.go

+
+	// if the secret is not found, continue, not a problem
+	if (err != nil) && (err.Error() != "secrets \"support\" not found") {
+		panic(err.Error())


t.Fatal(err.Error())

tisnik · 2019-11-21T19:31:13Z

test/integration/bugs_test.go

+	// check logs for "Gathering cluster info every 2h0m0s"
+	newPods, err := kubeClient.CoreV1().Pods("openshift-insights").List(metav1.ListOptions{})
+	if err != nil {
+		panic(err.Error())


t.Fatal(err.Error())

tisnik · 2019-11-21T19:40:30Z

test/integration/bugs_test.go

+		log := buf.String()
+
+		result := strings.Contains(log, "Gathering cluster info every 2h0m0s")
+		podLogs.Close()


I'd suggest to use defer podLogs.Close() on line 63 or so

I had it there but somehow got errors when I used it... I'll try to look into it again

now it seems to work, I was missing handling of one of the errors :)

tisnik · 2019-11-21T19:41:58Z

test/integration/main_test.go

+)
+
+func KubeClient() (result *kubernetes.Clientset) {
+	kubeconfig := os.Getenv("KUBECONFIG") // variable is a path to the local kubeconfig


would be good to test whether the $KUBECONFIG exists at all

done using os.LookupEnv

tisnik · 2019-11-21T19:43:32Z

test/integration/bugs_test.go

+			_, err := kubeClient.CoreV1().Pods("openshift-insights").Get(pod.Name, metav1.GetOptions{})
+			if err == nil {
+				fmt.Printf("the pod is not yet deleted: %v\n", err)
+				return false, nil


I'm not sure there - should not it be rather:

return false, err?

mfojtik · 2019-11-25T12:23:49Z

/ok-to-test

quarckster · 2019-12-09T11:10:19Z

/retest

quarckster · 2019-12-10T11:27:45Z

@mfojtik, please put a required label if you think this PR is fine.

mfojtik · 2019-12-11T09:45:48Z

/lgtm

openshift-ci-robot · 2019-12-11T09:46:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lina-nikiforova, mfojtik

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [mfojtik]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 18, 2019

openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 18, 2019

openshift-ci-robot requested review from derekwaynecarr and smarterclayton November 18, 2019 14:46

lina-is-here force-pushed the tests branch 3 times, most recently from 32da4ce to 4d5045e Compare November 19, 2019 12:20

quarckster mentioned this pull request Nov 19, 2019

Added e2e tests for insights-operator openshift/release#5996

Merged

lina-is-here force-pushed the tests branch 3 times, most recently from 14a27f7 to a9fcf55 Compare November 19, 2019 15:33

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 19, 2019

lina-is-here force-pushed the tests branch 4 times, most recently from 9035b30 to 7e79f42 Compare November 20, 2019 13:53

lina-is-here changed the title ~~WIP: initial commit for tests~~ initial commit for tests Nov 20, 2019

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 20, 2019

tisnik reviewed Nov 21, 2019

View reviewed changes

lina-is-here force-pushed the tests branch from 7e79f42 to 9b45cd9 Compare November 22, 2019 09:37

openshift-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 25, 2019

lina-is-here force-pushed the tests branch from 9b45cd9 to f23813e Compare November 25, 2019 14:50

initial commit for tests

1c341e6

lina-is-here force-pushed the tests branch 2 times, most recently from 9824ccd to ce14464 Compare November 26, 2019 11:40

removed unused var

5c7e813

lina-is-here force-pushed the tests branch from ce14464 to 5c7e813 Compare November 26, 2019 13:17

This was referenced Nov 29, 2019

Bug 1798889: initial commit for tests for release 4.2 #57

Merged

Bug 1788118: initial commit for tests for release 4.3 #58

Merged

added wait for the pods to be running

4a5e63d

openshift-ci-robot assigned mfojtik Dec 11, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 11, 2019

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 11, 2019

openshift-merge-robot merged commit ed5af49 into openshift:master Dec 11, 2019

lina-is-here deleted the tests branch December 11, 2019 11:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

initial commit for tests #50

initial commit for tests #50

lina-is-here commented Nov 18, 2019 •

edited

openshift-ci-robot commented Nov 18, 2019

tisnik left a comment

tisnik Nov 21, 2019

tisnik Nov 21, 2019

lina-is-here Nov 22, 2019

tisnik Nov 21, 2019

tisnik Nov 21, 2019

tisnik Nov 21, 2019

tisnik Nov 21, 2019

lina-is-here Nov 22, 2019

lina-is-here Nov 25, 2019

tisnik Nov 21, 2019

lina-is-here Nov 25, 2019

tisnik Nov 21, 2019

mfojtik commented Nov 25, 2019

quarckster commented Dec 9, 2019

quarckster commented Dec 10, 2019

mfojtik commented Dec 11, 2019

openshift-ci-robot commented Dec 11, 2019

initial commit for tests #50

initial commit for tests #50

Conversation

lina-is-here commented Nov 18, 2019 • edited

openshift-ci-robot commented Nov 18, 2019

tisnik 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfojtik commented Nov 25, 2019

quarckster commented Dec 9, 2019

quarckster commented Dec 10, 2019

mfojtik commented Dec 11, 2019

openshift-ci-robot commented Dec 11, 2019

lina-is-here commented Nov 18, 2019 •

edited