nginx-gateway-fabric Add tests for runtime manager

trafficstars

Proposed changes

Problem: Missing tests on runtime manager

Im adding this init pr with few tests, if this goes well I can add another pr with more tests

gentle ping for @sjberman

Solution: Added tests and tested locally

Testing: running go test locally

optional: additional questions to clarify mocking and provide more details

Is practice for mocking runtime manager, more to use current usage with: mgr = NewManagerImpl(&ngxclient.NginxClient{}, nil, logr.New(GinkgoLogr.GetSink()))

or I can use already created mock from runtimefakes: runtimefakes.FakeManager as mocked struct, even tho its mocked interface ?

or its required to actually mock MetricsCollector in runtimefakes by creating runtimefakes.FakeMetricsCollector with counterfeiter and then use that one, in NewManagerImpl(&ngxclient.NginxClient{}, &runtmefakes.FakeMetricCollector{}, logr.New(GinkgoLogr.GetSink()))

or in

mockedManager := &runtimefakes.FakeManager{}

any advice on this is appreciated a lot, thank you in advance

Closes https://github.com/nginxinc/nginx-gateway-fabric/issues/1498

Checklist

Before creating a PR, run through this checklist and mark each as complete.

[X] I have read the CONTRIBUTING doc
[X] I have added tests that prove my fix is effective or that my feature works
[X] I have checked that all unit tests pass after adding my changes
[X] I have updated necessary documentation
[X] I have rebased my branch onto main
[X] I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

Feb 09 '24 12:02 miledxz

@zedGGs To answer your question about mocking, one of the goals of this work is to test out the ManagerImpl functions in manager.go. So this means we'll need to use the real ManagerImpl implementation, but be able to mock out the components that it uses, so we ensure we're just testing the ManagerImpl.

Feb 09 '24 15:02 sjberman

@sjberman sounds good, thank you for the feedback and review, Im going to proceed accordingly

Feb 10 '24 19:02 miledxz

@sjberman apologies for not being on time with an update,

current test cases for ManagerImpl are for situations where theres an error, I can say I had little issues to properly mock correct upstreams on nginx client, so I have not added these test cases, even tho I tried to do this, I can say I actually don't know how this works, any advice is appreciated

thank you in advance !

if you want you can take current changes and I can add separated pr for good test cases,

issue happens to be:

failed to update servers of unknown upstream: failed to get the HTTP servers of upstream unknown: failed to get http/upstreams/unknown/servers: Get "http://10.0.0.1:80/9/http/upstreams/unknown/servers": context deadline exceeded (Client.Timeout exceeded while awaiting headers)

			serversMock = ngxclient.UpstreamServer{
				ID:          1,
				Server:      "10.0.0.1:80",
				MaxConns:    &MaxConnsMock,
				MaxFails:    &MaxFailsMock,
				FailTimeout: "test",
				SlowStart:   "test",
				Route:       "test",
				Backup:      &BackupMock,
				Down:        &DownMock,
				Drain:       false,
				Weight:      &WeightMock,
				Service:     "",
			}

with ngxclient.NewNginxClient("10.0.0.1:80", client.WithHTTPClient(httpClient))

Feb 16 '24 19:02 miledxz

@zedGGs Thanks for the update. The issue you're seeing is that the nginx client is attempting to reach a real server, and fails. For unit tests we obviously do not want to use a real server. This means we probably need to create interfaces in https://github.com/nginxinc/nginx-plus-go-client, so we can provide a mock client in our own unit tests that won't try to contact a real server.

Feb 16 '24 20:02 sjberman

@sjberman here's an update, I have added NginxPlusClient to use UpdateHTTPServers and GetUpstreams since they are only being used currently, can add more improvements in test overall if you think something can be better

thanks

Feb 20 '24 06:02 miledxz

It would be nice if we could also find a way to test the Reload() function. May require temporary files and another client mock.

Feb 20 '24 20:02 sjberman

@kate-osborn sure, I am going to try add tests for that too thanks for feedback !!

Feb 21 '24 04:02 miledxz

This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 14 days.

Mar 12 '24 02:03 github-actions[bot]

This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 14 days.

Apr 04 '24 02:04 github-actions[bot]

This PR was closed because it has been stalled for 14 days with no activity.

Apr 18 '24 02:04 github-actions[bot]

Reopening after discussion with Zedd!

Apr 22 '24 16:04 mpstefan

Codecov Report

Attention: Patch coverage is 47.82609% with 12 lines in your changes missing coverage. Please review.

Project coverage is 87.30%. Comparing base (6f709fe) to head (b5ccf05). Report is 40 commits behind head on main.

Files	Patch %	Lines
internal/mode/static/nginx/runtime/manager.go	50.00%	6 Missing and 1 partial :warning:
internal/mode/static/manager.go	0.00%	3 Missing :warning:
internal/mode/static/nginx/runtime/verify.go	66.66%	2 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1555      +/-   ##
==========================================
+ Coverage   87.04%   87.30%   +0.26%     
==========================================
  Files          89       89              
  Lines        6096     6103       +7     
  Branches       50       50              
==========================================
+ Hits         5306     5328      +22     
+ Misses        737      716      -21     
- Partials       53       59       +6

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.