Need e2e test for #912

[davecheney]

#912 landed late in the 0.10 cycle without an e2e test. It would be awesome if someone could write one so we don't revert this fix in the future.

Updates #603

[vaamarnath]

@davecheney Shall I pick this up?

[davecheney]

Yes please. I’d start by figuring out why none of the e2e tests failed for cds when this landed. I suspect that this is because they are using helpers from internal/envoy. If that is the case, then we don’t need an e2e test, just to check the test coverage for the relevant helper in internal/envoy.

…

On 9 Mar 2019, at 15:12, Amarnath ***@***.***> wrote: @davecheney Shall I pick this up? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[vaamarnath]

I went through cds_test.go and from what I understand all the e2e tests makes use of cluster() helper function which doesn't set HealthChecks. As a result of which, when e2e tests exercise the code, it never hits the true case here.

My current thought process is to add another helper function which lets us create v2.Cluster object with HealthChecks. Using this, I should be able to add an e2e test that exercises the newly added code path. Does this sound reasonable to you?

[davecheney]

Ahh, that would explain it. Have a look at the helpers at the bottom of cds_test, there may be some patterns you can follow

…

On 9 Mar 2019, at 15:24, Amarnath ***@***.***> wrote: I went through cds_test.go and from what I understand all the e2e tests makes use of cluster() helper function which doesn't set HealthCheck. As a result of which, when e2e tests exercise the code, it never hits the true case here. My current thought process is to add another helper function which lets us create v2.Cluster object with HealthCheck. Using this, I should be able to add an e2e test that exercises the newly added code path. Does this sound reasonable to you? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.