Rewrite e2e test to no longer use eventing sources. #726
Conversation
knative-prow-robot
added
the
size/XXL
test/test_images/logevents/main.go
Outdated
| Data string `json:"data"` | ||
| } | ||
|
|
||
| func handler(ctx context.Context, hb *Heartbeat) { |
There was a problem hiding this comment.
Not sure about this change - seems like it should be type-agnostic to accommodate future tests. Don't feel strongly though as long as it works (and it works, yay!)
I definitely like that this is its own thing so it can evolve as the e2e tests require without considering breakage (unlike the message-dumper tool).
There was a problem hiding this comment.
now that I can get it to work quickly I can iterate on adding the loose coupling back, I think the lib supports this but we have been attempting to do it in a non compatible way. The consumer side needs to pass a io reader, not a map[string]interface. because it can't reflectively create that object...
knative-prow-robot
added
the
approved
There was a problem hiding this comment.
Since we're not doing anything about a test, why not just do an http post from the test script directly? Seems like there would be more visibility in case of any errors since they would show up in teh test logs, now if anything goes wrong in the pod (http post failure, etc.) we have to do extra work to scrape those logs, etc.
| @@ -0,0 +1,143 @@ | |||
| /* | |||
There was a problem hiding this comment.
Just a question if there's any value in having a flag controller binary instead of just doing a POST from the test script?
There was a problem hiding this comment.
Point being that if something goes wrong in the POST for example, where will those errors surface.
There was a problem hiding this comment.
I like the idea of being able to post from the script for better error handling, if we can solve istio.
There was a problem hiding this comment.
yes this is a good idea. I will add a flag to control this.
There was a problem hiding this comment.
@grantr when you say from a script, do you mean you prefer a separate pod being created, or ? Just clarifying so I understand your preference :)
There was a problem hiding this comment.
@vaikas-google I think it would be more convenient to make requests to eventing endpoints directly from the pod running the go test. I do not prefer a separate pod creation, though I don't have a strong preference either way. This seems like something that can be changed in a future PR.
There was a problem hiding this comment.
we could make a bounce pod, like just a simple proxy pod that gets configured with the sink and then exposes it's self for a moment while the test runs.
There was a problem hiding this comment.
That's my preference as well, I think creating new pods just to send an event seems unnecessary and makes it harder to debug test failures.
There was a problem hiding this comment.
Ok, so just to close the loop on this. I didn't realize that the reason for this is because tests run outside of the cluster and hence they don't have access to sending events directly to the exposed channels. My bad.
| return ns, shutdown | ||
| } | ||
|
|
||
| func TestSingleBinaryEvent(t *testing.T) { |
There was a problem hiding this comment.
LOOK!!!
I verified this works running the test locally on an existing cluster but not with the script. It is still running...
There was a problem hiding this comment.
update: It worked!
PASS
ok github.com/knative/eventing/test/e2e 174.283s
**************************************
*** E2E TESTS PASSED ***
**************************************
| @@ -188,6 +218,17 @@ func CreateServiceAccountAndBinding(clients *test.Clients, name string, logger * | |||
| return nil | |||
| } | |||
|
|
|||
| // CreateService will create a Service | |||
There was a problem hiding this comment.
I think rather than create a service, using the route stuff will handle the istio related stuff. But, takes a depedency on serving, which I think we already might have?
There was a problem hiding this comment.
we do at the moment but as more things move out we might not in the future?
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: grantr, n3wscott, vaikas-google 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:
Approvers can indicate their approval by writing |
Fixes #686
Proposed Changes
Release Note