Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Regression tests #478

Merged
merged 21 commits into from
Mar 1, 2019
Merged

Regression tests #478

merged 21 commits into from
Mar 1, 2019

Conversation

marcogschmidt
Copy link
Contributor

@marcogschmidt marcogschmidt commented Feb 28, 2019

The new tests in the test/kube2e package install the 3 versions of gloo (gateway, ingress, and knative) and run smoke tests against them to identify regressions. The tests rely on assets in the _test directory which are build in previous build steps.

See the comments in the Makefile and the README in the test directory for more info.

Copy link
Member

@yuval-k yuval-k left a comment

Choose a reason for hiding this comment

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

did a quick first pass and have some questions.

Makefile Outdated
docker build -t soloio/gloo-envoy-wrapper:$(VERSION) $(OUTPUT_DIR) -f $(OUTPUT_DIR)/Dockerfile.envoyinit
docker build $(OUTPUT_DIR) -f $(OUTPUT_DIR)/Dockerfile.envoyinit \
-t soloio/gloo-envoy-wrapper:$(VERSION) \
-t $(GCR_REPO_PREFIX)/gloo-envoy-wrapper:$(TEST_IMAGE_TAG)
Copy link
Member

Choose a reason for hiding this comment

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

this will create a test tag every time. is this intentional?
will it still work if those variable not provided - like in local dev?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The fact that it creates a tag every time is intentional, I am basically piggybacking off the original build command instead of running it again as I figured that creating a tag is an inexpensive operation.

Good point regarding the local builds, I will find a sensible way of addressing that.

cloudbuild.yaml Outdated
waitFor: ['build-test-assets', 'test']

- name: gcr.io/cloud-builders/gcloud
args: ['container', 'clusters', 'get-credentials', 'kube2e-tests']
Copy link
Member

Choose a reason for hiding this comment

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

why is this needed? we have other tests that use a test cluster already, why do we need to get credentials again?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are using a different cluster here (kube2e-test vs test-cluster) so we need to update the KUBECONFIG. I thing the reason for using different clusters was that we want to isolate the two test phases, potentially having them run in parallel (Also, we already had a GKE cluster available that was not being used)

}

func WaitPodStatus(ctx context.Context, interval time.Duration, label, status string, finished func(output string) bool) error {
//func WaitPodsTerminated(ctx context.Context, interval time.Duration, labels ...string) error {

Choose a reason for hiding this comment

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

Remove commented out code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Actually removed the whole file, since it was not used.

cloudbuild.yaml Outdated
waitFor: ['compile']
id: 'build-test-assets'

- name: gcr.io/cloud-builders/gcloud
Copy link
Member

Choose a reason for hiding this comment

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

This one as well. The e2e-ginkgo container authenticates by default

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, didn't realize it. Thanks, I'll removed those two steps then.

@@ -51,7 +51,7 @@ func DeployGlooWithHelm(namespace, imageVersion string, enableKnative, verbose b
return nil
}

func GlooHelmValues(namespace, version string, enableKnative bool) io.Reader {
func glooHelmValues(namespace, version string, enableKnative bool) io.Reader {
b := &bytes.Buffer{}

err := template.Must(template.New("gloo-helm-values").Parse(`
Copy link
Member

Choose a reason for hiding this comment

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

I realize this isn't technically part of this PR, but it seems like these values should be pulled from the actual file rather than trying to keep this in line with the real file

Copy link
Contributor Author

@marcogschmidt marcogschmidt Feb 28, 2019

Choose a reason for hiding this comment

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

Actually, thanks for reminding me, most of this code is not used anymore, so I can just remove it and move the only used function (WaitPodsRunning) to the testrunner file.

@@ -204,6 +213,6 @@ func ToFile(content string) string {
n, err := f.WriteString(content)
ExpectWithOffset(1, err).NotTo(HaveOccurred())
ExpectWithOffset(1, n).To(Equal(len(content)))
f.Close()
_ = f.Close()
Copy link
Member

Choose a reason for hiding this comment

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

why _ = ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The IDE laments about an unhandled error. The _ = makes it explicit that we chose not to handle it.

Copy link
Member

@yuval-k yuval-k left a comment

Choose a reason for hiding this comment

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

@marcogschmidt had a closer look, no further comments

@yuval-k
Copy link
Member

yuval-k commented Mar 1, 2019

actually one more comment - add the larger machine type in cloud build yaml?

@marcogschmidt
Copy link
Contributor Author

actually one more comment - add the larger machine type in cloud build yaml?

It will soon push a commit with all the requested changes, including the larger machine type.

@@ -0,0 +1,3 @@
changelog:

Choose a reason for hiding this comment

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

It doesn't matter for this change, but I would start naming these yaml files something unique to the PR to avoid the possibility of merge conflicts with other open PRs.

@marcogschmidt
Copy link
Contributor Author

/kick

@rickducott rickducott merged commit 42a8957 into master Mar 1, 2019
@marcogschmidt marcogschmidt deleted the regression-tests branch March 1, 2019 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants