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

Closed
sjberman opened this issue Jan 23, 2024 · 6 comments · Fixed by #2175
Closed

Add unit tests for nginx runtime manager #1498

sjberman opened this issue Jan 23, 2024 · 6 comments · Fixed by #2175
Assignees
Labels
backlog Currently unprioritized work. May change with user feedback or as the product progresses. good first issue Good for newcomers refined Requirements are refined and the issue is ready to be implemented. tests Pull requests that update tests

Comments

@sjberman
Copy link
Collaborator

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
@miledxz
Copy link
Contributor

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

@miledxz
Copy link
Contributor

miledxz 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
Collaborator Author

sjberman commented Feb 6, 2024

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

@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
@mpstefan mpstefan removed this from the v1.4.0 milestone Jul 24, 2024
Copy link
Contributor

github-actions bot commented Aug 8, 2024

This issue 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 github-actions bot added the stale Pull requests/issues with no activity label Aug 8, 2024
Copy link
Contributor

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

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Aug 22, 2024
@sjberman sjberman reopened this Aug 22, 2024
@sjberman sjberman added backlog Currently unprioritized work. May change with user feedback or as the product progresses. and removed stale Pull requests/issues with no activity labels Aug 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog Currently unprioritized work. May change with user feedback or as the product progresses. 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
None yet
3 participants