cleanup go e2e tests and perform improvements by camilamacedo86 · Pull Request #3499 · operator-framework/operator-sdk

camilamacedo86 · 2020-07-23T12:06:20Z

Description of the change:

Update the go e2e test to have the suffix _test which has special meaning for the Go compiler.
Cleanup the test in order to make all following the same standard
Ensure that the test are done using the makefile targets scaffolds as we suggest in our docs.
Add tests to ensure the metrics, run the project locally and the bundle sdk features
fix the target in the doc https://master.sdk.operatorframework.io/docs/olm-integration/generation/#package-manifests-format. Users should use $(KUSTOMIZE) instead of kustomize directly.
Fix Prometheus typo issue

Motivation for the change:

maintainability, readability and reusability
ensure that our e2e tests are using the targets commands as we suggested to the users in the docs

NOTE: The first goal is to keep all tests following the same standard. However, it still has a hall for improvements that ought to make horizontally in follow-ups.

estroz

I’m not so sure we want to do this, at least not yet. Go e2e tests, while they could test more functionality, are fine as-is for v1.0. We can prioritize improving them after release.

/hold

camilamacedo86 · 2020-07-23T13:39:34Z

Hi @estroz,

I’m not so sure we want to do this, at least not yet. Go e2e tests, while they could test more functionality, are fine as-is for v1.0. We can prioritize improving them after release.

I do not agree with first because I do not see why it affects the v1.0 release. Also, we are applying here the changes/discussions made in the helm and then, I do not think that the tests are fine now because they are not using the makefile as we suggest to in the docs and we are unable to run them locally with any env for example.

camilamacedo86 · 2020-07-23T17:16:52Z

@joelanford wdyt? Can we move forward with this one?

joelanford · 2020-07-23T17:30:01Z

fix the target in the doc master.sdk.operatorframework.io/docs/olm-integration/generation/#package-manifests-format. Users should use $(KUSTOMIZE) instead of kustomize directly.

This is the only thing from this PR that I think would be better to get in before 1.0. Everything else is nice to have, but can merge before 1.0 (if we get time to review, etc.)

I agree with @estroz that 1.0 blockers are the priority at the moment.

camilamacedo86 · 2020-07-23T18:46:15Z

Removing the /hold since has I understand according to above comments that have no reason to hold that besides we are not prioritizing it for 1.0.0.

/hold cancel

jberkhahn · 2020-08-18T23:36:20Z

+	close(done)
+}, 360)
+
+// AfterSuite run after all the specs have run, regardless of whether any tests have failed to ensures that


Nice use of testing suite.

jmrodri

/lgtm

joelanford · 2020-09-14T16:13:15Z

+
+# Generate package manifests.
+packagemanifests: kustomize
+	operator-sdk generate kustomize manifests -q --interactive=false


Why do we specify --interactive=false here, but not for Go?

Because, the packagemanifests will calls interactive mode:

$ make packagemanifests operator-sdk generate kustomize manifests -q Display name for the operator (required): >

joelanford

One more round now that I had more time to look at it.

Looks great overall, but a I have a few more comments and suggestions.

joelanford · 2020-09-15T03:00:33Z

+			By("running custom scorecard tests")
+			runScorecardCmd = exec.Command(tc.BinaryName, "scorecard", "bundle",
+				"--selector=suite=custom",
+				"--output=json",
+				"--wait-time=40s")
+			scorecardOutputBytes, err = tc.Run(runScorecardCmd)
+			Expect(err).NotTo(HaveOccurred())
+			err = json.Unmarshal(scorecardOutputBytes, &scorecardOutput)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(len(scorecardOutput.Items)).To(Equal(2))


We talked about not running these tests as part of the Go e2e. Can we remove from here and do a follow-up to add them to the test/integration tests?

I think we need to discuss that before to do a follow. These tests were added via tasks planned in the sprint which means that was discussed before to get done. Also, I think we should keep them at least for now. See that when we split the repos will make sense we have only the e2e tests to ensure the SDK features ( scorecard and olm ) on this repo.

Also, see that the test/integration is not following the standards define so far and with them, we will NOT test it end to end and then, we will not able to ensure that the commands work with the project layout and with the makefile targets suggested in the docs.

In this way, before we decide to change the scorecard tests I think we need to discuss how the tests will be after the slit as what is the best approach for we do not spend time changing it many times.

See; https://github.com/operator-framework/enhancements/pull/44/files#diff-0af5b52afc5f9db2b08e97937d991b40R233

Ah I see now that this is just copied over from the existing file. Seems reasonable to keep for now, but we should add a TODO comment to split the scorecard custom test testing out.

We should also discuss if there's really a need for the custom test example at all. After all, the basic and olm tests are probably reasonable examples on their own.

https://github.com/operator-framework/operator-sdk/blob/master/test/e2e-helm/e2e_helm_suite_test.go#L117-L121

I will push soon the PR to split the scorecard.

We should also discuss if there's really a need for the custom test example at all. After all, the basic and olm tests are probably reasonable examples on their own.

we have a task/issue for that already: #3839

joelanford · 2020-09-15T03:22:33Z

+		makefilePackagemanifestsFragment = strings.Replace(makefilePackagemanifestsFragment,
+			"packagemanifests: kustomize manifests", "packagemanifests: kustomize", 1)


This is brittle. If we change the packagemanifests target to something slightly different, this replacement will silently fail.

Instead, what if we make the fragment a format string that we pass to fmt.Sprintf(). e.g.

makefilePackagemanifestsFragment := ` # Options for "packagemanifests". ifneq ($(origin FROM_VERSION), undefined) PKG_FROM_VERSION := --from-version=$(FROM_VERSION) endif ifneq ($(origin CHANNEL), undefined) PKG_CHANNELS := --channel=$(CHANNEL) endif ifeq ($(IS_CHANNEL_DEFAULT), 1) PKG_IS_DEFAULT_CHANNEL := --default-channel endif PKG_MAN_OPTS ?= $(FROM_VERSION) $(PKG_CHANNELS) $(PKG_IS_DEFAULT_CHANNEL) # Generate package manifests. packagemanifests: %s operator-sdk generate kustomize manifests -q --interactive=false cd config/manager && $(KUSTOMIZE) edit set image controller=$(IMG) $(KUSTOMIZE) build config/manifests | operator-sdk generate packagemanifests -q --version $(VERSION) $(PKG_MAN_OPTS) ` pmDeps := []string{"kustomize"} if strings.HasPrefix(c.Layout, "go") { pmDeps = append(pmDeps, "manifests") } fragment := fmt.Sprintf(makefilePackagemanifestsFragment, strings.Join(pmDeps, " "))

It is good always check. You are right here. Done.

joelanford · 2020-09-15T14:05:18Z

+		manifestsTarget := "packagemanifests: kustomize manifests"
+		if !strings.Contains(makefilePackagemanifestsFragment, manifestsTarget) {
+			return errors.New("unable to find the manifests target to be replaced")
+		}
 		makefilePackagemanifestsFragment = strings.Replace(makefilePackagemanifestsFragment,
-			"packagemanifests: kustomize manifests", "packagemanifests: kustomize", 1)
+			manifestsTarget, "packagemanifests: kustomize", 1)


Nit: string comparison and replacement seems more complex than the fmt.Sprintf idea, and it is still prone to accidental breakage. At least now the test would fail, but it seems more efficient and less error prone to build the string instead of search/replace it.

With this, it is possible that we'll make a change and not see that we accidentally broke something until 20-30 minutes into a CI run.

joelanford

Looks good overall. One more nit about the packagemanifests templating.

Otherwise
/lgtm

openshift-ci-robot · 2020-09-15T15:41:58Z

New changes are detected. LGTM label has been removed.

**Description of the change:** - only move the scorecard tests to e2e_<type>_scorecard_test **Motivation for the change:** - (+) maintainable and readable. - #3499 (comment)

openshift-ci-robot requested review from fabianvf and shawn-hurley July 23, 2020 12:06

camilamacedo86 mentioned this pull request Jul 23, 2020

Allows OLM subcommands work for Helm/Ansible new layout #3341

Merged

camilamacedo86 requested review from estroz, jmccormick2001, jmrodri and joelanford and removed request for shawn-hurley July 23, 2020 12:45

estroz reviewed Jul 23, 2020

View reviewed changes

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 23, 2020

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 23, 2020

camilamacedo86 changed the title ~~feat: cleanup go e2e tests and perform improvements~~ WIP: feat: cleanup go e2e tests and perform improvements Jul 24, 2020

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 24, 2020

camilamacedo86 changed the title ~~WIP: feat: cleanup go e2e tests and perform improvements~~ feat: cleanup go e2e tests and perform improvements Jul 28, 2020

camilamacedo86 removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 28, 2020

camilamacedo86 requested a review from estroz July 28, 2020 11:06

estroz reviewed Jul 28, 2020

View reviewed changes

Comment thread hack/tests/e2e-go.sh Outdated

openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 2, 2020

camilamacedo86 requested a review from estroz August 2, 2020 11:54

