Skip to content

🌱 Proposed way to unskip some tests - #874

Merged
tmshort merged 10 commits into
operator-framework:helm-pocfrom
bentito:helm-poc
May 29, 2024
Merged

🌱 Proposed way to unskip some tests#874
tmshort merged 10 commits into
operator-framework:helm-pocfrom
bentito:helm-poc

Conversation

@bentito

@bentitobentito commented May 20, 2024•
edited
Loading

Copy link
Copy Markdown
Contributor

Description

TestClusterExtensionChannelVersionExists mostly restored here.

This is a proposed way to change the other skipped tests as well.

Items skipped by the last conditional are:

  • Bundle Deployment related (can likely just be removed);
  • Invariant or other conditions on code paths we're avoiding due to a desire to lessen the mocking.

Reviewer Checklist

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

@bentito
bentito requested a review from a team as a code ownerMay 20, 2024 20:09
@bentitobentito changed the title Proposed way to unskip some tests🌱 Proposed way to unskip some testsMay 20, 2024
@bentitobentito mentioned this pull request May 20, 2024
Merged
4 tasks
bentito
bentito commented May 21, 2024
View reviewed changes
Comment threadinternal/controllers/clusterextension_controller_test.go Outdated
bentito
bentito commented May 21, 2024
View reviewed changes
Comment threadinternal/controllers/clusterextension_controller_test.go Outdated
bentito
bentito commented May 21, 2024
View reviewed changes
Comment threadinternal/controllers/clusterextension_controller_test.go Outdated
tmshort
tmshort reviewed May 21, 2024
View reviewed changes
Comment threadinternal/controllers/clusterextension_controller_test.go Outdated
tmshort
tmshort reviewed May 21, 2024
View reviewed changes
Comment threadinternal/controllers/suite_test.go Outdated
tmshort
tmshort reviewed May 21, 2024
View reviewed changes
Comment threadinternal/controllers/clusterextension_controller_test.go Outdated
varshaprasad96
varshaprasad96 reviewed May 21, 2024
View reviewed changes
Comment threadinternal/controllers/clusterextension_controller_test.go Outdated
Comment threadinternal/controllers/clusterextension_controller_test.go Outdated
Comment threadinternal/controllers/clusterextension_registryv1_validation_test.go
Comment threadinternal/controllers/suite_test.go
@varshaprasad96

varshaprasad96 commented May 21, 2024•
edited
Loading

Copy link
Copy Markdown
Member

@bentito: d649164 - I tried making changes to your branch, but GH doesn't let me create a PR against your repo 😅 With this change, we don't need to mock the loader itself. We use the mock unpacker to return UnpackPending and test relevant conditions which is equivalent on what the test did previously.

joelanford
joelanford reviewed May 21, 2024
View reviewed changes
Comment threadinternal/controllers/clusterextension_controller_test.go Outdated
joelanford
joelanford reviewed May 21, 2024
View reviewed changes
Comment threadinternal/controllers/clusterextension_controller_test.go
require.Equal(t, metav1.ConditionFalse, unpackedCond.Status)
require.Equal(t, rukpakv1alpha2.ReasonUnpackPending, unpackedCond.Reason)

verifyInvariants(ctx, t, reconciler.Client, clusterExtension)

@joelanfordjoelanfordMay 21, 2024•
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Curious why we're losing this verifyInvariants piece of the test?

@everettraveneverettravenMay 23, 2024

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.

Looks like verifyInvariants was removed from a few of the tests. I wasn't aware of what that function does so I went exploring. After learning what it's used for I agree that it should remain because IIUC we should be setting all of our status conditions on every reconcile.

After taking a look at the rest of the PR though, adding verifyInvariants back would be my only requested change (with the assumption that we currently expect some tests to be failing and will address them in another way).

@varshaprasad96varshaprasad96May 28, 2024•
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We have way more condition types and reasons now - so setting all of the conditions whenever the reconciler exits is an issue by itself. We are working on fixing this and brining verifyInvariants back for now. But we would strongly urge for us to evaluate the whole design of what conditions we want. #842 deals with some of it, but have also created an issue to track it.

@bentitobentitoMay 29, 2024

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.

verifyInvarients is restored here and everywhere now.

joelanford
joelanford reviewed May 21, 2024
View reviewed changes
Comment threadinternal/controllers/suite_test.go
testutil "github.com/operator-framework/operator-controller/test/util"
)

// MockUnpacker is a mock of Unpacker interface

@joelanfordjoelanfordMay 21, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: move the mocks to a separate package and/or file to make them easier to find?

@tmshorttmshortMay 29, 2024

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.

Added as #895.

@openshift-merge-robotopenshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 23, 2024
@varshaprasad96varshaprasad96 mentioned this pull request May 28, 2024
Closed
bentitoand others added 9 commits May 28, 2024 12:35
@bentito@dtfranz
Proposed way to unskip some tests
41b25a4
TestClusterExtensionChannelVersionExists mostly restored here
Signed-off-by: Brett Tofel <btofel@redhat.com>
@bentito@dtfranz
Refactors global variable names
50ab93e
Signed-off-by: Brett Tofel <btofel@redhat.com>
@bentito@dtfranz
Removes BundleDeployment related checking
0949f71
In just one test, for now.
Signed-off-by: Brett Tofel <btofel@redhat.com>
@varshaprasad96@dtfranz
fix unit test - TestClusterExtensionChannelVersionExists
6d008b9
@bentito@dtfranz
Unskip all - failing tests (up|down)grade should err
00c2a7b
Unskips all in clusterextension_controller_test.go
Signed-off-by: Brett Tofel <btofel@redhat.com>
@bentito@dtfranz
Unskip all clusterextension_registryv1_validation_test.go
39e42df
Still failing tests (up|down)grade should err b/c we don't have an installedBundle to check against
Signed-off-by: Brett Tofel <btofel@redhat.com>
@varshaprasad96@dtfranz
debugging
6caaf33
Signed-off-by: Varsha Prasad Narsing <varshaprasad96@gmail.com>
@bentito@dtfranz
Fixes and additions pending & unpack path
3b21c1d
Signed-off-by: Brett Tofel <btofel@redhat.com>
@dtfranz
Fix linter, remove debug logs and extraneous file
507c86e
Signed-off-by: dtfranz <dfranz@redhat.com>
@openshift-merge-robotopenshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 28, 2024
@dtfranz
Fix e2e failure due to race condition
c462f87
Signed-off-by: dtfranz <dfranz@redhat.com>
dtfranz
dtfranz reviewed May 28, 2024
View reviewed changes
Comment threadtest/e2e/cluster_extension_install_test.go
assert.Equal(ct, ocv1alpha1.ReasonSuccess, cond.Reason)
assert.Contains(ct, cond.Message, "resolved to")
assert.Equal(ct, &ocv1alpha1.BundleMetadata{Name: "prometheus-operator.1.0.0", Version: "1.0.0"}, clusterExtension.Status.ResolvedBundle)
assert.Equal(ct, &ocv1alpha1.BundleMetadata{Name: "prometheus-operator.1.0.0", Version: "1.0.0"}, clusterExtension.Status.InstalledBundle)

@dtfranzdtfranzMay 28, 2024

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.

If we don't actually check that this version has been installed, we may run into a scenario where we've resolved to 1.0.0 but haven't installed it yet. Then, if the user immediately changes the ClusterExtension to 1.2.0, we're essentially installing 1.2.0 from scratch, and the non-successor limitation doesn't apply in that case. I don't personally think this is a bug.

This check is not necessary on main because when we finish resolve the BundleDeployment is immediately created with the installed version flags present, regardless of whether or not the 1.0.0 operator has finished installing yet. I think this is actually less desirable compared to the above scenario; for instance if the user quickly changed their mind about which version they wanted they wouldn't be able to install 1.2.0 even though 1.0.0 hasn't even installed yet.

varshaprasad96
varshaprasad96 approved these changes May 28, 2024
View reviewed changes

@varshaprasad96varshaprasad96 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

/lgtm

