Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add unit tests for nginx runtime manager #1498

Open
sjberman opened this issue Jan 23, 2024 · 4 comments · May be fixed by #1555
Open

Add unit tests for nginx runtime manager #1498

sjberman opened this issue Jan 23, 2024 · 4 comments · May be fixed by #1555
Assignees
Labels
good first issue Good for newcomers refined Requirements are refined and the issue is ready to be implemented. tests Pull requests that update tests
Milestone

Comments

@sjberman
Copy link
Contributor

sjberman commented Jan 23, 2024

As a developer,
I want my code to be thoroughly unit tested,
so that I can ensure that everything works as expected.

The nginx runtime manager is lacking in unit test coverage.

Acceptance

  • Add unit tests to thoroughly cover the nginx runtime manager code

Dev notes

  • will likely need mocks for nginx plus client and "verifyClient"
  • use ginkgo since we're testing interfaces
@sjberman sjberman added tests Pull requests that update tests good first issue Good for newcomers labels Jan 23, 2024
@zedGGs
Copy link

zedGGs commented Feb 5, 2024

Heey @sjberman, feel free to assign me, would be cool to add some tests there !

@mpstefan mpstefan added this to the v1.2.0 milestone Feb 5, 2024
@mpstefan mpstefan added the refined Requirements are refined and the issue is ready to be implemented. label Feb 5, 2024
@mpstefan
Copy link
Collaborator

mpstefan commented Feb 5, 2024

Hey @zedGGs, thanks for the help on this. Assigned the issue to you and will pull your PR into the sprint when we see it!

Let us know if you need any help!

@zedGGs
Copy link

zedGGs commented Feb 6, 2024

Heey @mpstefan, awesome stuff !

There's one thing, adding tests for interfaces, for example this one:
https://github.com/nginxinc/nginx-gateway-fabric/blob/main/internal/mode/static/nginx/runtime/manager.go#L49

that's actually being used in ManagerImpl, do you prefer using counterfeiter here ? Or to manually mock this interface,
I have checked few other examples, maybe this one: https://github.com/nginxinc/nginx-gateway-fabric/blob/main/internal/framework/events/first_eventbatch_preparer.go#L38,

and then in tests theres is a fakeReader, but still not sure does these examples correlate,

Thank you in advance

@sjberman
Copy link
Contributor Author

sjberman commented Feb 6, 2024

@zedGGs Feel free to use counterfeiter to mock any interfaces that need it.

@zedGGs zedGGs linked a pull request Feb 9, 2024 that will close this issue
6 tasks
@mpstefan mpstefan modified the milestones: v1.2.0, v1.3.0 Mar 6, 2024
@mpstefan mpstefan modified the milestones: v1.3.0, v1.4.0 May 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers refined Requirements are refined and the issue is ready to be implemented. tests Pull requests that update tests
Projects
Status: 🏗 In Progress
Development

Successfully merging a pull request may close this issue.

3 participants