Conversation
Signed-off-by: mathetake <takeshi@tetrate.io>
Signed-off-by: mathetake <takeshi@tetrate.io>
Signed-off-by: mathetake <takeshi@tetrate.io>
Signed-off-by: mathetake <takeshi@tetrate.io>
Signed-off-by: mathetake <takeshi@tetrate.io>
Signed-off-by: mathetake <takeshi@tetrate.io>
Signed-off-by: mathetake <takeshi@tetrate.io>
Signed-off-by: mathetake <takeshi@tetrate.io>
Signed-off-by: mathetake <takeshi@tetrate.io>
Signed-off-by: mathetake <takeshi@tetrate.io>
Signed-off-by: mathetake <takeshi@tetrate.io>
Signed-off-by: mathetake <takeshi@tetrate.io>
|
TODO: write some integration tests |
mathetake
changed the title
|
|
||
| - name: Test | ||
| run: | | ||
| "bazel" test //integration_test/envoy/... --define wasm=wasmtime --verbose_failures |
There was a problem hiding this comment.
What is this supposed to test? It doesn't interact with changes in this repository, it simply runs Envoy tests.
There was a problem hiding this comment.
Yeah, now it's just simply building envoy with the HEAD of this repo. I was thinking about writing e2e test as in istio/proxy https://github.com/istio/proxy/tree/master/test/envoye2e, or run envoy's integration test directly. What do you think?
There was a problem hiding this comment.
Right, overriding proxy_wasm_cpp_host works, and you could use the existing Wasm unit tests in Envoy, but that results in a circular dependency, where you cannot commit any changes to this repo that also require changes in Envoy upstream, because it would break this test.
There was a problem hiding this comment.
ah, that's true. So We should have our own test plugins and e2e tests in this repository rather than depending on integration tests in Envoy.
There was a problem hiding this comment.
hmm, in any case, we cannot avoid changing the Envoy location in this repo to folk Envoy repositories if we want to make any breaking change just like as we do in "opening a PR in Envoy to run unit tests" 😞
There was a problem hiding this comment.
It's not so much about our own test plugins and e2e tests, as it's about a lot of real non-cleanup changes requiring changes in both repositories, which would always fail this test.
We could potentially mark it as an optional CI target, but then we might get in a habit of always ignoring it, so I'm not sure how much value it would really provide vs being always false negative.
There was a problem hiding this comment.
@lizan I would like to have your thoughts on this PR. As you know that cpp-host is tightly coupled with Envoy right now, we have to open PR in Envoy corresponding to almost every PRs in this repository to ensure that all the unittest passes, which is a little bit time consuming and complicated process. What's more, sometimes istio/proxy's e2e test(https://github.com/istio/proxy/tree/master/test/envoye2e) fails(?) even after the Envoy's unit test passes.
That's why I initially thought that building Envoy and running e2e tests in this repository would be great for us as host implementation developers. But as Piotr said, the cycle dependency seems problematic and I really agree with that point.
There was a problem hiding this comment.
We could potentially mark it as an optional CI target, but then we might get in a habit of always ignoring it, so I'm not sure how much value it would really provide vs being always false negative.
I don't have a real good answer for this. I would agree this is the way to go. The real question is how many changes also requires Envoy change. So we can try optional CI target and if that doesn't work just delete it later?
3 participants
depends on #105