Skip to content

🌱 Export variable fields - #505

Closed
m1kola wants to merge 1 commit into
operator-framework:mainfrom
m1kola:export_variable_fields
Closed

🌱 Export variable fields#505
m1kola wants to merge 1 commit into
operator-framework:mainfrom
m1kola:export_variable_fields

Conversation

@m1kola

@m1kolam1kola commented Nov 2, 2023

Copy link
Copy Markdown
Member

To do:

Description

Experiment to make testing a bit easier.

Reviewer Checklist

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

Signed-off-by: Mikalai Radchuk <mradchuk@redhat.com>
@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 Nov 2, 2023
deppy.Identifier("fake-catalog-test-package-test-package.v1.0.1"),
),
),
ID: "test-package package uniqueness",

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

This is not the best example, but I'm trying to find a nicier way to do these comparisons.

E.g. in #498 we do this:

gocmpopts:= []cmp.Option{
cmpopts.IgnoreUnexported(sync.RWMutex{}),
cmp.AllowUnexported(
sync.RWMutex{},
olmvariables.BundleVariable{},
input.SimpleVariable{},
constraint.DependencyConstraint{},
catalogmetadata.Bundle{},
),
cmpopts.IgnoreFields(
// Do not compare a func field
catalogmetadata.PackageRequired{}, "SemverRange",
),
}
require.Empty(t, cmp.Diff(bundles, expectedVariables, gocmpopts...))

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Haven't had enough time to review all of it - but you likely do not want to compare internals of sync.RWMutex - perhaps even of the others. For each, ask if it's possible or valid for the third-party package to change the implementation detail of unexported fields and if that would be a bona-fide concern for the caller here.

@codecov

codecovBot commented Nov 2, 2023

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (9c7291d) 83.92% compared to head (93ebbc9) 83.97%.
Report is 48 commits behind head on main.

Additional details and impacted files
@@ Coverage Diff @@## main #505 +/- ##
==========================================
+ Coverage 83.92% 83.97% +0.05% 
==========================================
Files 23 23 Lines 877 880 +3 ==========================================
+ Hits 736 739 +3 
Misses 96 96 Partials 45 45 
FlagCoverage Δ
e2e65.56% <100.00%> (+0.11%)⬆️
unit78.76% <100.00%> (+0.07%)⬆️

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.

@ncdcncdc changed the title Export variable fields🌱 Export variable fieldsNov 6, 2023
@openshift-merge-robotopenshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 16, 2023
@openshift-merge-robot

Copy link
Copy Markdown

PR needs rebase.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@m1kola

m1kola commented Feb 19, 2024

Copy link
Copy Markdown
MemberAuthor

Closing for now, but I'm hoping to come back to it at some point.

@m1kolam1kola closed this Feb 19, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do-not-merge/work-in-progressIndicates that a PR should not merge because it is a work in progress.needs-rebaseIndicates a PR cannot be merged because it has merge conflicts with HEAD.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@m1kola@openshift-merge-robot@stevekuznetsov