Add the support to test against specified release of Knative serving #170

houshengbo · 2019-06-07T21:42:50Z

This PR adds an environment variable $KNATIVE_VERSION for both e2e-tests.sh and presubmit-tests.sh, in order to specify the version of Knative serving to be installed.

Closes: #168

knative-prow-robot · 2019-06-07T21:43:05Z

Hi @houshengbo. Thanks for your PR.

I'm waiting for a knative 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.

houshengbo · 2019-06-07T21:51:26Z

/assign @sixolet @rhuss

rhuss

Thanks ! I have some suggestion to discuss wrt/ organizing the script.

rhuss · 2019-06-08T08:27:11Z

test/e2e-tests.sh

-  start_latest_knative_serving
-}
+export KNATIVE_SERVING_VERSION=latest
+source $(dirname $0)/e2e-common.sh


Instead of sourcing the same file twice, could we put the actual test code into a reusable function which gets the target version as argument?

Its easier to maintain if you transport the input to a function with arguments and not global environment variables (which also avoid unwanted side effects because of this global state), and you can reuse it much easier e.g. when you want to add another version.

Therefore I would also roll up the code into a loop which makes it more concise.

rhuss · 2019-06-08T08:27:53Z

test/e2e-common.sh

+
+initialize $@
+
+header "Running tests"


Can we output the version to test again here, too ? Makes it easier to analyse test failures.

rhuss · 2019-06-08T08:31:00Z

test/e2e-common.sh

+# Helper functions.
+
+function knative_setup() {
+  start_release_knative_serving "$KNATIVE_SERVING_VERSION"


I understand that this callback function is only easy to customize with an environment variable (as you can't change the signature). However, you could put this kind of implementation detail inside the function that is called from outside this script so that you can avoid to source this file twice.

This function knative_setup
is called by source $(dirname $0)/../vendor/github.com/knative/test-infra/scripts/e2e-tests.sh.

It has to be defined after source $(dirname $0)/../vendor/github.com/knative/test-infra/scripts/e2e-tests.sh. Am I correct?

If it is defined in e2e-tests.sh with soucing github.com/knative/test-infra/scripts/e2e-tests.sh, I do not think it will be called.

Tbh, I don't think that e2e-tests.sh execute any codes, but only defines functions which can be called later. Otherwise knative_setup would have to be defined before sourcing this file

As a rule of thumb: Any shell script which is source must only define functions or global environment variables (if they have to).

rhuss · 2019-06-08T10:28:50Z

test/e2e-tests.sh

-./kn service delete hello || fail_test
-./kn service delete foo || fail_test
-success
+export KNATIVE_SERVING_VERSION=$PREVIOUS_RELEASE


To be more concrete, my suggestion would be something along those lines:

e2e-tests.sh:

sourcce e2e-commons.sh for version in latest $PREVIOUS_RELEASE; do echon "===== Starting E2E tests for $version run_tests $version done

e2e-common.sh:

run_tests() { local $version=${1} export KNATIVE_SERVING_VERSION=$version initialize header "[$version]: Running tests" kn_tests } kn_tests() { ./kn service create hello --image gcr.io/knative-samples/helloworld-go -e TARGET=Knative || fail_test sleep 5 ./kn service get || fail_test ./kn service update hello --env TARGET=kn || fail_test sleep 3 ./kn revision get || fail_test ./kn service get || fail_test ./kn service create hello --force --image gcr.io/knative-samples/helloworld-go -e TARGET=Awesome || fail_test ./kn service create foo --force --image gcr.io/knative-samples/helloworld-go -e TARGET=foo || fail_testa sleep 5 ./kn revision get || fail_test ./kn service get || fail_test ./kn service describe hello || fail_test ./kn service delete hello || fail_test ./kn service delete foo || fail_test success }

rhuss

Thanks a lot for putting the test code in functions. However, as mentioned in the review we must not couple ourselves to the internal functions of the e2e library.

I opened knative/test-infra#897 for updating the upstream script to allow a parameterization of the Knative installation against which we want to install.

rhuss · 2019-06-11T16:09:50Z

test/e2e-common.sh

+# This method is copied from github.com/knative/test-infra/scripts/e2e-tests.sh, since the original var
+# $KNATIVE_SERVING_RELEASE is readonly. In order to make it changeable, we rewrite the method with another
+# variable $KNATIVE_SERVING_RELEASE_YAML.
+function start_release_knative_serving() {


I see your motivation but we shouldn't do this, because:

We don't benefit from changes in this method upstream

When the overridden function gets renamed the script fails silently (because this method is just not called).

Instead we should push a change upstream to make the version to install parameterizable either via an argument or an environment variable.

Tbh I think its even a fault that this method can be overridden. This subtle coupling is really hard to debug.

Let me open an issue in the testing upstream repo.

Yep, it is good to see test-infra is able to support the installation of custom release number.

rhuss · 2019-06-11T16:57:28Z

/ok-to-test

houshengbo · 2019-06-14T21:19:39Z

/retest

rhuss · 2019-06-17T10:13:35Z

test/e2e-common.sh

+}
+
+echo "===== Starting E2E tests for Knative serving of the release $KNATIVE_SERVING_VERSION"
+run_tests $@


This is the function call which will trigger everything. Up to this point, everything needs to be defined/sourced.

As I expect e22-common.sh to be a kind of library it should also only contain function definitions but must not call or execute any command when being included.

rhuss

Thanks, you are heading in the right direction. However, you should take care to only call the final execution function run_tests in the outermost script, not by calling or sourcing an external script. See the comments inline.

rhuss · 2019-06-17T10:14:32Z

test/e2e-common.sh

+}
+
+echo "===== Starting E2E tests for Knative serving of the release $KNATIVE_SERVING_VERSION"
+run_tests $@


As I expect e22-common.sh to be a kind of library it should also only contain function definitions but must not call or execute any command when being included.

rhuss · 2019-06-17T10:15:33Z

test/e2e-tests.sh

-success
+for version in "${listOfRelease[@]}"; do
+   export KNATIVE_SERVING_VERSION=$version
+   $(dirname $0)/e2e-common.sh


Instead of calling out to e2e-commons.sh, call here run_tests $@ and source all function definitions before.

rhuss · 2019-06-17T10:17:26Z

test/e2e-common.sh

+# Helper functions.
+
+function knative_setup() {
+  start_release_knative_serving "$KNATIVE_SERVING_VERSION"


Tbh, I don't think that e2e-tests.sh execute any codes, but only defines functions which can be called later. Otherwise knative_setup would have to be defined before sourcing this file

As a rule of thumb: Any shell script which is source must only define functions or global environment variables (if they have to).

rhuss · 2019-06-17T10:27:39Z

The integration tests fails with a

I0617 01:33:17.642] 2019/06/17 01:33:17 process.go:155: Step '/go/src/github.com/knative/client/test/e2e-common.sh --run-tests' finished in 1m17.206419171s
I0617 01:33:17.658] 2019/06/17 01:33:17 main.go:312: Something went wrong: encountered 1 errors: [error during /go/src/github.com/knative/client/test/e2e-common.sh --run-tests: signal: segmentation fault (core dumped)]
I0617 01:33:17.714] Test subprocess exited with code 1

