Skip to content

🌱 Internal Rukpak Package Tests - #1100

Merged
dtfranz merged 1 commit into
operator-framework:mainfrom
dtfranz:rukpak-source-util-tests
Aug 26, 2024
Merged

🌱 Internal Rukpak Package Tests#1100
dtfranz merged 1 commit into
operator-framework:mainfrom
dtfranz:rukpak-source-util-tests

Conversation

@dtfranz

@dtfranzdtfranz commented Aug 7, 2024

Copy link
Copy Markdown
Contributor

Adds more tests to the packages migrated from rukpak.

Closes#1037

Reviewer Checklist

  • API Go Documentation
  • Tests: Unit Tests (and E2E Tests, if appropriate)
  • Comprehensive Commit Messages
  • Links to related GitHub Issue(s)

@openshift-ciopenshift-ciBot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 7, 2024
@netlify

netlifyBot commented Aug 7, 2024

Copy link
Copy Markdown

Deploy Preview for olmv1 ready!

NameLink
🔨 Latest commit916f9e4
🔍 Latest deploy loghttps://app.netlify.com/sites/olmv1/deploys/66c36ba86567ba000839a751
😎 Deploy Previewhttps://deploy-preview-1100--olmv1.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@codecov

codecovBot commented Aug 7, 2024

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.85%. Comparing base (74959d0) to head (916f9e4).
Report is 12 commits behind head on main.

Additional details and impacted files
@@ Coverage Diff @@## main #1100 +/- ##
==========================================
+ Coverage 75.44% 76.85% +1.40% 
==========================================
Files 35 35 Lines 1918 1918 ==========================================
+ Hits 1447 1474 +27 + Misses 329 310 -19 + Partials 142 134 -8 
FlagCoverage Δ
e2e58.08% <ø> (-0.11%)⬇️
unit54.58% <ø> (+3.70%)⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

Comment threadinternal/rukpak/util/hash_test.go Outdated
@dtfranz
dtfranzforce-pushed the rukpak-source-util-tests branch from 88a0562 to 532801cCompareAugust 14, 2024 16:04
@dtfranz
dtfranz marked this pull request as ready for review August 14, 2024 16:04
@dtfranz
dtfranz requested a review from a team as a code ownerAugust 14, 2024 16:04
@openshift-ciopenshift-ciBot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 14, 2024
@dtfranzdtfranz changed the title 🌱 WIP: Internal Rukpak Tests🌱 Internal Rukpak Package TestsAug 14, 2024
@dtfranz
dtfranzforce-pushed the rukpak-source-util-tests branch from 532801c to 3ce9591CompareAugust 14, 2024 21:08
Comment threadinternal/rukpak/util/util_test.go
@dtfranz
dtfranzforce-pushed the rukpak-source-util-tests branch from 3ce9591 to 5f8df64CompareAugust 15, 2024 17:30
Comment threadinternal/rukpak/util/hash_test.go Outdated
@dtfranz
dtfranzforce-pushed the rukpak-source-util-tests branch 3 times, most recently from 08aef25 to 7d3377bCompareAugust 15, 2024 22:47
Adds more tests to the packages migrated from rukpak.
Signed-off-by: dtfranz <dfranz@redhat.com>
@dtfranz
dtfranzforce-pushed the rukpak-source-util-tests branch from 7d3377b to 916f9e4CompareAugust 19, 2024 15:58

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Tests look fine to me in this file.

I think it might be worth comparing what they look like in catalogd: https://github.com/operator-framework/catalogd/blob/main/internal/source/image_registry_client_test.go

It looks like there may be some more complexity in this set of tests for serving the manifests. Maybe we could simplify it by using the same method of standing up and populating an image registry for testing that catalogd does?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just to clarify, this is not something that I think should block this PR from going in. Just something that stood out to me during review and having the contextual knowledge that we implemented unit tests for similar functionality in catalogd a while back

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Thanks for showing me that! I missed that in there; I'll follow that example and see how much simplification can be done in here.

@everettraveneverettraven left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Overall looks good to me. Left a general comment for comparison of the image registry tests implemented here and in catalogd, but it doesn't need to be addressed for this PR to merge IMO.

@dtfranz

Copy link
Copy Markdown
ContributorAuthor

Overall looks good to me. Left a general comment for comparison of the image registry tests implemented here and in catalogd, but it doesn't need to be addressed for this PR to merge IMO.

It currently looks to me like I'll need to keep the low-level http response stuff in there for some of the error returns; I'll revisit this at another time and merge for now. Thank you!

@dtfranz
dtfranz added this pull request to the merge queue Aug 26, 2024
Merged via the queue into operator-framework:main with commit 162c4f1Aug 26, 2024
@skattojuskattoju mentioned this pull request Sep 25, 2024
4 tasks
Sign up for freeto 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.

Add unit test coverage for migrated rukpak packages

4 participants

@dtfranz@joelanford@LalatenduMohanty@everettraven