Skip to content

Geo Proxy API affects workhorse testing

The following discussion from !74329 (merged) should be addressed:

@cat started a discussion: (+3 comments)

Changed the order because of !74329 (comment 731837887) - to allow an extra ~10s before the first interval polling the API.

@nolith

can we think of another way to disable this behavior during tests?

Relying on timing during tests is really fragile. Even if the 10s is a huge time during a unit test, it's still not so great.

What if we implement a testing middleware that handles the "GET /api/v4/geo/proxy" and forwards everything else to the next testing handler?

@cat

Thanks @nolith - that would make sense, yeah, taking a look currently at what would be the best way, maybe changing startWorkhorseServerWithConfig, although it wouldn't really help since tests start their own upstream tests with httptest.NewServer directly so we'd need to update existing tests to do that.

Current options that I see would be either:

  1. Have a helper to generate upstream test servers and update all tests to use it, then configure the middleware as you mentioned there (currently, 32 calls to httptest.newServer through a grep)
  2. Potentially move the enableGeoProxyFeature to config.Config and make it configurable through the .toml (and/or Omnibus setting), then disable it for all tests explicitly in startWorkhorseServerWithConfig and only enable it in the feature-specific tests
  3. Expose the routesCallback from newUpstream (and implicitly the upstream.upstream type), then update startWorkhorseServerWithConfig to use that and add an explicit callback to skip Geo proxy API requests

I think 1 would be nice, though quite a change for this MR (could be done separately before, or in a follow-up) but would require future httptest server uses to use this helper instead; 2 would be more interesting, but not sure if best to just disable the feature for all tests (although in the end, implicitly, 1 and 3 would do the same)

WDYT about those options?

I'll consider giving a second shot to option 1

(currently, 32 calls to httptest.newServer through a grep)

I may be wrong, but I think we only care about the following ones (or even a subset of those)

sendfile_test.go 38: ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { 72: ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {

authorization_test.go 73: ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { 87: ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { 99: ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {

main_test.go 499: server := httptest.NewServer(tc.handler) 819: return httptest.NewServer(u) 976: originResourceServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {

Reason being, that we usually test single handlers inside internal and we only test the whole workhorse behaviour on the main package.