refactor prometheus test code #185
Conversation
surajssd
commented
Mar 18, 2020
•
surajssd
commented
- Create a prometheus client only once and then pass it to the tests that need it.
- This refactor will be consumed in prometheus: Test if all the endpoints are scraped #120 and metallb: Add alerts for metallb #140
| v1 "github.com/prometheus/client_golang/api/prometheus/v1" | ||
| ) | ||
|
|
||
| func TestPrometheus(t *testing.T) { |
There was a problem hiding this comment.
Hm, I think that reduces test readability/makes it more complex. It's fine to even have repetition in tests, they should be as straightforward as possible.
That also seems like a premature optimization, if we have no other tests to add at the moment.
If we're going to have more tests, I would suggest alternative approach. How about adding getPrometheusClient() function, which will return prometheus client? Then each test will just call this function at the beginning to get a client instance and then they can proceed on their own. I'm aware that this will create multiple instances of the client, but I don't think that should be a problem.
There was a problem hiding this comment.
That also seems like a premature optimization, if we have no other tests to add at the moment.
This is not premature optimization because it will be consumed in #120 and #140
I would suggest alternative approach. How about adding getPrometheusClient() function,
The problem with this approach is that then who takes care of closing the ports open for port forwarding? The whole point of the PR is to reuse the prometheus client. Since every port forward request consumes almost 3-4 seconds and for every test we cannot afford to have one. This reuses the client connection and the same function also takes care of closing the connection.
There was a problem hiding this comment.
This is not premature optimization because it will be consumed in #120 and #140
That has not been mentioned in original PR message. It make sense then.
The problem with this approach is that then who takes care of closing the ports open for port forwarding? The whole point of the PR is to reuse the prometheus client. Since every port forward request consumes almost 3-4 seconds and for every test we cannot afford to have one. This reuses the client connection and the same function also takes care of closing the connection.
That make sense then, but again, it has not been mentioned in the commit message nor in the PR message. Can we then please both tests in the single file, so they are easier to read? I would even consider flattening the test matrix, as nesting in twice seems really odd to me.
There was a problem hiding this comment.
Also, please document the reasoning of chosen approach in some comments to test test, so it's not confusing for other people too.
There was a problem hiding this comment.
Can we then please both tests in the single file, so they are easier to read?
I think keeping it in separate files is more readable IMHO because then it reduces the endless scroll. Also with growing number of things we add to the test the files ought to increase in length.
I would even consider flattening the test matrix, as nesting in twice seems really odd to me.
I can flatten it yes.
There was a problem hiding this comment.
I think keeping it in separate files is more readable IMHO because then it reduces the endless scroll. Also with growing number of things we add to the test the files ought to increase in length.
The problem with 2 files is, that in order to understand the test, you need to open 2 files. As far as I remember, that should be avoided for simplicity, if things are related to each other. I wonder what other think about it.
There was a problem hiding this comment.
Things are not same, because in one we are testing what all metrics are loaded and in other we are seeing the scrape targets and in other we are looking at alerts.
surajssd
force-pushed
the
surajssd/refactor-prometheus-e2e-tests
branch
from
b24dd41
to
165004e
Compare
This commit adds a test function which initialises the prometheus client and then passes it to the prometheus tests that need it. This approach is preferable over creating a new port-forward for every test is because initialising a port forward takes around 3-4 seconds and this can significantly increase the total running time of tests. Reusing port forward connection and prometheus client saves time for test. And having a single place of creating port forward and client intialisation ensures that we close that connection gracefully. Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>
Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>
surajssd
force-pushed
the
surajssd/refactor-prometheus-e2e-tests
branch
from
165004e
to
c44f61a
Compare
There was a problem hiding this comment.
@surajssd what about flattening I mentioned. I still see TestPrometheus calling testControlPlanePrometheusMetrics and then this one also calls t.Run() I think that's way too complicated as for the tests...
I'll pull additional reviewers.
I would have called |
