search

GoogleCloudPlatform / prometheus-engine

Google Cloud Managed Service for Prometheus libraries and manifests.
https://g.co/cloud/managedprometheus
Apache License 2.0
195 stars 93 forks source link

[E2E refactor - 7] refactor authorization e2e test #751

Closed pintohutch closed 9 months ago

pintohutch commented 10 months ago

I decided to break out the kind E2E test refactor PR https://github.com/GoogleCloudPlatform/prometheus-engine/pull/738 into smaller, digestible PRs for reviewing.

Note: because of the nature of the change, presubmits (i.e. Github Actions) may fail until the final PR is merged.

This is the seventh one, where we refactor and simplify authorization e2e tests.

TheSpiritXIII commented 9 months ago

The way you configured subtests here is non-conventional. Fundamentally, I see subtests as a grouping of tests. You do some common setup, and then you run similar tests as subtests. If you configure it this way, you get full tooling support. For example, in the past, if I was making changes to TLS and I want to run all TLS tests, I would do go test -run "TestTLS". If I want to run TLS tests on PodMonitoring, I would do go test -run "TestTLS/pod-monitoring".

This is no longer possible here because:

  1. Setup is a separate subtest. I can no longer run subtests because they are no longer hermetic. Ultimately, I fail to see much benefit of running setup in separate subtests versus before actual subtests. My belief is that other tests would more closely test popular setup scenarios (e.g. in this case, TestCollector would give more detailed information on why the collector might fail to be ready so the setup subtests in TestAuthorization are redundant).
  2. You solved 1 by splitting up what I consider subtests into individual tests but now I can't run similar tests together unless I craft a regular expression that contains all the tests I want.

Can you maybe explain some of your test philosophy? Maybe a different test framework would more closely align our individual philosophies (e.g. such as BDD style frameworks like Ginko which allow you to explicitly mark setup code).

pintohutch commented 9 months ago

Yea - here you just run, e.g. TEST_RUN=TestBasicAuthClusterPodMonitoring make e2e to get the desired affect.

I did it this way, against some convention for two reasons:

  1. Readability and reusability - each Test has clear steps with comments, and each flavor of test is separated out for clear distinction. Obviously, the main drawback here is DRY - but I lean towards repeating oneself if it makes the code easier to follow - particularly in Go, where this is more conventional. Additionally, the subtests run sequentially and are not marked with t.Parallel().
  2. Parallelism in GH Actions. The conventional way would essentially recommend bundling everything in one TestXXX function, in-line your setup and teardown, and sandwich in your subtests do the various flavors. This works in some cases, and doesn't work in others. For example, in our case, where we need to patch the example-app with specific flags for each subtest. Doing this in one Test function increases build times and can cause timeout issues against the K8s API server.

If we really wanted to force convention for our use case, we could look into changing the way we build our TEST_RUN matrix to list all subtests as well. However, I'm not sure how to do this.

pintohutch commented 9 months ago

Thanks for the feedback - that SGTM!