Skip to content

ci: create baseline and experimental test app pipelines#2075

Merged
goaway merged 5 commits intomainfrom
ms/ci-experimental-pipeline
Mar 22, 2022
Merged

ci: create baseline and experimental test app pipelines#2075
goaway merged 5 commits intomainfrom
ms/ci-experimental-pipeline

Conversation

@goaway
Copy link
Contributor

@goaway goaway commented Feb 28, 2022

Description: Creates distinct test app profiles for end-to-end testing of baseline and new/experimental features in CI on Swift and Kotlin. Previously only a baseline configuration was run in CI which resulted in blindspots, especially with respect to upstream changes.
Risk Level: Low
Testing: Local & CI

Signed-off-by: Mike Schore mike.schore@gmail.com

goaway added 4 commits March 16, 2022 01:01
Signed-off-by: Mike Schore <mike.schore@gmail.com>
Signed-off-by: Mike Schore <mike.schore@gmail.com>
Signed-off-by: Mike Schore <mike.schore@gmail.com>
Signed-off-by: Mike Schore <mike.schore@gmail.com>
@goaway goaway force-pushed the ms/ci-experimental-pipeline branch from d1780f1 to 5736e99 Compare March 15, 2022 17:01
@goaway goaway marked this pull request as ready for review March 15, 2022 17:52
@goaway goaway requested a review from Augustyniak March 16, 2022 11:28
@Augustyniak Augustyniak self-assigned this Mar 16, 2022
Copy link
Contributor

@Augustyniak Augustyniak left a comment

Choose a reason for hiding this comment

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

I like the general idea of us testing how the engine behaves when all/some of its features are enabled.

So just to make sure that you know where I am coming from. As I understood that when we were talking about these changes (back when you proposed them originally) the idea behind them was to "continue to make sure that the engine continues to perform well even if all of its feature are enabled".

Some of the thoughts after a brief look at the PR:

  1. It adds a lot of complexity/maintenance burden as we introduce another copy(ies) of filters and some other components and some of the upstream changes will require us to update all of these copies.
  2. It may add to the confusion for people who want to contribute to Envoy Mobile. We have 3(?) example apps per platform now as opposed to one and that may lead to a lot of questions. If we decide to stick with this approach we should add some documents that would explain why we need all of these example apps (and to make people update them as they keep adding features?)
  3. Couldn't we just add a few integration tests (one with all feature disabled/one with all features enabled) that would start the engine and perform a requests (or a few of them) to see whether EM works well? Wouldn't that give us exactly the same level of confidence as all of these newly introduced example apps do? At the same time, with integration tests approach we could cut significantly reduce the amount of added code (and using integration tests to verify things seems to be a more common practice than using custom example apps).
  1. and 2) are the issues that I see with the currently proposed approach. 3) is a question regarding an alternative that I can see.

@goaway
Copy link
Contributor Author

goaway commented Mar 16, 2022

I think I maybe wasn't clear enough in that I consider this PR as a starting point, and not the ideal end state.

  1. It adds a lot of complexity/maintenance burden as we introduce another copy(ies) of filters and some other components and some of the upstream changes will require us to update all of these copies.

The idea is that these wouldn't be copies long-term. They're starting that way because it doesn't substantially change the functionality of the tests from today. I can (and would) open a tracking issue to delete most of the "example" code from the test harnesses and most of the "test" code from the example apps - resulting in different and simpler setups that serve their respective purposes better.

  1. It may add to the confusion for people who want to contribute to Envoy Mobile. We have 3(?) example apps per platform now as opposed to one and that may lead to a lot of questions. If we decide to stick with this approach we should add some documents that would explain why we need all of these example apps (and to make people update them as they keep adding features?)

I'd actually like to move the test harness apps into the /tests and leave only the example apps in /examples. I'd actually suggest it's more confusing to have code that exists purely for CI verification sitting in the example apps.

  1. Couldn't we just add a few integration tests (one with all feature disabled/one with all features enabled) that would start the engine and perform a requests (or a few of them) to see whether EM works well? Wouldn't that give us exactly the same level of confidence as all of these newly introduced example apps do? At the same time, with integration tests approach we could cut significantly reduce the amount of added code (and using integration tests to verify things seems to be a more common practice than using custom example apps).

We have these tests because we've found historically that integration tests are not enough to catch everything. Envoy Mobile has a lot of layer-to-layer wiring, and the narrower scope of both Unit and Integration tests have historically missed issues that cross those boundaries.

@Augustyniak
Copy link
Contributor

After talking with @goaway offline:

  • @goaway is going to open a GH issue that will track the future work for simplifying (removing) code from new CI example apps. We are going to remove some http filters, simplify apps' ViewControllers etc.
  • we are going to add some documentation that would explain what's the intent behind these new CI example apps and how they should be updated when somebody add a new feature to Envoy Mobile.
  • new CI example apps are going to be moved out of examples directory

Signed-off-by: Mike Schore <mike.schore@gmail.com>
Copy link
Contributor

@Augustyniak Augustyniak left a comment

Choose a reason for hiding this comment

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

Let's make sure that we do simplify newly added soon. I will advocate for removing these if we see that they add too much of maintenance burden.

@goaway goaway merged commit 0141a66 into main Mar 22, 2022
@goaway goaway deleted the ms/ci-experimental-pipeline branch March 22, 2022 22:08
jpsim added a commit that referenced this pull request Apr 12, 2022
* main: (59 commits)
  Bump Lyft Support Rotation (#2156)
  add specifying more maven deps (#2151)
  update envoy@e4eaf1b97 (#2146)
  bazel: create symbol mapping file (#2126)
  Bump Lyft Support Rotation (#2148)
  bazel: remove sandbox disable
  build: export flatbuffer jvm dep (#2147)
  Bump Lyft Support Rotation (#2143)
  bazel: Add flatbuffers Swift hack
  key_value: structure for prefs based key value store (#2120)
  build: add flatbuffers (#2133)
  Bump Lyft Support Rotation (#2131)
  envoy: bump upstream Envoy to 419e237 (#2132)
  stats: enable more metrics (#2130)
  Use the right type (envoy_network_t) (#2125)
  Bump Lyft Support Rotation (#2118)
  Update CONTRIBUTING.md to include updating subrepos (#2023)
  ci: create baseline and experimental test app pipelines (#2075)
  config: temporarily hardcode h2 max concurrent streams to 100 (#2124)
  ...

Signed-off-by: JP Simard <jp@jpsim.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants