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 !!

sjberman · 2024-02-26T15:18:13Z

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

+			err := manager.Reload(context.Background(), 1)
+			Expect(err).To(BeNil())


Suggested change

err := manager.Reload(context.Background(), 1)

Expect(err).To(BeNil())

Expect(manager.Reload(context.Background(), 1)).To(Succeed())

I would also verify that each of the other interface functions were called (WaitForCorrectVersionCallCount, SyscallKillCallCount, etc)

will do, thats good idea

sjberman · 2024-02-26T15:20:32Z

internal/mode/static/manager.go

+			nil,
+			nil,


These need to be initialized for the real thing.

@sjberman btw to update this properly I think I need for ProcessHandler interface, add ProcessHandlerImpl, if this is even good approach,

thank you !

sjberman · 2024-02-26T15:22:54Z

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

+type processHandler interface {
+	FindMainProcess(ctx context.Context, checkFile CheckFileFunc, readFile ReadFileFunc, timeout time.Duration) (int, error)
+	ReadFile(file string) ([]byte, error)
+	SysCallKill(pid int, signum syscall.Signal) error


I would just call this Kill.

sounds good

sjberman · 2024-02-26T15:42:40Z

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

 	logger           logr.Logger
+	process          processHandler


I'd call the variable processHandler as well

sure will do that

sjberman · 2024-02-26T15:43:25Z

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

+	"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/runtime"
+	"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/runtime/runtimefakes"


Separate these internal imports into a third category below the third-party imports.

sjberman · 2024-02-26T15:43:53Z

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

 )

-var _ = Describe("NGINX Runtime Manager", func() {
+var _ = Describe("NGINX Runtime Manager", Ordered, func() {


I don't believe these tests need to be Ordered, do they?

when I have used BeforeAll, I was required to use Ordered, but if I use BeforeEach, then its not required, if you agree on that I can make updates

BeforeEach is fine in this case

awesome, going to proceed accordingly

sjberman · 2024-02-26T15:45:13Z

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

+		upstreamServer = 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:     "test",
+		}


Are these fields actually necessary to fill out for the test? I'm assuming we could just use an empty list of servers when calling the API function

seems to be good without all of this stuff, so I will not fill these fields

sjberman · 2024-02-26T15:45:37Z

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

+			err := manager.UpdateHTTPServers("test", upstreamServers)
+
+			Expect(err).To(BeNil())


Suggested change

err := manager.UpdateHTTPServers("test", upstreamServers)

Expect(err).To(BeNil())

Expect(manager.UpdateHTTPServers("test", upstreamServers)).To(Succeed())

thanks for suggestion Im going to add an update

zedGGs · 2024-02-26T16:50:52Z

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

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


gentle ping @sjberman do you want me to add processHandlerImpl ? so I can use that outside, but then
func(p *ProcessHandlerImpl) FindMainProcess gets also updated, if this is even good approach, the goal is to cover these nil, nil that I used for real implementation

Yes, that would be necessary to create an implementation.

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.

sjberman · 2024-03-12T20:54:12Z

internal/mode/static/manager.go

@@ -212,6 +213,8 @@ func StartManager(cfg config.Config) error {
 			ngxPlusClient,
 			ngxruntimeCollector,
 			cfg.Logger.WithName("nginxRuntimeManager"),
+			&ngxruntime.ProcessHandlerImpl{},
+			ngxruntime.NewVerifyClient(1*time.Second),


This timeout should be the same as nginxReloadTimeout in the runtime manager. Let's export and use it.

sjberman · 2024-03-12T20:56:04Z

internal/mode/static/manager.go

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

 	// Ensure NGINX is running before registering metrics & starting the manager.
-	if err := ngxruntime.EnsureNginxRunning(ctx); err != nil {
+	if err := ngxruntime.EnsureNginxRunning(ctx, &ngxruntime.ProcessHandlerImpl{}); err != nil {


I think I'd rather initialize a single processHandler, make the EnsureNginxRunning function a method for that object, and then reuse the processHandler in the runtime manager initialization.

sjberman · 2024-03-12T20:56:12Z

internal/mode/static/manager.go

@@ -144,6 +144,7 @@ func StartManager(cfg config.Config) error {

 	var ngxPlusClient *ngxclient.NginxClient
 	var usageSecret *usage.Secret
+	//var processHandler *ngxruntime.ProcessHandler


unneeded line

sure, will add an update

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!

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

zedGGs force-pushed the test/add-runtime-manager-tests branch 2 times, most recently from f027f15 to dd4a219 Compare February 26, 2024 11:05

adding additional updates with Reload fnction tests

724f615

zedGGs force-pushed the test/add-runtime-manager-tests branch from dd4a219 to 724f615 Compare February 26, 2024 11:07

sjberman reviewed Feb 26, 2024

View reviewed changes

zedGGs commented Feb 26, 2024

View reviewed changes

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

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 2 commits May 6, 2024 17:36

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

7a2c352

adding fixes

837770a

		err := manager.Reload(context.Background(), 1)
		Expect(err).To(BeNil())

	err := manager.Reload(context.Background(), 1)
	Expect(err).To(BeNil())
	Expect(manager.Reload(context.Background(), 1)).To(Succeed())

		"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/runtime"
		"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/runtime/runtimefakes"

		err := manager.UpdateHTTPServers("test", upstreamServers)

		Expect(err).To(BeNil())


		//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

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

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

github-actions bot commented Mar 12, 2024

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

github-actions bot commented Apr 4, 2024

github-actions bot commented Apr 18, 2024

mpstefan commented Apr 22, 2024

zedGGs commented Feb 9, 2024 •

edited

zedGGs commented Feb 20, 2024 •

edited

zedGGs commented Feb 21, 2024 •

edited