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

Test/add runtime manager tests #2175

Open
wants to merge 26 commits into
base: main
Choose a base branch
from

Conversation

miledxz
Copy link

@miledxz miledxz commented Jun 27, 2024

Proposed changes

Reopening pr trough another pr because issues on github

update on this: #1555, since I did mistake and discarded all commits,
I haven't find better solution so I opened another pr @sjberman @kate-osborn

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

Release notes

If this PR introduces a change that affects users and needs to be mentioned in the release notes,
please add a brief note that summarizes the change.


@miledxz miledxz requested a review from a team as a code owner June 27, 2024 10:30
@github-actions github-actions bot added the tests Pull requests that update tests label Jun 27, 2024
internal/mode/static/manager.go Outdated Show resolved Hide resolved
internal/mode/static/nginx/runtime/manager.go Outdated Show resolved Hide resolved
internal/mode/static/nginx/runtime/manager_test.go Outdated Show resolved Hide resolved
internal/mode/static/nginx/runtime/manager_test.go Outdated Show resolved Hide resolved
internal/mode/static/nginx/runtime/manager_test.go Outdated Show resolved Hide resolved
internal/mode/static/nginx/runtime/verify.go Outdated Show resolved Hide resolved
@miledxz miledxz force-pushed the test/add-runtime-manager-tests branch from 218715f to ec0048d Compare June 29, 2024 12:35
@miledxz miledxz requested a review from kate-osborn June 29, 2024 12:37
internal/mode/static/manager.go Outdated Show resolved Hide resolved
internal/mode/static/nginx/runtime/manager.go Outdated Show resolved Hide resolved
internal/mode/static/nginx/runtime/manager.go Outdated Show resolved Hide resolved
internal/mode/static/nginx/runtime/manager.go Outdated Show resolved Hide resolved
internal/mode/static/nginx/runtime/manager.go Outdated Show resolved Hide resolved
internal/mode/static/nginx/runtime/manager.go Outdated Show resolved Hide resolved
internal/mode/static/nginx/runtime/manager.go Outdated Show resolved Hide resolved
internal/mode/static/nginx/runtime/manager_test.go Outdated Show resolved Hide resolved
internal/mode/static/nginx/runtime/manager_test.go Outdated Show resolved Hide resolved
internal/mode/static/nginx/runtime/manager_test.go Outdated Show resolved Hide resolved
@miledxz miledxz force-pushed the test/add-runtime-manager-tests branch 4 times, most recently from da784fc to e0d5d1c Compare July 7, 2024 13:25
@miledxz miledxz force-pushed the test/add-runtime-manager-tests branch from e0d5d1c to 7f6d1bf Compare July 7, 2024 13:30
@miledxz miledxz force-pushed the test/add-runtime-manager-tests branch 2 times, most recently from 70302bd to 1d3108f Compare July 14, 2024 15:52
@miledxz miledxz force-pushed the test/add-runtime-manager-tests branch from 1d3108f to 6963fd4 Compare July 14, 2024 15:57
@miledxz miledxz requested a review from kate-osborn July 15, 2024 06:31
internal/mode/static/manager.go Outdated Show resolved Hide resolved
internal/mode/static/nginx/runtime/manager.go Outdated Show resolved Hide resolved
internal/mode/static/nginx/runtime/manager.go Outdated Show resolved Hide resolved
internal/mode/static/nginx/runtime/manager.go Outdated Show resolved Hide resolved
@mpstefan mpstefan removed this from the v1.4.0 milestone Jul 24, 2024
Copy link
Contributor

github-actions bot commented Aug 8, 2024

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 github-actions bot added stale Pull requests/issues with no activity and removed stale Pull requests/issues with no activity labels Aug 8, 2024
@miledxz miledxz force-pushed the test/add-runtime-manager-tests branch 2 times, most recently from 4daaeb0 to fe8d2fc Compare August 11, 2024 10:16
@miledxz
Copy link
Author

miledxz commented Aug 11, 2024

I just added another commit update here:
fe8d2fc

according to these:
#2175 (comment)
#2175 (comment)

#1555 (comment)
#1555 (comment)

gentle ping for @kate-osborn , I can add more updates if required

@miledxz miledxz force-pushed the test/add-runtime-manager-tests branch from fe8d2fc to 547e903 Compare August 11, 2024 11:00
Copy link

codecov bot commented Aug 20, 2024

Codecov Report

Attention: Patch coverage is 61.53846% with 10 lines in your changes missing coverage. Please review.

Project coverage is 89.26%. Comparing base (9a2c91b) to head (eda39b2).

Files Patch % Lines
internal/mode/static/manager.go 0.00% 4 Missing ⚠️
internal/mode/static/nginx/runtime/manager.go 75.00% 4 Missing ⚠️
internal/mode/static/nginx/runtime/verify.go 66.66% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2175      +/-   ##
==========================================
+ Coverage   88.84%   89.26%   +0.42%     
==========================================
  Files         100      100              
  Lines        7527     7535       +8     
  Branches       50       50              
==========================================
+ Hits         6687     6726      +39     
+ Misses        784      752      -32     
- Partials       56       57       +1     

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

@miledxz miledxz force-pushed the test/add-runtime-manager-tests branch from 1318e1b to 2626ca2 Compare August 20, 2024 17:03
internal/mode/static/nginx/runtime/manager.go Outdated Show resolved Hide resolved
internal/mode/static/nginx/runtime/manager.go Outdated Show resolved Hide resolved
@miledxz miledxz force-pushed the test/add-runtime-manager-tests branch from 2626ca2 to 8b94a05 Compare August 20, 2024 18:45
@miledxz
Copy link
Author

miledxz commented Aug 20, 2024

latest updates are in this commit: 8b94a05

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

var ngxPlusClient *ngxclient.NginxClient
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@miledxz can you change this type to nginxPlusClient interface defined in the nginx runtime package?

The functional tests fail because the function IsPlus on the nginx runtime manager returns true even when the nginxPlusClient is nil. This is due to a subtlety around interface types.

If you pass a nil *ngxclient.NginxClient as the type nginxPlusClient, it will not evaluate as nil. Take a look at this go playground example: https://goplay.tools/snippet/sDiRga5qSqm

We need to change this type to the interface nginxPlusClient type to fix this. This means you will need to export the nginxPlusClient interface and modify the collectors.NewNginxPlusMetricsCollector function to accept the interface instead of the actual client.

Apologies for not catching this sooner.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@miledxz there's a couple errors in the pipeline that need to be fixed:

cannot find package with target: nginxPlusClient
Writing `FakeNginxPlusClient` to `runtimefakes/fake_nginx_plus_client.go`... exit status 1
  Error: internal/mode/static/metrics/collectors/nginx.go:33:3: cannot use plusClient (variable of type "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/runtime".NginxPlusClient) as *"github.com/nginxinc/nginx-plus-go-client/client".NginxClient value in argument to nginxCollector.NewNginxPlusCollector: need type assertion (typecheck)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community tests Pull requests that update tests
Projects
Status: 🏗 In Progress
Development

Successfully merging this pull request may close these issues.

None yet

3 participants