CI: Track integration test coverage separately from unit tests

[oschaaf]

Following an offline discussion about coverage flakes in CI, and how to best
improve there we decided to exclude the integration tests altogether from strict coverage
treshold expectations. As such, we lower the unit testing coverage threshold to 92.1%
here. We still run the integration tests in a new CI task, with the intent to make that not
required to pass for merging, so informational only.

Fixes #362

Signed-off-by: Otto van der Schaaf oschaaf@we-amp.com

[mum4k]

As discussed offline, I believe this is the right decision considering a) integration tests don't really show the true coverage, they are better used to measure end-to-end behaviors; and b) the integration test coverage is flaky and the time needed to fix it may be better used by improving our unit test coverage.

I am fine with this PR, however adding @htuch in case he has any thoughts, since this setup predates me.

[htuch]

I'm OK with this, but it's worth noting that you're now diverging from Envoy coverage philosophy. Thinking a little on this, I think it would have sense to have two coverage metrics, one for unit tests and another for integration tests. While we should strive for complete unit test coverage, integration tests are essential and should be covering as many behavior as possible, since this is when the rubber meets the road.

[oschaaf]

to float an idea, I think it would be low effort to add a new CI task which separately runs the integration test for coverage and uploads the results. We could configure that CI task to not block merging upon failure and allow a bounded number of retries for failed tests (to eliminate one cause of the flakes we've seen). That way we would have a relatively easy means to separately set a treshold for this, as well as inspect coverage produced by the just the integration tests. If would still occasionally fail, but as it is not required to pass for merging, it would not be as annoying?

[mum4k]

Thank you @htuch for your input and the note about Envoy's strategy. @oschaaf your idea about two numbers, one being informative only sounds good. Is this something we would want to implement in this PR or a separate one?

[oschaaf]

@mum4k added d559cca to include the proposed new integration test coverage task here, with an independent threshold.

[oschaaf]

Mysteriously, the new CI task isn't showing up in the tasks that CircleCI runs in this PR. I'm not sure yet what that is about.

[mum4k]

If you want, we can split it out from this PR and address it separately? Or do we merge as is and address the issue afterwards?

[oschaaf]

I think it's worth trying to merge as-is. Hopefully the new task will pick up once merged to master, and if it isn't I'll look into it right away