(not sure where this comes from) and the client test fails because the deps have not been updated.

@houshengbo could you please run a hack/build.sh -u and commit the updated modules.txt to fix the client test ?

Let's hope that the integration test failure is just a flake.

houshengbo · 2019-06-17T14:31:33Z

@rhuss Thanks for your comments. I am thinking of another way to fix this issue.
For the repo of knative client, I would like to add the support of a param, so that e2e-tests.sh can either run against the latest build or a previous release.
Then, we need to make the integration job of pull-knative-client-integration-tests to test both the latest and previous release. Instead of running test/e2e-tests.sh once, we can two builds, like
./test/e2e-tests.sh latest and ./test/e2e-tests.sh 0.6.0.
It is not quite friendly to run tests against two knative installations on local machines. Folks are a sort of forced to run twice of the setup and integration tests.
What do you think?

rhuss · 2019-06-17T18:57:00Z

What do you think?

yes, sounds good to me. Also if we could configure that as two different GitHub checks then it would be evident which one is failing. Also it helps in parallelizing the tests which should it make faster.

rhuss · 2019-06-28T11:43:08Z

@houshengbo There are some minor merge conflicts. Could you please fix them? I will review the PR in the meantime. Thanks !

rhuss

See my comments below.

And I would rename the variable to KNATIVE_SERVING_VERSION as it is really about installing Knative Serving (and we might want to have different numbers for Knative eventing when it enters the client)

test/e2e-tests.sh

rhuss · 2019-06-28T12:29:58Z

test/e2e-tests.sh


 # Script entry point.

 initialize $@

-header "Running tests"
+header "Running tests for Knative serving version of $KNATIVE_VERSION"


"Running tests for Knative serving $KNATIVE_VERSION"

test/e2e-tests.sh

rhuss · 2019-06-28T14:18:31Z

test/presubmit-integration-tests-previous-release.sh

@@ -0,0 +1,21 @@
+#!/usr/bin/env bash


I don't really get this file (but could be also my ignorance ;)

