Skip to content
This repository was archived by the owner on Jun 4, 2021. It is now read-only.

Another attempt to add an E2E test#178

Merged
knative-prow-robot merged 34 commits into
knative:masterfrom
bbrowning:sources-e2e-take-2
Mar 18, 2019
Merged

Another attempt to add an E2E test#178
knative-prow-robot merged 34 commits into
knative:masterfrom
bbrowning:sources-e2e-take-2

Conversation

@bbrowning
Copy link
Copy Markdown
Contributor

This is an attempt to push #121 to completion so we get some basic E2E testing for eventing-sources. It may take a couple of iterations here to get things passing.

grantr and others added 14 commits November 20, 2018 10:28
This is a copy of the eventing E2E test which checks if
KubernetesEventSource can deliver an event to a sink.
Dep added a bunch of non-go vendored files from eventing/cmd/* for some
reason.
This was fixed in Istio 1.0.
It isn't installed by default for e2e tests.
This copies over the k8sevents image, some other necessary utility
classes, and updates the location the e2e tests look for images to the
published eventing-sources image path.
@knative-prow-robot knative-prow-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jan 11, 2019
Update the test import in k8s_events_test.go to eventing-sources/test
instead of eventing/test. This fixes an error around duplicated e2e
test flags and ensures we aren't reliant on implementation details of
the eventing repo's tests.
@bbrowning
Copy link
Copy Markdown
Contributor Author

/test pull-knative-eventing-sources-integration-tests

@vaikas
Copy link
Copy Markdown
Contributor

vaikas commented Jan 17, 2019

@n3wscott since you just cleaned up eventing e2e tests.

Comment thread test/crd.go Outdated
Comment thread test/e2e/e2e.go
Comment thread test/e2e/e2e.go
Comment thread test/e2e/k8s_events_test.go
@n3wscott
Copy link
Copy Markdown
Contributor

It looks like there is a lot of copy/pasta from the e2e test from eventing. I think we can let this in at the moment but I think we (knative) should build up a e2e framework that can be shared between the project. What do you think of that overall plan?

So, let this in, but it is tech-debt.

@bbrowning
Copy link
Copy Markdown
Contributor Author

There is definitely copy & paste here - both from the previous PR (121) and from Eventing repo in general. At the time, my goal was to just push the e2e work already started to a working state so that we'd get some e2e test coverage here.

I've resolved the merge conflicts here so it's unblocked from getting in. And I'll create an issue in knative/pkg to track pulling these common e2e test bits out. I believe other repos also have similar duplication of e2e helpers that would also benefit from consolidation.

@bbrowning
Copy link
Copy Markdown
Contributor Author

@vaikas-google @n3wscott Note that the most recent unit tests run as part of this PR exposed the same data race observed on #191 - a lot of DATA RACE mentions at https://storage.googleapis.com/knative-prow/pr-logs/pull/knative_eventing-sources/178/pull-knative-eventing-sources-unit-tests/1093897152024809473/build-log.txt

@bbrowning
Copy link
Copy Markdown
Contributor Author

/test pull-knative-eventing-sources-unit-tests

Comment thread test/cleanup.go Outdated
@bbrowning bbrowning changed the title Another attempt to add an E2E test [WIP] Another attempt to add an E2E test Mar 15, 2019
@knative-prow-robot knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 15, 2019
@bbrowning bbrowning changed the title [WIP] Another attempt to add an E2E test Another attempt to add an E2E test Mar 15, 2019
@knative-prow-robot knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 15, 2019
@bbrowning
Copy link
Copy Markdown
Contributor Author

This initial e2e test is passing again and ready for an updated review. The expectation is that once we get an e2e suite bootstrapped others can start adding e2e tests for each source.

This is a full E2E test of the Kubernetes Events source wired up via Eventing. I know we've moved towards reducing test dependencies between repos in the other projects, but I left that dependency in for now as otherwise there's nowhere that confirms Eventing Sources and Eventing actually work together.

I believe the existing issue knative/pkg#26 is probably the best place to track reducing this kind of copy/paste between e2e setups in each repo and I added a comment there as well as this comment that will link the two issues.

Copy link
Copy Markdown
Contributor

@srinivashegde86 srinivashegde86 left a comment

Choose a reason for hiding this comment

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

we can use a few pkg/test methods here already

Comment thread test/clients.go Outdated
Comment thread test/cleanup.go Outdated
Comment thread test/e2e/e2e.go
Comment thread test/e2e/e2e.go
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/util/wait"

// Mysteriously required to support GCP auth (required by k8s libs).
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 dont think we need this anymore

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Serving and Eventing master still have this in as well. I can attempt to take it out and see what happens, but I assume something still needs it.

@srinivashegde86
Copy link
Copy Markdown
Contributor

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 18, 2019
Copy link
Copy Markdown
Contributor

@grantr grantr left a comment

Choose a reason for hiding this comment

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

Thanks @bbrowning for getting this across the line! The first one is the hardest.

These will probably need more work as we expand the number of Addressables (e.g. Broker) and write E2E tests for more sources, but good start as is.

@knative-prow-robot
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bbrowning, grantr

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

@knative-prow-robot knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 18, 2019
@knative-prow-robot knative-prow-robot merged commit 53f911f into knative:master Mar 18, 2019
@bbrowning
Copy link
Copy Markdown
Contributor Author

Thanks for the reviews!

@bbrowning bbrowning deleted the sources-e2e-take-2 branch March 18, 2019 21:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants