[WIP] Added httptest for testing API

[geekodour]

This is very much wip.
see #485

• created a new temporary file api_refactored_test.go to show the new pattern using net/http/httptest
• removed all other tests in this new file keeping only doAlertManagers
• removed extra fields from apiTest
• removed URL and Do methods from the test file and using URL and Do methods from the actual client at /api/client.go
• renamed apiTest to apiTest_ and TestAPIs to TestAPIs_ to avoid name collision with the original api_test.go

To make it easy to understand I've demonstrated this with doAlertmanagers only which is for the /api/v1/alertmanagers endpoint.

Now I tried importing github.com/prometheus/prometheus/web/api/v1 but,

If I uncomment importing prometheus "github.com/prometheus/prometheus/web/api/v1" I get the following error:

➜  client_golang git:(refactorhttptests) ✗ go test -v -mod=vendor ./api/prometheus/v1/...
# github.com/prometheus/prometheus/discovery/dns
vendor/github.com/prometheus/prometheus/discovery/dns/dns.go:320:12: undefined: dns.ErrTruncated

# github.com/prometheus/prometheus/storage/tsdb
vendor/github.com/prometheus/prometheus/storage/tsdb/tsdb.go:144:3: unknown field 'WALFlushInterval' in struct literal of type tsdb.Options

# github.com/prometheus/prometheus/discovery/kubernetes
vendor/github.com/prometheus/prometheus/discovery/kubernetes/client_metrics.go:216:23: cannot use f (type *clientGoWorkqueueMetricsProvider) as type workqueue.MetricsProvider in argument to workqueue.SetProvider:
        *clientGoWorkqueueMetricsProvider does not implement workqueue.MetricsProvider (missing NewLongestRunningProcessorMicrosecondsMetric method)

FAIL    github.com/prometheus/client_golang/api/prometheus/v1 [build failed]

Please let me know if this pattern can be followed for the rest of the tests.

cc: @krasi-georgiev

[krasi-georgiev]

I was hoping we could use the actual web package from Prometheus as the server.

https://github.com/prometheus/prometheus/blob/a1f34bec2e6584a2fee9aec901f3157e3e12cbaa/web/web.go#L415

or if that would be too much trouble maybe run an actual Prometheus server in the background and run queries against that. The idea is that instead of mocking the server we should try to be as close as possible to an actual Prometheus server. This way any breaking changes would be caught quite easily.

[krasi-georgiev]

here is another example why we should try to use the actual api module or an actual prometheus server instead of mocking the responses.
#529

[geekodour]

yes I'll try using the /web package today.

I didn't figure out how to get rid of the errors I get when I try to import "github.com/prometheus/prometheus/web/api/v1" as mentioned in the original comment.

Othewise, recreating the ServeMux for each subtest should be alright (as in this PR) ?

[geekodour]

the prometheus/web package is usable now, the errors were due to wrong versions in go.mod I committed the vendor directory too. will try implementing it in the tests now.

[krasi-georgiev]

another approach would be to start an actual Prometheus server in the background and send api requests against it.

[geekodour]

@krasi-georgiev the /prometheus/web/api/v1 package is accessible so I don't think there will be a need to run an actual Prometheus server. I borrowed some lines from prometheus/web/api/v1/api_test.go .

Will be rewriting current tests now.

Let me know if I should commit the vendor packages in another PR, or should I even commit the vendor directory as prometheus/client_golang didn't have one before.

tests failing on go version < 10

[krasi-georgiev]

Thanks , the progress looks promising 👍 .
I am almost sure that for Prometheus libraries we don't use vendoring so no need to add anything. Just update the go.mod file if needed.

tests failing on go version < 10

why do you think that is?

[geekodour]

The commented functions call admin api endpoints. Should I use a fakeDB for these, as in here:

https://github.com/prometheus/prometheus/blob/7f7b21104734fd1c7a5d2b3bc812da96cd7c35a3/web/api/v1/api_test.go#L1006-L1020

[krasi-georgiev]

yes I think this will do.

[geekodour]

why do you think that is?

It's probably something related to go modules, it's failing on v10 once I removed the vendor directory from the commit. Now only passes on v11.

@krasi-georgiev

[beorn7]

After #533 is merged, tests should pass again.

[simonpasquier]

The key problem with unit tests for Go 1.10 (and before) are the dependencies which are both included by this project and vendored in github.com/prometheus/prometheus (see prometheus/prometheus#1720). Also without Go modules, we can't enforce which version of the github.com/prometheus/prometheus/... packages will be used.

Besides these limitations, it feels a bit awkward for this module to depend on github.com/prometheus/prometheus even if it is only for testing. Having integration tests against a real Prometheus instance makes more sense IMO.

[krasi-georgiev]

@simonpasquier yeah very valid point, thanks for the input.
I completely forgot about that.

Can't think of another solution but starting some Prometheus server in the background.

[geekodour]

@krasi-georgiev in that case I think we should close this PR. I'll make another one that will send requests to an actual prometheus. I can add a comment on the top of the test that a local prometheus server should be running for this test. the test will fail if it does not detect a running prometheus server at 9090. is that a possible approach?

[krasi-georgiev]

Yeah a new PR is fine, but the test should be fully automated or we can't run it in the CI.

[simonpasquier]

@geekodour my take would be to deploy a Prometheus instance in the Travis CI infrastructure (you can run Docker containers within Travis) and skip the tests unless the TRAVIS environment variable is true.

[geekodour]

@krasi-georgiev @simonpasquier okay. looking into it. thanks for the pointers.