Skip to content

Invocation of upstream knative/serving-operator e2e tests#53

Merged
openshift-merge-robot merged 1 commit into
openshift-knative:release-1.3from
cardil:feature/run-upstream-serving-tests-v2
Dec 10, 2019
Merged

Invocation of upstream knative/serving-operator e2e tests#53
openshift-merge-robot merged 1 commit into
openshift-knative:release-1.3from
cardil:feature/run-upstream-serving-tests-v2

Conversation

@cardil
Copy link
Copy Markdown
Member

@cardil cardil commented Dec 6, 2019

A bash version of test invocation of upstream knative/serving-operator. Previous proposition is #32.

There is a patch that skips TestKnativeServingDeployment/configure due to SRVKS-241.

There is a 0.9.0 version of knative/serving-operator used, as current serverless operator do not handle knativeservings.operator.knative.dev definitions yet. After supporting that, versions should be set to support/v0.10.0.

For 0.11+ we can use knative instead of fork (cardil), as master has configurable test namespace knative/serving-operator#223.

@openshift-ci-robot
Copy link
Copy Markdown

Hi @cardil. Thanks for your PR.

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

Details

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.

@jcrossley3
Copy link
Copy Markdown
Contributor

/ok-to-test

@cardil cardil force-pushed the feature/run-upstream-serving-tests-v2 branch 4 times, most recently from 11f4f89 to 489e2aa Compare December 9, 2019 17:22
A bash version of test invocation of upstream knative/serving-operator.

There is a patch that skips TestKnativeServingDeployment/configure due
to SRVKS-241.

There is a 0.9.0 version of knative/serving-operator executed, as
current serverless operator do not handle
knativeservings.operator.knative.dev yet. After supporting that
versions should be set to support/v0.10.0. For 0.11 we can use use
knative instead of fork (cardil) as master has configurable test
namespace.
@cardil cardil force-pushed the feature/run-upstream-serving-tests-v2 branch from 489e2aa to 28edfb1 Compare December 9, 2019 19:04
@cardil
Copy link
Copy Markdown
Member Author

cardil commented Dec 10, 2019

/retest

Copy link
Copy Markdown
Contributor

@mgencur mgencur left a comment

Choose a reason for hiding this comment

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

Hi @cardil generally looks good! Just few comments here and there.

Comment thread test/e2e-tests.sh
# Run upstream knative serving tests
RUN_KNATIVE_SERVING_UPGRADE_TESTS=false
RUN_KNATIVE_SERVING_E2E=true
RUN_KNATIVE_SERVING_UPGRADE_TESTS=${RUN_KNATIVE_SERVING_UPGRADE_TESTS:-false}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we will never want to override it from outside. This concrete script is for running E2E tests, not rolling upgrades. I'm removing these flags in the other PR for separating E2E and Rollups

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I see. So, it will be gone. This change of mine wont damage anything.

Comment thread test/lib.bash
Comment thread test/lib.bash
Comment thread test/lib.bash
Comment thread test/lib.bash
Comment thread test/lib.bash

logger.info "Removing GOPATH: ${GOPATH}"
rm -rf "${GOPATH}"
popd || return $?
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We're running in a subshell so this is probably not necessary?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes. Not necessary, but a good practice to restore.

Comment thread test/lib.bash

logger.info "Run tests of knative/serving-operator @ ${gitdesc}"
env TEST_NAMESPACE='knative-serving' \
go test -v -tags=e2e -count=1 -timeout=30m -parallel=1 ./test/e2e \
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's replace this line with go_test_e2e -tags=e2e -timeout=30m -parallel=1 ./test/e2e \
That way we'll have JUnit reports generated.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Sure.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We can fix this in the other PR as well.

@mgencur
Copy link
Copy Markdown
Contributor

mgencur commented Dec 10, 2019

/lgtm

The things I mentioned are rather cosmetic and there's another PR open that touches the code so I'll fix them there.

@openshift-ci-robot
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cardil, mgencur

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

The pull request process is described here

Details Needs approval from an approver in each of these files:

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

@openshift-merge-robot openshift-merge-robot merged commit eb2d358 into openshift-knative:release-1.3 Dec 10, 2019
@cardil cardil deleted the feature/run-upstream-serving-tests-v2 branch December 10, 2019 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants