Restores the bash e2e tests as a smoke test

[maximilien]

This should allow a set of small e2e tests that only use kn and do not depends on the e2e Golang code and other dependencies that it might create.

[rhuss]

LGTM. Are there any plans to run those test for each PR, too ? Where would this need to be triggered ?

[maximilien]

LGTM. Are there any plans to run those test for each PR, too ? Where would this need to be triggered ?

Great question. Was wondering that myself. Perhaps @adrcunha has some insights to share? I know he helped various times for infra scripts. Thx in advance.

[adrcunha]

Get this merged, clone https://github.com/knative/test-infra/pull/908/files#diff-6b96213e43367b89ca71567c589995ed , run make config in ci/prow, include both files (config.yaml and config_knative.yaml) in a PR and send it to test-infra. Once that's merged and the job is up, set it as a required check in the master branch of this repo.

[adrcunha]

I suggest doing what serving does, and move the common code (source, cluster_setup, knative_setup, initalize) into a e2e-common.sh file in a (near) future PR.

[maximilien]

OK

[adrcunha]

Hopefully soon this can be replaced with a proper checking, in order to avoid flakes?

[rhuss]

Yes, with #54 (landing soon hopefully) we will have synchronous creation by default and don't need any sleep here.

[maximilien]

Get this merged, clone https://github.com/knative/test-infra/pull/908/files#diff-6b96213e43367b89ca71567c589995ed , run make config in ci/prow, include both files (config.yaml and config_knative.yaml) in a PR and send it to test-infra. Once that's merged and the job is up, set it as a required check in the master branch of this repo.

Slight confusion here. The PR 908 (link) is merged. I cloned the test-infra repo and did as you suggested: run make config in ci/prow.

➜  prow git:(master) make config
go run make_config.go --prow-config-output="/Users/maximilien/Desktop/github/knative-test-infra/src/github.com/knative/test-infra/ci/prow//config.yaml" --testgrid-config-output="/Users/maximilien/Desktop/github/knative-test-infra/src/github.com/knative/test-infra/ci/prow//../testgrid/config.yaml" config_knative.yaml

The files have not changed... so perhaps I should skip this step.

[cppforlife]

@sixolet i dont think these tests are useful especially since they do not assert on anything except an error code. if go e2e arent readable, we should fix that instead.

[adrcunha]

@maximilien sorry about not being clear, I wrote that in a hurry. Don't worry about test-infra for now. When I wrote "clone 908" I meant the same kind of job needs to be added to the settings for the client repo so we have the presubmit. Once this PR is merged we can talk about it.

[rhuss]

@sixolet i dont think these tests are useful especially since they do not assert on anything except an error code. if go e2e arent readable, we should fix that instead.

The (small) value I see here is that you call the command through a shell and not through exec (like in go's e2e). E.g. in the discussion about using wildcard patterns with kn there was the legitimate concern that wildcard expansion happens on the shell layer. That wouldn't be caught by exec based tests.

Said that maybe it would make more sense to extend the go based e2e tests to use a shell (if they not already do, sorry haven't checked that yet, but I doubt they do).

If it's not too expensive, having a simple test testing in one (or if we want to go over the top, with multiple) shell makes sense, as kn is supposed to be executed from within a shell. It's as close to the real thing as it gets.

[maximilien]

/retest

[maximilien]

OK I think the backend infra is failing miserably on all PRs tests... Istio failing here.

go: error loading module requirements
ERROR: cluster setup failed
***************************************
***         E2E TEST FAILED         ***

...
istio-system      6m26s       Warning   FailedMount                    Pod                       MountVolume.SetUp failed for volume "certs" : secrets "istio.istio-galley-service-account" not found

I'll attempt retesting later or tomorrow.

[adrcunha]

No, the failure happens when building kn, in cluster_setup():

...
20170904100407-a1d1b6c41b1c
go: k8s.io/[email protected]: unknown revision v0.0.0
go: finding github.com/golangplus/bytes v0.0.0-20160111154220-45c989fe5450
go: finding github.com/storageos/go-api v0.0.0-20180912212459-343b3eff91fc
go: finding github.com/golang/mock v0.0.0-20160127222235-bd3c8e81be01
go: finding github.com/gorilla/mux v1.7.0
go: error loading module requirements
ERROR: cluster setup failed
***************************************
***         E2E TEST FAILED         ***
***    Start of information dump    ***
***************************************

[adrcunha]

/retest

[cppforlife]

Said that maybe it would make more sense to extend the go based e2e tests to use a shell (if they not already do, sorry haven't checked that yet, but I doubt they do).

im not quite clear what benefit we would be getting out of testing in bash vs exec-ing? user contract of the binary isnt changing based on the shell.

[rhuss]

im not quite clear what benefit we would be getting out of testing in bash vs exec-ing? user contract of the binary isnt changing based on the shell.

E.g Wildcard expansion ? There are differences how arguments interpreted, shell vs. exec.

[sixolet]

/lgtm
/approve

[knative-prow-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maximilien, rhuss, sixolet

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 [sixolet]

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