@openshift-ciopenshift-ciBot added the lgtm Indicates that a PR is ready to be merged. label May 28, 2024
varshaprasad96
varshaprasad96 reviewed May 29, 2024
View reviewed changes
Comment threadinternal/controllers/common_controller.go
func updateStatusUnpackPending(status *ocv1alpha1.ClusterExtensionStatus, result *source.Result, generation int64) {
status.InstalledBundle = nil
apimeta.SetStatusCondition(&status.Conditions, metav1.Condition{
Type: rukpakv1alpha2.TypeUnpacked,

@varshaprasad96varshaprasad96May 29, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same for the other two unpack status conditions below.

Suggested change
Type: rukpakv1alpha2.TypeUnpacked,
Type: ocv1alpha1.TypeUnpacked,

@tmshorttmshortMay 29, 2024

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.

This is a status condition, didn't we say that another PR will deal with that stuff? (Although the above suggestion is correct.)

@tmshorttmshortMay 29, 2024

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.

#896

This was referenced May 29, 2024
Closed
Closed
@tmshort
tmshort merged commit 04df6fb into operator-framework:helm-pocMay 29, 2024
@tmshorttmshort mentioned this pull request May 29, 2024
Closed
github-merge-queueBot pushed a commit that referenced this pull request May 30, 2024
@bentito@tmshort
@dtfranz@varshaprasad96
✨ Modify ClusterExtension to use Helm under the hood to apply content…
0058054
…s into the cluster (#846)
* Helm POC rebase-a-thon
* Unpack Image (#811)
Builds and loads the unpacker binary into the operator-controller image so we don't need to use the rukpak image.
Signed-off-by: dtfranz <dfranz@redhat.com>
* ✨ OPRUN-3293 Fixes several unit tests - not cache ones (#831)
* Fix, rm or skip BundleDeployment related tests
Signed-off-by: Brett Tofel <btofel@redhat.com>
* Address lint errors
Signed-off-by: Brett Tofel <btofel@redhat.com>
---------
Signed-off-by: Brett Tofel <btofel@redhat.com>
* Fix e2e (#843)
Signed-off-by: Varsha Prasad Narsing <varshaprasad96@gmail.com>
* Make goreleaser build unpack
Signed-off-by: Brett Tofel <btofel@redhat.com>
* Rm commented code
Signed-off-by: Brett Tofel <btofel@redhat.com>
* Rebase required changes
Signed-off-by: Brett Tofel <btofel@redhat.com>
* Cleanups (#854)
Signed-off-by: Varsha Prasad Narsing <varshaprasad96@gmail.com>
* Expand comments in Dockerfile
Signed-off-by: Brett Tofel <btofel@redhat.com>
* Address Reviews
Signed-off-by: Varsha Prasad Narsing <varshaprasad96@gmail.com>
* Back out packagerrors
It's agreed that this makes debugging and reasoning about log messages and runtime errors much harder.
Signed-off-by: Brett Tofel <btofel@redhat.com>
* Helm POC e2e fixes (#870)
Fixes the e2e tests in helm-poc branch and restores feature-gate switching.
Signed-off-by: dtfranz <dfranz@redhat.com>
* Fix defer location (#883)
Fixes the defer location to make sure that all resources are cleaned up properly.
Signed-off-by: dtfranz <dfranz@redhat.com>
* Remove BundleDeployments gathering from e2e's gatherArtifacts (#886)
When tests fail, gatherArtifacts is run to collect cluster state. This PR removes gathering of BundleDeployments since we don't use them anymore and it's generating additional misleading error messages.
Signed-off-by: dtfranz <dfranz@redhat.com>
* Address reviews (#887)
Signed-off-by: Varsha Prasad Narsing <varshaprasad96@gmail.com>
* 🌱 Proposed way to unskip some tests (#874)
* Proposed way to unskip some tests
TestClusterExtensionChannelVersionExists mostly restored here
Signed-off-by: Brett Tofel <btofel@redhat.com>
* Refactors global variable names
Signed-off-by: Brett Tofel <btofel@redhat.com>
* Removes BundleDeployment related checking
In just one test, for now.
Signed-off-by: Brett Tofel <btofel@redhat.com>
* fix unit test - TestClusterExtensionChannelVersionExists
* Unskip all - failing tests (up|down)grade should err
Unskips all in clusterextension_controller_test.go
Signed-off-by: Brett Tofel <btofel@redhat.com>
* Unskip all clusterextension_registryv1_validation_test.go
Still failing tests (up|down)grade should err b/c we don't have an installedBundle to check against
Signed-off-by: Brett Tofel <btofel@redhat.com>
* debugging
Signed-off-by: Varsha Prasad Narsing <varshaprasad96@gmail.com>
* Fixes and additions pending & unpack path
Signed-off-by: Brett Tofel <btofel@redhat.com>
* Fix linter, remove debug logs and extraneous file
Signed-off-by: dtfranz <dfranz@redhat.com>
* Fix e2e failure due to race condition
Signed-off-by: dtfranz <dfranz@redhat.com>
---------
Signed-off-by: Brett Tofel <btofel@redhat.com>
Signed-off-by: Varsha Prasad Narsing <varshaprasad96@gmail.com>
Signed-off-by: dtfranz <dfranz@redhat.com>
Co-authored-by: Varsha Prasad Narsing <varshaprasad96@gmail.com>
Co-authored-by: dtfranz <dfranz@redhat.com>
* Revert goreleaser to use buildx
Signed-off-by: Brett Tofel <btofel@redhat.com>
* Revert all of goreleaser to use buildx
Signed-off-by: Brett Tofel <btofel@redhat.com>
* Revert errorf->printf & rm some logging in tests
Signed-off-by: Brett Tofel <btofel@redhat.com>
---------
Signed-off-by: dtfranz <dfranz@redhat.com>
Signed-off-by: Brett Tofel <btofel@redhat.com>
Signed-off-by: Varsha Prasad Narsing <varshaprasad96@gmail.com>
Co-authored-by: Todd Short <tshort@redhat.com>
Co-authored-by: Daniel Franz <dfranz@redhat.com>
Co-authored-by: Varsha <varshaprasad96@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@joelanfordjoelanfordjoelanford left review comments
@dtfranzdtfranzdtfranz left review comments
@tmshorttmshorttmshort left review comments
@varshaprasad96varshaprasad96varshaprasad96 approved these changes
+1 more reviewer
@everettraveneverettraveneverettraven left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@varshaprasad96varshaprasad96

Labels

lgtmIndicates that a PR is ready to be merged.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@bentito@varshaprasad96@joelanford@dtfranz@tmshort@everettraven@openshift-merge-robot