Skip to content
This repository has been archived by the owner on Jul 11, 2023. It is now read-only.

injector: Adding unit tests for the injector package #1772

Merged
merged 1 commit into from
Oct 9, 2020

Conversation

draychev
Copy link
Contributor

@draychev draychev commented Sep 30, 2020

This PR adds more unit test coverage for the pkg/injector

This PR also includes a rewrite of the updateAnnotation() function - to simplify and correct.
Previous iteration of this function returned add JSONPatchOperation with Value being sometimes string and other times map[string]string. The most recent iteration always creates add JSONPatchOperation with Value of map[string]string.

Before

$ go test -cover ./pkg/injector/...
ok      github.com/openservicemesh/osm/pkg/injector     0.058s  coverage: 27.6% of statements

After

$ go test -cover ./pkg/injector/...
ok      github.com/openservicemesh/osm/pkg/injector     6.821s  coverage: 75.7% of statements

Please mark with X for applicable areas.

  • New Functionality [ ]
  • Documentation [ ]
  • Install [ ]
  • Control Plane [ ]
  • CLI Tool [ ]
  • Certificate Management [ ]
  • Networking [ ]
  • Metrics [ ]
  • SMI Policy [ ]
  • Security [ ]
  • Tests / CI System [X]
  • Other [ ]

Please answer the following questions with yes/no.

  • Does this change contain code from or inspired by another project? If so, did you notify the maintainers and provide attribution? no

@draychev draychev added the wip Work-in-Progress label Sep 30, 2020
@draychev draychev linked an issue Sep 30, 2020 that may be closed by this pull request
15 tasks
@draychev draychev force-pushed the test-injector branch 4 times, most recently from 98b9c36 to f656d18 Compare October 1, 2020 00:16
@draychev draychev marked this pull request as ready for review October 1, 2020 00:36
@draychev draychev requested a review from a team as a code owner October 1, 2020 00:36
@shashankram
Copy link
Member

This PR also includes a rewrite of the updateAnnotation() function - to simplify and correct.
Previous iteration of this function returned add JSONPatchOperation with Value being sometimes string and other times map[string]string. The most recent iteration always creates add JSONPatchOperation with Value of map[string]string.

This isn't incorrect but rather as expected. The way the function works prior to this change is that if target is nil, then no annotations exist, and hence the patch value is a map, ie. a new annotation is added. If the annotation exists, then the patch no longer needs to create a new map, and only needs to update the existing object with the add map.

Copy link
Member

@shashankram shashankram left a comment

Choose a reason for hiding this comment

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

Thanks for adding the tests, some comments regarding the existing functionality.

pkg/injector/patch.go Outdated Show resolved Hide resolved
pkg/injector/patch.go Outdated Show resolved Hide resolved
pkg/injector/webhook.go Show resolved Hide resolved
@draychev draychev force-pushed the test-injector branch 3 times, most recently from 8400633 to d45cb71 Compare October 8, 2020 22:24
@codecov-io
Copy link

codecov-io commented Oct 8, 2020

Codecov Report

Merging #1772 into main will increase coverage by 3.70%.
The diff coverage is 93.02%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1772      +/-   ##
==========================================
+ Coverage   55.51%   59.21%   +3.70%     
==========================================
  Files         125      125              
  Lines        5163     5171       +8     
==========================================
+ Hits         2866     3062     +196     
+ Misses       2295     2106     -189     
- Partials        2        3       +1     
Impacted Files Coverage Δ
pkg/injector/webhook.go 73.48% <71.42%> (+39.96%) ⬆️
pkg/injector/patch.go 86.29% <97.05%> (+76.12%) ⬆️
pkg/injector/init-container.go 100.00% <100.00%> (ø)
pkg/envoy/route/config.go 95.83% <0.00%> (+0.83%) ⬆️
pkg/injector/config.go 93.91% <0.00%> (+16.52%) ⬆️
pkg/injector/volumes.go 100.00% <0.00%> (+100.00%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ff637fc...da4ec7b. Read the comment docs.

Copy link
Member

@shashankram shashankram left a comment

Choose a reason for hiding this comment

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

Looks correct overall, few comments and some linting issues to resolve.

pkg/injector/patch.go Outdated Show resolved Hide resolved
pkg/injector/patch.go Show resolved Hide resolved
pkg/injector/patch.go Show resolved Hide resolved
@draychev draychev force-pushed the test-injector branch 2 times, most recently from da4ec7b to 1809bd1 Compare October 9, 2020 18:34
@draychev draychev removed the wip Work-in-Progress label Oct 9, 2020
@draychev draychev merged commit 4352d3f into openservicemesh:main Oct 9, 2020
@draychev draychev deleted the test-injector branch October 9, 2020 18:59
@openservicemesh openservicemesh deleted a comment from shashankram Oct 9, 2020
kubeClient: client,
kubeController: mockNsController,
certManager: tresor.NewFakeCertManager(&cache, mockConfigurator),
meshCatalog: catalog.NewFakeMeshCatalog(client),
Copy link
Contributor

@eduser25 eduser25 Oct 9, 2020

Choose a reason for hiding this comment

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

You know we have a mock for catalog too right? >:)

eduser25 pushed a commit to eduser25/osm that referenced this pull request Oct 14, 2020
draychev added a commit to draychev/osm that referenced this pull request Oct 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(root) Improve test coverage (goal: 80%)
5 participants