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

(techdebt): refactor catalog controller unit tests #196

Conversation

everettraven
Copy link
Collaborator

@everettraven everettraven commented Oct 10, 2023

Description:

  • Removes the HTTPServer feature gate as there is no other logic for serving catalog contents which means this should now be the default.
  • Refactors the Catalog controller unit tests to use pure Go testing + testify instead of Ginkgo.
  • Removes the use of envtest for the Catalog controller tests and instead targets the reconcile() function for unit testing.
    • This does result in a lower test coverage score, but the removal of envtest results in a ~90% decrease in time spent running the unit tests (on my M2 Macbook Air main runs tests for the pkg/controllers/core package in ~6 seconds and this branch runs in ~.5 seconds)
    • A future tech debt item could be to investigate refactoring the Reconcile() function to make it easier to test the logic without using envtest.
  • Removes the tests for the SetupWithManager() function as it wasn't really providing any value

Motivation

to no longer use Ginkgo and instead use
the native Go testing and testify

Signed-off-by: Bryce Palmer <everettraven@gmail.com>
@everettraven everettraven requested a review from a team as a code owner October 10, 2023 21:20
@codecov
Copy link

codecov bot commented Oct 10, 2023

Codecov Report

Attention: 23 lines in your changes are missing coverage. Please review.

Comparison is base (9f3ba06) 49.72% compared to head (f3788f7) 46.15%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #196      +/-   ##
==========================================
- Coverage   49.72%   46.15%   -3.58%     
==========================================
  Files           6        6              
  Lines         366      364       -2     
==========================================
- Hits          182      168      -14     
- Misses        163      177      +14     
+ Partials       21       19       -2     
Files Coverage Δ
pkg/controllers/core/catalog_controller.go 77.22% <100.00%> (-12.97%) ⬇️
cmd/manager/main.go 10.00% <0.00%> (+0.07%) ⬆️

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

// Having the parameterized fields allows us to easily change the values that are used in
// the tests by changing them in one place as opposed to manually changing many string literals
// throughout the code.
const testBundleTemplate = `---
Copy link
Member

Choose a reason for hiding this comment

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

Why was it valid to lose this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was necessary when the reconcile() function would call helper functions, passing the fs.FS that was returned from the source.Unpacker.Unpack() call, that would create the Package, BundleMetadata, and CatalogMetadata CRs. Since this was happening as part of the reconciliation loop we had to ensure our MockSource was returning an fs.FS with a proper set of FBC files. Since we no longer do that and the logic for storing catalog contents is now abstracted + mocked for these unit tests we can provide an empty fs.FS from our mock source.

This should instead be used when testing the storage logic directly and has already been duplicated over there previously:

const testBundleTemplate = `---
image: %s
name: %s
schema: olm.bundle
package: %s
relatedImages:
- name: %s
image: %s
properties:
- type: olm.bundle.object
value:
data: %s
- type: some.other
value:
data: arbitrary-info
`
const testPackageTemplate = `---
defaultChannel: %s
name: %s
schema: olm.package
`
const testChannelTemplate = `---
schema: olm.channel
package: %s
name: %s
entries:
- name: %s
`

…ocks. goimports.

Signed-off-by: Bryce Palmer <everettraven@gmail.com>
@stevekuznetsov stevekuznetsov added this pull request to the merge queue Oct 11, 2023
Merged via the queue into operator-framework:main with commit 9f91fc5 Oct 11, 2023
7 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants