Add balancer unit tests #2410
Add balancer unit tests #2410
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
/approve |
k8s-ci-robot
added
the
approved
|
@zrdaley thank you for doing this |
rootfs/etc/nginx/lua/balancer.lua
Outdated
| @@ -164,6 +167,9 @@ function _M.call() | |||
| ngx_balancer.set_more_tries(1) | |||
|
|
|||
| local host, port = balance() | |||
| if not host then | |||
| return error("balancer did not return a host") | |||
There was a problem hiding this comment.
Instead of returning error please set the ngx.status to ngx.HTTP_SERVICE_UNAVAILABLE and log the error - this function is not expected to return anything
There was a problem hiding this comment.
hmm it actually does return something - but still when we can not find any upstream to proxy to we should return HTTP 503(ngx.HTTP_SERVICE_UNAVAILABLE)
| local ewma_after_balance_spy = spy.on(mock_ewma, "after_balance") | ||
|
|
||
| assert.has_no_errors(balancer.call) | ||
| assert.spy(backend_get_spy).was_called_with(match.is_table(), "mock_ewma_backend") |
There was a problem hiding this comment.
This does not add any value IMHO and is leaking internal knowledge about implementation.
| local ewma_after_balance_spy = spy.on(mock_ewma, "after_balance") | ||
|
|
||
| assert.has_no_errors(balancer.call) | ||
| assert.spy(backend_get_spy).was_called_with(match.is_table(), "mock_rr_backend") |
| mock_backends._vals = {} | ||
|
|
||
| local backend_get_spy = spy.on(mock_backends, "get") | ||
| local ngx_spy = spy.on(_G.ngx, "log") |
There was a problem hiding this comment.
this does not seem to be used anywhere
There was a problem hiding this comment.
Great first step toward Lua unit test coverage! @zrdaley had just a few small comments, let's address them then it is good to merge.
zrdaley
force-pushed
the
balancer-tests
branch
2 times, most recently
from
b416ec9
to
9c6bae0
Compare
|
@ElvinEfendi I addressed all of your concerns. I was slightly confused about your comments in balancer.lua, so I set ngx.status to HTTP_SERVICE_UNAVAILABLE and also returned an error message. |
Codecov Report
@@ Coverage Diff @@
## master #2410 +/- ##
==========================================
- Coverage 40.97% 40.91% -0.06%
==========================================
Files 74 74
Lines 5252 5252
==========================================
- Hits 2152 2149 -3
- Misses 2807 2809 +2
- Partials 293 294 +1
Continue to review full report at Codecov.
|
rootfs/etc/nginx/lua/balancer.lua
Outdated
| @@ -164,6 +167,10 @@ function _M.call() | |||
| ngx_balancer.set_more_tries(1) | |||
|
|
|||
| local host, port = balance() | |||
| if not host then | |||
| ngx.status = ngx.HTTP_SERVICE_UNAVAILABLE | |||
| return error(ngx.HTTP_SERVICE_UNAVAILABLE .. ": service unavailable") | |||
There was a problem hiding this comment.
just return without anything
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aledbf, ElvinEfendi, zrdaley The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What this PR does / why we need it:
Currently no unit tests exist for ingress-nginx/rootfs/etc/nginx/lua/balancer.lua. This PR adds unit tests for the file.
Which issue this PR fixes: N/A