Add tests for runtime manager

[miledxz]

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 #1498

Checklist

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

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

[sjberman]

@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.

[miledxz]

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

[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))

[sjberman]

@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.

[miledxz]

@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

[sjberman]

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

[miledxz]

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

[github-actions]

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.

[github-actions]

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

[mpstefan]

Reopening after discussion with Zedd!

[codecov]

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 ⚠️
internal/mode/static/manager.go	0.00%	3 Missing ⚠️
internal/mode/static/nginx/runtime/verify.go	66.66%	2 Missing ⚠️

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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[kate-osborn]

Let's create processHandler here right before it's used.

[miledxz]

sounds amazing

[kate-osborn]

Let's use the following structure when testing for panics:

When("MetricsCollector is nil")
It("panics")

[miledxz]

executed like a great idea

[kate-osborn]

I think this is a different test case from the panics in the blocks below. We really want to test reload failures, which happens in the following cases:

• failed to find main process
• failed to read file
• failed to send kill signal
• timed out waiting for correct version

I think we want a test for each one of these cases. You can configure the mocks to return errors to trigger these cases.

[kate-osborn]

I think we also need a test for the success case. Let's add upstream servers to the mock and test that they are returned by manager.GetUpstreams

[kate-osborn]

Let's also check that we called the mock's UpdateHTTPServers method here with the input "test"

[sjberman]

#1990 was just merged that changes how we specify these generation statements now, take a look.

[github-actions]

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.

[miledxz]

Reopen please (don't know does this works)

[kate-osborn]

@zedGGs looks like I can't reopen. The button is greyed out.

Are you able to reopen?

[sarthyparty]

It says this on the commits page. Maybe try pushing a commit?