camilamacedo86 mentioned this pull request Aug 7, 2020

Go tests should cover the features with apis scaffolding using v1 version #3680

Closed

openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 9, 2020

camilamacedo86 commented Aug 11, 2020

View reviewed changes

Comment thread test/e2e/e2e_go_olm_test.go Outdated

jberkhahn reviewed Aug 18, 2020

View reviewed changes

Comment thread test/e2e/e2e_go_cluster_test.go

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 9, 2020

camilamacedo86 changed the title ~~WIP: feat: cleanup go e2e tests and perform improvements~~ WIP: feat: cleanup go e2e tests and perform improvements (need to update with master) Sep 9, 2020

camilamacedo86 changed the title ~~WIP: feat: cleanup go e2e tests and perform improvements (need to update with master)~~ cleanup go e2e tests and perform improvements (need to update with master) Sep 10, 2020

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 Sep 10, 2020

camilamacedo86 changed the title ~~cleanup go e2e tests and perform improvements (need to update with master)~~ cleanup go e2e tests and perform improvements Sep 10, 2020

camilamacedo86 requested a review from jberkhahn September 10, 2020 12:50

jmrodri approved these changes Sep 13, 2020

View reviewed changes

openshift-ci-robot assigned jmrodri Sep 13, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 13, 2020

asmacdo approved these changes Sep 14, 2020

View reviewed changes

Comment thread test/internal/packagemanifests.go Outdated

Comment thread test/internal/packagemanifests.go Outdated

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 14, 2020

joelanford requested changes Sep 14, 2020

View reviewed changes

estroz removed the ci label Sep 14, 2020

joelanford requested changes Sep 15, 2020

View reviewed changes

camilamacedo86 mentioned this pull request Sep 15, 2020

Cleanup after test e2e funcs for Ansible/Helm/Go #3878

Closed

joelanford reviewed Sep 15, 2020

View reviewed changes

joelanford approved these changes Sep 15, 2020

View reviewed changes

openshift-ci-robot assigned joelanford Sep 15, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 15, 2020

camilamacedo86 added 4 commits September 15, 2020 16:33

feat: cleanup go e2e tests and perform improvements

7c3e38d

improve pkg mock target

4bd445a

applying suggestions

45d8a79

uses string fmt to update the pkg manifest

ae4a5fa

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 15, 2020

camilamacedo86 merged commit 838ed74 into operator-framework:master Sep 15, 2020

camilamacedo86 deleted the golang-cleanup-test branch September 15, 2020 16:28

prafull01 mentioned this pull request Sep 18, 2020

[e2e-test] Added the ginkgo style checks for e2e tests #3914

Merged

camilamacedo86 mentioned this pull request Sep 19, 2020

cleanup: scorecard with its own test #3921

Merged

2 tasks

		makefilePackagemanifestsFragment = strings.Replace(makefilePackagemanifestsFragment,
		"packagemanifests: kustomize manifests", "packagemanifests: kustomize", 1)

Conversation

camilamacedo86 commented Jul 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

estroz left a comment

Choose a reason for hiding this comment

Uh oh!

camilamacedo86 commented Jul 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

camilamacedo86 commented Jul 23, 2020

Uh oh!

joelanford commented Jul 23, 2020

Uh oh!

camilamacedo86 commented Jul 23, 2020

Uh oh!

Uh oh!

Uh oh!

jberkhahn Aug 18, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jmrodri left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

joelanford Sep 14, 2020

Choose a reason for hiding this comment

Uh oh!

camilamacedo86 Sep 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joelanford left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

joelanford Sep 15, 2020

Choose a reason for hiding this comment

Uh oh!

camilamacedo86 Sep 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

camilamacedo86 Sep 15, 2020

Choose a reason for hiding this comment

Uh oh!

joelanford Sep 15, 2020

Choose a reason for hiding this comment

Uh oh!

camilamacedo86 Sep 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

joelanford Sep 15, 2020

Choose a reason for hiding this comment

Uh oh!

camilamacedo86 Sep 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joelanford Sep 15, 2020

Choose a reason for hiding this comment

Uh oh!

joelanford left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci-robot commented Sep 15, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

camilamacedo86 commented Jul 23, 2020 •

edited

Loading

camilamacedo86 commented Jul 23, 2020 •

edited

Loading

camilamacedo86 Sep 14, 2020 •

edited

Loading

camilamacedo86 Sep 15, 2020 •

edited

Loading

camilamacedo86 Sep 17, 2020 •

edited

Loading

camilamacedo86 Sep 15, 2020 •

edited

Loading