Shouldn't it be called '...latest-release.sh' as it refers to the latest release whereas "latest" refers to HEAD of master (unreleased code). A bit confusing. Or maybe call it really with the version number ('presubmit-integration-tests-0.6.0.sh') and update the configuration that calls this file

Who is calling this file ?

Why it is calling presubmit checks and not the e2e tests like in e2e-tests.sh but with a environment variable set ?

So I would have expected something like

export KNATIVE_VERSION="0.6.0" $(dirname $0)/e2e-tests.sh

Talked offline with @houshengbo . It's complicated (as the second job needs to be a custom job).

Thank @rhuss for his suggestion. We will use latest-release.
The repo test-infra defines four basic type of jobs, every PR can enable: integration-tests, build-tests, go-coverage, and unit-tests. We can only have four regular jobs, with one for each of them respectively. It means even if we would like to have a new INTEGRATION TEST, we need to go through CUSTOM TEST JOB in test-infra. We cannot use the existing presubmit-tests.sh --integration-tests with more argument to specify the version or anything else, and we cannot use extra argument for command in CUSTOM TEST either. That was the reason we needed to have a separate script like this one, with the correct env var setting for a specific version, to launch separately with a CUSTOM TEST JOB in test-infra. BTW, another PR for test-infra related to this PR is knative/test-infra#954.

rhuss · 2019-06-28T18:08:27Z

/lgtm

knative-prow-robot · 2019-06-28T18:08:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: houshengbo, rhuss

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~~ [rhuss]

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

rhuss · 2019-06-28T18:08:34Z

Thanks !

…native#170)

* Create a helper for running e2e go tests Let's hide the complexity of running e2e go tests in `e2e-tests.sh`. Bonus: move documentation to `README.md` to make it more visible. Part of knative#170. * Add kubectl call to example

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jun 7, 2019

knative-prow-robot requested review from cppforlife and evankanderson June 7, 2019 21:43

knative-prow-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 7, 2019

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 7, 2019

houshengbo changed the title ~~Add the support test against the previous release and the latest source~~ Add the support to test against the previous release and the latest source Jun 7, 2019

knative-prow-robot assigned rhuss and sixolet Jun 7, 2019

rhuss requested changes Jun 8, 2019

View reviewed changes

rhuss requested changes Jun 11, 2019

View reviewed changes

knative-prow-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 Jun 11, 2019

rhuss reviewed Jun 17, 2019

View reviewed changes

rhuss requested changes Jun 17, 2019

View reviewed changes

rhuss mentioned this pull request Jun 17, 2019

WIP: Add the support to test against the previous release and the latest source #190

Closed

knative-prow-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 17, 2019

knative-prow-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 Jun 17, 2019

houshengbo changed the title ~~Add the support to test against the previous release and the latest source~~ Add the support to test against specified previous release Jun 17, 2019

houshengbo mentioned this pull request Jun 18, 2019

Add a new integration tests job for knative client for latest release knative/test-infra#954

Merged

knative-prow-robot removed the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 28, 2019

knative-prow-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 28, 2019

rhuss requested changes Jun 28, 2019

View reviewed changes

Add the support to test against specified release of Knative serving

3463afc

houshengbo changed the title ~~Add the support to test against specified previous release~~ Add the support to test against specified release of Knative serving Jun 28, 2019

rhuss approved these changes Jun 28, 2019

View reviewed changes

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 28, 2019

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 28, 2019

knative-prow-robot merged commit 663d76a into knative:master Jun 28, 2019

maximilien pushed a commit to maximilien/client that referenced this pull request Jul 1, 2019

Add the support to test against specified release of Knative serving (k…

e3ca309

…native#170)

Add the support to test against specified release of Knative serving #170

Add the support to test against specified release of Knative serving #170

Conversation

houshengbo commented Jun 7, 2019 • edited Loading

knative-prow-robot commented Jun 7, 2019

houshengbo commented Jun 7, 2019

rhuss 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

houshengbo Jun 10, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhuss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhuss commented Jun 11, 2019

houshengbo commented Jun 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhuss 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

rhuss commented Jun 17, 2019

houshengbo commented Jun 17, 2019

rhuss commented Jun 17, 2019

rhuss commented Jun 28, 2019

rhuss 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

houshengbo Jun 28, 2019 • edited Loading

Choose a reason for hiding this comment

rhuss commented Jun 28, 2019

knative-prow-robot commented Jun 28, 2019

rhuss commented Jun 28, 2019

houshengbo commented Jun 7, 2019 •

edited

Loading

houshengbo Jun 10, 2019 •

edited

Loading

houshengbo Jun 28, 2019 •

edited

Loading