Add tests for runtime manager #1555

zedGGs · 2024-02-09T12:34:37Z

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

internal/mode/static/nginx/runtime/runtime_manager_test.go

sjberman · 2024-02-09T15:23:56Z

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

zedGGs · 2024-02-10T19:34:28Z

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

zedGGs · 2024-02-16T19:50:12Z

@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 · 2024-02-16T20:02:57Z

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

internal/mode/static/nginx/runtime/manager_test.go

zedGGs · 2024-02-20T06:12:52Z

@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

internal/mode/static/nginx/runtime/manager.go

internal/mode/static/nginx/runtime/manager_test.go

internal/mode/static/nginx/runtime/runtime_manager_test.go

sjberman · 2024-02-20T20:39:05Z

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

zedGGs · 2024-02-21T04:21:12Z

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

github-actions · 2024-03-12T02:33:40Z

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.

internal/mode/static/manager.go

github-actions · 2024-04-04T02:35:02Z

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 · 2024-04-18T02:36:20Z

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

mpstefan · 2024-04-22T16:12:23Z

Reopening after discussion with Zedd!

internal/mode/static/manager.go

internal/mode/static/nginx/runtime/manager.go

internal/mode/static/nginx/runtime/manager_test.go

codecov · 2024-05-20T15:38:15Z

Codecov Report

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

Project coverage is 87.30%. Comparing base (6f709fe) to head (b5ccf05).

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 · 2024-05-20T17:26:02Z

internal/mode/static/nginx/runtime/verify.go

-type verifyClient struct {
+//go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 . verifyClient
+
+type verifyClient interface {


Suggested change

type verifyClient interface {

type nginxConfigVerifier interface {

Then we can keep the struct names verifyClient and avoid adding Impl to the name, which is technically an anti-pattern.

kate-osborn · 2024-05-20T18:04:56Z

internal/mode/static/manager.go

@@ -134,7 +137,7 @@ func StartManager(cfg config.Config) error {
 	}



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

kate-osborn · 2024-05-20T18:08:35Z

internal/mode/static/nginx/runtime/manager.go

@@ -17,18 +17,36 @@ import (
 )

 const (
-	pidFile            = "/var/run/nginx/nginx.pid"
+	PidFile            = "/var/run/nginx/nginx.pid"


Let's add comments to all the newly exported variables and types.

kate-osborn · 2024-05-20T18:12:18Z

internal/mode/static/nginx/runtime/manager.go

+
+//go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 . ProcessHandler
+
+type ProcessHandler interface {


I think we can simplify this interface.

type ProcessHandler interface { FindMainProcess(ctx context.Context, timeout time.Duration) (int, error) ReadFile(file string) ([]byte, error) Kill(pid int) error } type ProcessHandlerImpl struct { readFile ReadFileFunc checkFile CheckFileFunc } func NewProcessHandlerImpl(readFileFunc ReadFileFunc, checkFileFunc CheckFileFunc) *ProcessHandlerImpl { return &ProcessHandlerImpl{readFile: readFileFunc, checkFile: checkFileFunc} } func (p *ProcessHandlerImpl) ReadFile(file string) ([]byte, error) { return p.readFile(file) } func (p *ProcessHandlerImpl) Kill(pid int) error { return syscall.Kill(pid, syscall.SIGHUP) } ...

Other changes:

FindMainProcess will use the readFile and checkFile funcs that are stored on ProcessHandlerImpl.

EnsureNginxRunning method is removed. In static/manager.go replace call to EnsureNginxRunning with processHandler.FindMainProcess.

I also don't think ProcessHandler needs to be exported since it's not used outside of this file.

kate-osborn · 2024-05-20T18:35:52Z

internal/mode/static/nginx/runtime/manager_test.go

+			Expect(process.KillCallCount()).To(Equal(1))
+			Expect(metrics.IncReloadCountCallCount()).To(Equal(1))
+			Expect(verifyClient.WaitForCorrectVersionCallCount()).To(Equal(1))
+			Expect(metrics.ObserveLastReloadTimeCallCount()).To(Equal(1))


let's also check that the IncReloadErrorsCallCount is 0.

kate-osborn · 2024-05-20T18:37:34Z

internal/mode/static/nginx/runtime/manager_test.go

+		})
+
+		When("NGINX configuration reload is not successful", func() {
+			It("should panic if MetricsCollector not enabled", func() {


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

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

kate-osborn · 2024-05-20T18:40:46Z

internal/mode/static/nginx/runtime/manager_test.go

+			Expect(metrics.ObserveLastReloadTimeCallCount()).To(Equal(1))
+		})
+
+		When("NGINX configuration reload is not successful", func() {


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 · 2024-05-20T18:43:02Z

internal/mode/static/nginx/runtime/manager_test.go

+			Expect(manager.UpdateHTTPServers("test", upstreamServers)).To(Succeed())
+		})
+
+		It("returns no upstreams from NGINX Plus API", func() {


We are missing a test case here. This test covers the case where the upstreams are nil, but we still need to cover the case where the ngxPlusClient returns an error for GetUpstreams:

nginx-gateway-fabric/internal/mode/static/nginx/runtime/manager.go

Line 142 in 6f709fe

upstreams, err := m.ngxPlusClient.GetUpstreams()

You can do this by configuring the mock to return an error for the method GetUpstreams

kate-osborn · 2024-05-20T18:45:24Z

internal/mode/static/nginx/runtime/manager_test.go

+		It("successfully updates HTTP server upstream", func() {
+			Expect(manager.UpdateHTTPServers("test", upstreamServers)).To(Succeed())
+		})
+


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 · 2024-05-20T18:46:31Z

internal/mode/static/nginx/runtime/manager_test.go

+		})
+
+		It("successfully updates HTTP server upstream", func() {
+			Expect(manager.UpdateHTTPServers("test", upstreamServers)).To(Succeed())


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

sjberman · 2024-05-20T20:32:08Z

internal/mode/static/nginx/runtime/manager.go

 )

 var childProcPathFmt = "/proc/%[1]v/task/%[1]v/children"

+//go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 . nginxPlusClient


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

zedGGs requested a review from a team as a code owner February 9, 2024 12:34

github-actions bot added the tests Pull requests that update tests label Feb 9, 2024

add tests for runtime manager

99997c4

zedGGs force-pushed the test/add-runtime-manager-tests branch from ec34013 to 99997c4 Compare February 9, 2024 12:58

sjberman reviewed Feb 9, 2024

View reviewed changes

internal/mode/static/nginx/runtime/runtime_manager_test.go Outdated Show resolved Hide resolved

pleshakov assigned sjberman Feb 12, 2024

zedGGs added 2 commits February 14, 2024 18:04

Merge branch 'nginxinc:main' into test/add-runtime-manager-tests

3a8c115

Merge branch 'nginxinc:main' into test/add-runtime-manager-tests

ba60c5d

zedGGs requested a review from sjberman February 16, 2024 19:36

adding required tests update

b36adc6

zedGGs force-pushed the test/add-runtime-manager-tests branch from b84583c to b36adc6 Compare February 16, 2024 19:56

sjberman reviewed Feb 16, 2024

View reviewed changes

internal/mode/static/nginx/runtime/manager_test.go Outdated Show resolved Hide resolved

sjberman reviewed Feb 16, 2024

View reviewed changes

internal/mode/static/nginx/runtime/manager_test.go Outdated Show resolved Hide resolved

zedGGs requested a review from sjberman February 20, 2024 06:09

test update for ManagerImpl

0b42d66

zedGGs force-pushed the test/add-runtime-manager-tests branch from 4663750 to 0b42d66 Compare February 20, 2024 06:21

sjberman reviewed Feb 20, 2024

View reviewed changes

lucacome added the community label Feb 23, 2024

Merge branch 'nginxinc:main' into test/add-runtime-manager-tests

8c396cd

zedGGs force-pushed the test/add-runtime-manager-tests branch from f7e7508 to 6532721 Compare February 26, 2024 10:58

zedGGs requested review from sjberman and kate-osborn February 26, 2024 11:00

zedGGs force-pushed the test/add-runtime-manager-tests branch from 6532721 to 06c586f Compare February 26, 2024 11:01

github-actions bot added the stale Pull requests/issues with no activity label Mar 12, 2024

zedGGs and others added 2 commits March 12, 2024 19:08

Merge branch 'nginxinc:main' into test/add-runtime-manager-tests

a3e2879

adding required updates

293272e

zedGGs requested a review from sjberman March 12, 2024 18:46

sjberman removed the stale Pull requests/issues with no activity label Mar 12, 2024

sjberman reviewed Mar 12, 2024

View reviewed changes

internal/mode/static/manager.go Outdated Show resolved Hide resolved

internal/mode/static/manager.go Outdated Show resolved Hide resolved

internal/mode/static/manager.go Outdated Show resolved Hide resolved

mpstefan added this to the v1.3.0 milestone Mar 20, 2024

github-actions bot added the stale Pull requests/issues with no activity label Apr 4, 2024

github-actions bot closed this Apr 18, 2024

mpstefan reopened this Apr 22, 2024

sjberman removed the stale Pull requests/issues with no activity label Apr 22, 2024

zedGGs and others added 4 commits May 6, 2024 17:36

Merge branch 'nginxinc:main' into test/add-runtime-manager-tests

7a2c352

adding fixes

837770a

Merge branch 'nginxinc:main' into test/add-runtime-manager-tests

2d30681

adding processHandler struct update

8d57c25

zedGGs force-pushed the test/add-runtime-manager-tests branch from a172d77 to 8d57c25 Compare May 20, 2024 14:48

zedGGs requested a review from sjberman May 20, 2024 14:50

sjberman reviewed May 20, 2024

View reviewed changes

adding cleanup fix update

1e09248

zedGGs requested a review from sjberman May 20, 2024 15:22

zedGGs and others added 3 commits May 20, 2024 17:58

adding gofumpt fix update

7f0f76e

Merge branch 'nginxinc:main' into test/add-runtime-manager-tests

e8bc9ef

adding additional fix updates

b5ccf05

kate-osborn requested changes May 20, 2024

View reviewed changes

sjberman reviewed May 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tests for runtime manager #1555

Add tests for runtime manager #1555

zedGGs commented Feb 9, 2024 •

edited

sjberman commented Feb 9, 2024

zedGGs commented Feb 10, 2024

zedGGs commented Feb 16, 2024

sjberman commented Feb 16, 2024

zedGGs commented Feb 20, 2024 •

edited

sjberman commented Feb 20, 2024

zedGGs commented Feb 21, 2024 •

edited

github-actions bot commented Mar 12, 2024

github-actions bot commented Apr 4, 2024

github-actions bot commented Apr 18, 2024

mpstefan commented Apr 22, 2024

codecov bot commented May 20, 2024 •

edited

kate-osborn May 20, 2024

kate-osborn May 20, 2024

kate-osborn May 20, 2024

kate-osborn May 20, 2024

kate-osborn May 20, 2024

kate-osborn May 20, 2024

kate-osborn May 20, 2024

kate-osborn May 20, 2024

kate-osborn May 20, 2024

kate-osborn May 20, 2024

kate-osborn May 20, 2024

sjberman May 20, 2024

	type verifyClient interface {
	type nginxConfigVerifier interface {

		@@ -134,7 +137,7 @@ func StartManager(cfg config.Config) error {
		}


		//go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 . ProcessHandler

		type ProcessHandler interface {

Add tests for runtime manager #1555

Are you sure you want to change the base?

Add tests for runtime manager #1555

Conversation

zedGGs commented Feb 9, 2024 • edited

Proposed changes

Checklist

sjberman commented Feb 9, 2024

zedGGs commented Feb 10, 2024

zedGGs commented Feb 16, 2024

sjberman commented Feb 16, 2024

zedGGs commented Feb 20, 2024 • edited

sjberman commented Feb 20, 2024

zedGGs commented Feb 21, 2024 • edited

github-actions bot commented Mar 12, 2024

github-actions bot commented Apr 4, 2024

github-actions bot commented Apr 18, 2024

mpstefan commented Apr 22, 2024

codecov bot commented May 20, 2024 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zedGGs commented Feb 9, 2024 •

edited

zedGGs commented Feb 20, 2024 •

edited

zedGGs commented Feb 21, 2024 •

edited

codecov bot commented May 20, 2024 •

edited