From d31a8b43dbe8e6fe585d8791f65c3afd88587373 Mon Sep 17 00:00:00 2001 From: Jonathan Ogilvie Date: Tue, 10 Nov 2026 19:04:49 -0500 Subject: [PATCH 1/6] Add envtest integration tests for xpkg install Co-Authored-By: Claude Sonnet 5.5 Signed-off-by: Jonathan Ogilvie --- .../xpkg/install_integration_test.go | 341 ++++++++++++++++++ 1 file changed, 341 insertions(+) create mode 100644 cmd/crossplane/xpkg/install_integration_test.go diff --git a/cmd/crossplane/xpkg/install_integration_test.go b/cmd/crossplane/xpkg/install_integration_test.go new file mode 100644 index 00000000..112fe669 --- /dev/null +++ b/cmd/crossplane/xpkg/install_integration_test.go @@ -0,0 +1,341 @@ +//go:build integration + +/* +Copyright 2026 The Crossplane Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package xpkg + +import ( + "bytes" + "fmt" + "os" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/alecthomas/kong" + "github.com/google/go-cmp/cmp" + corev1 "k8s.io/api/core/v1" + extv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/utils/ptr" + "sigs.k8s.io/controller-runtime/pkg/client" + + "github.com/crossplane/crossplane-runtime/v2/pkg/logging" + + v1 "github.com/crossplane/crossplane/apis/v2/pkg/v1" + + "github.com/crossplane/cli/v2/internal/testutils/integration" +) + +func TestMain(m *testing.M) { + integration.ReapOrphanedEnvtest(func(format string, args ...any) { fmt.Fprintf(os.Stderr, format+"\n", args...) }) + os.Exit(m.Run()) +} + +// startInstallCluster starts a cluster serving Crossplane's CRDs and points KUBECONFIG, which install loads via +// ctrl.GetConfig, at it. +func startInstallCluster(t *testing.T) *integration.Cluster { + t.Helper() + + cluster := integration.Start(t, integration.Setup{}) + + kubeconfig := filepath.Join(t.TempDir(), "kubeconfig") + if err := os.WriteFile(kubeconfig, cluster.Kubeconfig, 0o600); err != nil { + t.Fatalf("cannot write kubeconfig: %v", err) + } + + t.Setenv("KUBECONFIG", kubeconfig) + + return cluster +} + +// runInstall parses args as `crossplane xpkg install` would and runs it, returning its stdout. +func runInstall(t *testing.T, args ...string) (string, error) { + t.Helper() + + var c installCmd + + parser, err := kong.New(&c) + if err != nil { + t.Fatalf("kong.New(): %v", err) + } + + kongCtx, err := parser.Parse(args) + if err != nil { + t.Fatalf("Parse(%v): %v", args, err) + } + + var stdout bytes.Buffer + kongCtx.Stdout = &stdout + + err = c.Run(kongCtx, logging.NewNopLogger()) + + return stdout.String(), err +} + +// TestInstallIntegration installs packages into a real API server and checks the objects read back from it. +func TestInstallIntegration(t *testing.T) { + cluster := startInstallCluster(t) + + type want struct { + obj v1.Package + spec v1.PackageSpec + rcName string + stdout string + errSub string + } + + auto, manual := v1.AutomaticActivation, v1.ManualActivation + + cases := map[string]struct { + reason string + args []string + want want + }{ + "Provider": { + reason: "A provider is created with automatic activation, no pull secrets and a zero history limit by default.", + args: []string{"provider", "xpkg.crossplane.io/crossplane-contrib/provider-nop:v0.4.0", "provider-a"}, + want: want{ + obj: &v1.Provider{}, + spec: v1.PackageSpec{ + Package: "xpkg.crossplane.io/crossplane-contrib/provider-nop:v0.4.0", + RevisionActivationPolicy: &auto, + RevisionHistoryLimit: ptr.To[int64](0), + }, + rcName: "default", // The CRD defaults it. + stdout: "provider/provider-a created\n", + }, + }, + "ConfigurationWithFlags": { + reason: "Manual activation, pull secrets and a history limit are set on a configuration's spec.", + args: []string{ + "configuration", "xpkg.crossplane.io/acme/config:v1.0.0", "config-a", + "--manual-activation", "--package-pull-secrets=s1,s2", "--revision-history-limit=3", + }, + want: want{ + obj: &v1.Configuration{}, + spec: v1.PackageSpec{ + Package: "xpkg.crossplane.io/acme/config:v1.0.0", + RevisionActivationPolicy: &manual, + RevisionHistoryLimit: ptr.To[int64](3), + PackagePullSecrets: []corev1.LocalObjectReference{{Name: "s1"}, {Name: "s2"}}, + }, + stdout: "configuration/config-a created\n", + }, + }, + "FunctionWithRuntimeConfig": { + reason: "A function references the runtime config it was installed with.", + args: []string{"function", "xpkg.crossplane.io/acme/fn:v1.0.0", "fn-a", "--runtime-config=rc"}, + want: want{ + obj: &v1.Function{}, + spec: v1.PackageSpec{ + Package: "xpkg.crossplane.io/acme/fn:v1.0.0", + RevisionActivationPolicy: &auto, + RevisionHistoryLimit: ptr.To[int64](0), + }, + rcName: "rc", + stdout: "function/fn-a created\n", + }, + }, + "ProviderWithRuntimeConfig": { + reason: "A provider references the runtime config it was installed with.", + args: []string{"provider", "xpkg.crossplane.io/acme/prov:v1.0.0", "provider-rc", "--runtime-config=rc"}, + want: want{ + obj: &v1.Provider{}, + spec: v1.PackageSpec{ + Package: "xpkg.crossplane.io/acme/prov:v1.0.0", + RevisionActivationPolicy: &auto, + RevisionHistoryLimit: ptr.To[int64](0), + }, + rcName: "rc", + stdout: "provider/provider-rc created\n", + }, + }, + "ConfigurationWithRuntimeConfig": { + reason: "A configuration has no runtime, so a runtime config is refused before anything is created.", + args: []string{"configuration", "xpkg.crossplane.io/acme/config:v1.0.0", "config-rc", "--runtime-config=rc"}, + want: want{errSub: "does not support runtime configuration"}, + }, + "DefaultName": { + reason: "Without a name the package is named for its repository, as a DNS label.", + args: []string{"function", "xpkg.crossplane.io/crossplane-contrib/function-patch-and-transform:v0.8.0"}, + want: want{ + obj: &v1.Function{ObjectMeta: metav1.ObjectMeta{Name: "crossplane-contrib-function-patch-and-transform"}}, + spec: v1.PackageSpec{ + Package: "xpkg.crossplane.io/crossplane-contrib/function-patch-and-transform:v0.8.0", + RevisionActivationPolicy: &auto, + RevisionHistoryLimit: ptr.To[int64](0), + }, + rcName: "default", + stdout: "function/crossplane-contrib-function-patch-and-transform created\n", + }, + }, + "AlreadyExists": { + reason: "Installing a package under a name that is taken fails.", + args: []string{"provider", "xpkg.crossplane.io/crossplane-contrib/provider-nop:v0.4.0", "provider-a"}, + want: want{errSub: "already exists"}, + }, + } + + // Cases share a cluster and AlreadyExists depends on Provider, so they run in order. + for _, name := range []string{ + "Provider", "ConfigurationWithFlags", "FunctionWithRuntimeConfig", "ProviderWithRuntimeConfig", + "ConfigurationWithRuntimeConfig", "DefaultName", "AlreadyExists", + } { + tc := cases[name] + t.Run(name, func(t *testing.T) { + stdout, err := runInstall(t, tc.args...) + + if tc.want.errSub != "" { + if err == nil || !strings.Contains(err.Error(), tc.want.errSub) { + t.Fatalf("\n%s\nRun(): want error containing %q, got %v", tc.reason, tc.want.errSub, err) + } + + return + } + + if err != nil { + t.Fatalf("\n%s\nRun(): %v", tc.reason, err) + } + + if diff := cmp.Diff(tc.want.stdout, stdout); diff != "" { + t.Errorf("\n%s\nstdout: -want, +got:\n%s", tc.reason, diff) + } + + got := tc.want.obj + key := client.ObjectKey{Name: got.GetName()} + if key.Name == "" { + key.Name = tc.args[2] + } + + if err := cluster.Client.Get(cluster.Context, key, got); err != nil { + t.Fatalf("\n%s\nGet(%s): %v", tc.reason, key.Name, err) + } + + gotSpec := v1.PackageSpec{ + Package: got.GetSource(), + RevisionActivationPolicy: got.GetActivationPolicy(), + RevisionHistoryLimit: got.GetRevisionHistoryLimit(), + PackagePullSecrets: got.GetPackagePullSecrets(), + } + if diff := cmp.Diff(tc.want.spec, gotSpec); diff != "" { + t.Errorf("\n%s\nspec: -want, +got:\n%s", tc.reason, diff) + } + + gotRC := "" + if rp, ok := got.(v1.PackageWithRuntime); ok && rp.GetRuntimeConfigRef() != nil { + gotRC = rp.GetRuntimeConfigRef().Name + } + + if gotRC != tc.want.rcName { + t.Errorf("\n%s\nruntimeConfigRef.name: want %q, got %q", tc.reason, tc.want.rcName, gotRC) + } + }) + } +} + +// TestInstallWaitIntegration checks that --wait returns once the package reports Healthy, and fails when it never +// does. +func TestInstallWaitIntegration(t *testing.T) { + cluster := startInstallCluster(t) + + cases := map[string]struct { + reason string + name string + healthy bool + wantErr bool + }{ + "BecomesHealthy": { + reason: "Run returns success once the package's Healthy condition turns true.", + name: "provider-healthy", + healthy: true, + }, + "NeverHealthy": { + reason: "Run fails when the package isn't healthy before the wait times out.", + name: "provider-unhealthy", + wantErr: true, + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + if tc.healthy { + // No package manager runs under envtest, so mark the package healthy once Run has created it. + go func() { + p := &v1.Provider{} + for cluster.Context.Err() == nil { + if err := cluster.Client.Get(cluster.Context, client.ObjectKey{Name: tc.name}, p); err == nil { + p.SetConditions(v1.Healthy()) + if err := cluster.Client.Status().Update(cluster.Context, p); err == nil { + return + } + } + + time.Sleep(100 * time.Millisecond) + } + }() + } + + start := time.Now() + _, err := runInstall(t, "provider", "xpkg.crossplane.io/acme/prov:v1.0.0", tc.name, "--wait=5s") + + if tc.wantErr { + if err == nil || !strings.Contains(err.Error(), "Package did not become ready") { + t.Fatalf("\n%s\nRun(): want a not-ready error, got %v", tc.reason, err) + } + + return + } + + if err != nil { + t.Fatalf("\n%s\nRun(): %v", tc.reason, err) + } + + if elapsed := time.Since(start); elapsed >= 5*time.Second { + t.Errorf("\n%s\nRun() returned after %s, want before the 5s wait elapsed", tc.reason, elapsed) + } + }) + } +} + +// TestInstallMissingCRDIntegration checks the error install returns when the cluster doesn't serve the package +// kind, such as one without Crossplane installed. +func TestInstallMissingCRDIntegration(t *testing.T) { + cluster := startInstallCluster(t) + + // Start always serves Crossplane's CRDs, so remove the one this case needs gone. + crd := &extv1.CustomResourceDefinition{ObjectMeta: metav1.ObjectMeta{Name: "providers.pkg.crossplane.io"}} + if err := cluster.Client.Delete(cluster.Context, crd); err != nil { + t.Fatalf("cannot delete the Provider CRD: %v", err) + } + + for cluster.Client.Get(cluster.Context, client.ObjectKeyFromObject(crd), crd) == nil { + time.Sleep(100 * time.Millisecond) + } + + // The client's RESTMapper fails before any request, so the error is a no-match, not the 404 warnIfNotFound + // annotates with a hint that the CLI might be out of date. + _, err := runInstall(t, "provider", "xpkg.crossplane.io/acme/prov:v1.0.0", "provider-missing") + if err == nil || !strings.Contains(err.Error(), `no matches for kind "Provider"`) { + t.Fatalf("Run(): want a no-match error for the Provider kind, got %v", err) + } + + if strings.Contains(err.Error(), "might be out of date") { + t.Errorf("Run(): warnIfNotFound unexpectedly annotated a no-match error: %v", err) + } +} From 7c787bb35c0d63d99b11df44b442403d65e47178 Mon Sep 17 00:00:00 2001 From: Jonathan Ogilvie Date: Tue, 10 Nov 2026 19:04:49 -0500 Subject: [PATCH 2/6] Add envtest integration tests for FetchCrossplaneVersion Co-Authored-By: Claude Sonnet 5.5 Signed-off-by: Jonathan Ogilvie --- .../version/fetch_integration_test.go | 157 ++++++++++++++++++ 1 file changed, 157 insertions(+) create mode 100644 cmd/crossplane/version/fetch_integration_test.go diff --git a/cmd/crossplane/version/fetch_integration_test.go b/cmd/crossplane/version/fetch_integration_test.go new file mode 100644 index 00000000..2d22ed98 --- /dev/null +++ b/cmd/crossplane/version/fetch_integration_test.go @@ -0,0 +1,157 @@ +//go:build integration + +/* +Copyright 2026 The Crossplane Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package version + +import ( + "fmt" + "os" + "path/filepath" + "strings" + "testing" + + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + "github.com/crossplane/cli/v2/internal/kube" + "github.com/crossplane/cli/v2/internal/testutils/integration" +) + +func TestMain(m *testing.M) { + integration.ReapOrphanedEnvtest(func(format string, args ...any) { fmt.Fprintf(os.Stderr, format+"\n", args...) }) + os.Exit(m.Run()) +} + +// deployment returns a Deployment in crossplane-system with the given labels and first container image. +func deployment(name string, labels map[string]string, image string) *appsv1.Deployment { + sel := map[string]string{"pod": name} + + return &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: "crossplane-system", Labels: labels}, + Spec: appsv1.DeploymentSpec{ + Selector: &metav1.LabelSelector{MatchLabels: sel}, + Template: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{Labels: sel}, + Spec: corev1.PodSpec{Containers: []corev1.Container{{Name: "c", Image: image}}}, + }, + }, + } +} + +// TestFetchCrossplaneVersionIntegration reads the Crossplane version from Deployments in a real API server. Each +// case gets its own cluster, since FetchCrossplaneVersion lists Deployments in every namespace. Cases run in turn +// because each sets KUBECONFIG. +func TestFetchCrossplaneVersionIntegration(t *testing.T) { + xp := map[string]string{"app": "crossplane"} + withVersion := func(v string) map[string]string { + return map[string]string{"app": "crossplane", "app.kubernetes.io/version": v} + } + + type want struct { + version string + errSub string + } + + cases := map[string]struct { + reason string + deployments []*appsv1.Deployment + want want + }{ + "VersionLabel": { + reason: "A version label without a leading v is returned with one.", + deployments: []*appsv1.Deployment{deployment("crossplane", withVersion("2.0.0"), "xpkg.crossplane.io/crossplane/crossplane:v9.9.9")}, + want: want{version: "v2.0.0"}, + }, + "VersionLabelWithV": { + // BUG: fetch.go only assigns version when it prepends a v, so a label that already starts with v + // returns "". This case asserts the current behavior; the correct result is "v2.0.0". + reason: "A version label that already has a leading v is (currently, wrongly) returned as an empty string.", + deployments: []*appsv1.Deployment{deployment("crossplane", withVersion("v2.0.0"), "xpkg.crossplane.io/crossplane/crossplane:v9.9.9")}, + want: want{version: ""}, + }, + "ImageTag": { + reason: "Without a version label, the image tag is returned with a leading v added.", + deployments: []*appsv1.Deployment{deployment("crossplane", xp, "xpkg.crossplane.io/crossplane/crossplane:2.1.0")}, + want: want{version: "v2.1.0"}, + }, + "ImageTagWithV": { + reason: "Without a version label, an image tag that has a leading v is returned unchanged.", + deployments: []*appsv1.Deployment{deployment("crossplane", xp, "xpkg.crossplane.io/crossplane/crossplane:v2.1.0")}, + want: want{version: "v2.1.0"}, + }, + "ImageDigest": { + reason: "An image referenced by digest has no tag, so no version is found.", + deployments: []*appsv1.Deployment{deployment("crossplane", xp, + "xpkg.crossplane.io/crossplane/crossplane@sha256:"+strings.Repeat("a", 64))}, + want: want{errSub: "Crossplane version or image tag not found"}, + }, + "NoDeployments": { + reason: "With no Deployments there is no version to find.", + want: want{errSub: "Crossplane version or image tag not found"}, + }, + "NoMatchingDeployments": { + reason: "Deployments without the app=crossplane label are ignored.", + deployments: []*appsv1.Deployment{deployment("other", map[string]string{"app": "other", "app.kubernetes.io/version": "2.0.0"}, "example.com/other:v1.0.0")}, + want: want{errSub: "Crossplane version or image tag not found"}, + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + cluster := integration.Start(t, integration.Setup{}) + + ns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "crossplane-system"}} + if err := cluster.Client.Create(cluster.Context, ns); err != nil { + t.Fatalf("cannot create namespace: %v", err) + } + + for _, d := range tc.deployments { + if err := cluster.Client.Create(cluster.Context, d); err != nil { + t.Fatalf("cannot create deployment %s: %v", d.GetName(), err) + } + } + + kubeconfig := filepath.Join(t.TempDir(), "kubeconfig") + if err := os.WriteFile(kubeconfig, cluster.Kubeconfig, 0o600); err != nil { + t.Fatalf("cannot write kubeconfig: %v", err) + } + + // FetchCrossplaneVersion loads its config via ctrl.GetConfig. + t.Setenv("KUBECONFIG", kubeconfig) + + got, err := FetchCrossplaneVersion(cluster.Context, kube.ImpersonationFlags{}) + + if tc.want.errSub != "" { + if err == nil || !strings.Contains(err.Error(), tc.want.errSub) { + t.Fatalf("\n%s\nFetchCrossplaneVersion(): want error containing %q, got %q, %v", tc.reason, tc.want.errSub, got, err) + } + + return + } + + if err != nil { + t.Fatalf("\n%s\nFetchCrossplaneVersion(): %v", tc.reason, err) + } + + if got != tc.want.version { + t.Errorf("\n%s\nFetchCrossplaneVersion(): want %q, got %q", tc.reason, tc.want.version, got) + } + }) + } +} From 289fcc0856a7cd9765d4751c66afa8f606da425d Mon Sep 17 00:00:00 2001 From: Jonathan Ogilvie Date: Wed, 11 Nov 2026 10:18:51 -0500 Subject: [PATCH 3/6] Return version label with exactly one leading v FetchCrossplaneVersion returned an empty string when the app.kubernetes.io/version label already started with v. Co-Authored-By: Claude Sonnet 5.5 Signed-off-by: Jonathan Ogilvie --- cmd/crossplane/version/fetch.go | 8 +------- cmd/crossplane/version/fetch_integration_test.go | 6 ++---- 2 files changed, 3 insertions(+), 11 deletions(-) diff --git a/cmd/crossplane/version/fetch.go b/cmd/crossplane/version/fetch.go index ba8e80e8..51b6a1b7 100644 --- a/cmd/crossplane/version/fetch.go +++ b/cmd/crossplane/version/fetch.go @@ -40,8 +40,6 @@ const ( // and returns the version of the Crossplane deployment. If the version // does not have a leading 'v', it prepends it. func FetchCrossplaneVersion(ctx context.Context, imp kube.ImpersonationFlags) (string, error) { - var version string - config, err := ctrl.GetConfig() if err != nil { return "", errors.Wrap(err, errKubeConfig) @@ -64,11 +62,7 @@ func FetchCrossplaneVersion(ctx context.Context, imp kube.ImpersonationFlags) (s for _, deployment := range deployments.Items { v, ok := deployment.Labels["app.kubernetes.io/version"] if ok { - if !strings.HasPrefix(v, "v") { - version = "v" + v - } - - return version, nil + return "v" + strings.TrimPrefix(v, "v"), nil } if len(deployment.Spec.Template.Spec.Containers) > 0 { diff --git a/cmd/crossplane/version/fetch_integration_test.go b/cmd/crossplane/version/fetch_integration_test.go index 2d22ed98..81666349 100644 --- a/cmd/crossplane/version/fetch_integration_test.go +++ b/cmd/crossplane/version/fetch_integration_test.go @@ -79,11 +79,9 @@ func TestFetchCrossplaneVersionIntegration(t *testing.T) { want: want{version: "v2.0.0"}, }, "VersionLabelWithV": { - // BUG: fetch.go only assigns version when it prepends a v, so a label that already starts with v - // returns "". This case asserts the current behavior; the correct result is "v2.0.0". - reason: "A version label that already has a leading v is (currently, wrongly) returned as an empty string.", + reason: "A version label that already has a leading v is returned unchanged.", deployments: []*appsv1.Deployment{deployment("crossplane", withVersion("v2.0.0"), "xpkg.crossplane.io/crossplane/crossplane:v9.9.9")}, - want: want{version: ""}, + want: want{version: "v2.0.0"}, }, "ImageTag": { reason: "Without a version label, the image tag is returned with a leading v added.", From 846e4fcc5afbf2533b95727d72438d5e3424e278 Mon Sep 17 00:00:00 2001 From: Jonathan Ogilvie Date: Wed, 11 Nov 2026 11:31:27 -0500 Subject: [PATCH 4/6] Use args/want and error comparison in version fetch integration test Co-Authored-By: Claude Sonnet 5.5 Signed-off-by: Jonathan Ogilvie --- .../version/fetch_integration_test.go | 74 ++++++++++--------- 1 file changed, 38 insertions(+), 36 deletions(-) diff --git a/cmd/crossplane/version/fetch_integration_test.go b/cmd/crossplane/version/fetch_integration_test.go index 81666349..3e1765ed 100644 --- a/cmd/crossplane/version/fetch_integration_test.go +++ b/cmd/crossplane/version/fetch_integration_test.go @@ -25,10 +25,14 @@ import ( "strings" "testing" + "github.com/google/go-cmp/cmp" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "github.com/crossplane/crossplane-runtime/v2/pkg/errors" + "github.com/crossplane/crossplane-runtime/v2/pkg/test" + "github.com/crossplane/cli/v2/internal/kube" "github.com/crossplane/cli/v2/internal/testutils/integration" ) @@ -63,50 +67,56 @@ func TestFetchCrossplaneVersionIntegration(t *testing.T) { return map[string]string{"app": "crossplane", "app.kubernetes.io/version": v} } + type args struct { + deployments []*appsv1.Deployment + } + type want struct { version string - errSub string + err error } + errNotFound := errors.New("Crossplane version or image tag not found") + cases := map[string]struct { - reason string - deployments []*appsv1.Deployment - want want + reason string + args args + want want }{ "VersionLabel": { - reason: "A version label without a leading v is returned with one.", - deployments: []*appsv1.Deployment{deployment("crossplane", withVersion("2.0.0"), "xpkg.crossplane.io/crossplane/crossplane:v9.9.9")}, - want: want{version: "v2.0.0"}, + reason: "A version label without a leading v is returned with one.", + args: args{deployments: []*appsv1.Deployment{deployment("crossplane", withVersion("2.0.0"), "xpkg.crossplane.io/crossplane/crossplane:v9.9.9")}}, + want: want{version: "v2.0.0"}, }, "VersionLabelWithV": { - reason: "A version label that already has a leading v is returned unchanged.", - deployments: []*appsv1.Deployment{deployment("crossplane", withVersion("v2.0.0"), "xpkg.crossplane.io/crossplane/crossplane:v9.9.9")}, - want: want{version: "v2.0.0"}, + reason: "A version label that already has a leading v is returned unchanged.", + args: args{deployments: []*appsv1.Deployment{deployment("crossplane", withVersion("v2.0.0"), "xpkg.crossplane.io/crossplane/crossplane:v9.9.9")}}, + want: want{version: "v2.0.0"}, }, "ImageTag": { - reason: "Without a version label, the image tag is returned with a leading v added.", - deployments: []*appsv1.Deployment{deployment("crossplane", xp, "xpkg.crossplane.io/crossplane/crossplane:2.1.0")}, - want: want{version: "v2.1.0"}, + reason: "Without a version label, the image tag is returned with a leading v added.", + args: args{deployments: []*appsv1.Deployment{deployment("crossplane", xp, "xpkg.crossplane.io/crossplane/crossplane:2.1.0")}}, + want: want{version: "v2.1.0"}, }, "ImageTagWithV": { - reason: "Without a version label, an image tag that has a leading v is returned unchanged.", - deployments: []*appsv1.Deployment{deployment("crossplane", xp, "xpkg.crossplane.io/crossplane/crossplane:v2.1.0")}, - want: want{version: "v2.1.0"}, + reason: "Without a version label, an image tag that has a leading v is returned unchanged.", + args: args{deployments: []*appsv1.Deployment{deployment("crossplane", xp, "xpkg.crossplane.io/crossplane/crossplane:v2.1.0")}}, + want: want{version: "v2.1.0"}, }, "ImageDigest": { reason: "An image referenced by digest has no tag, so no version is found.", - deployments: []*appsv1.Deployment{deployment("crossplane", xp, - "xpkg.crossplane.io/crossplane/crossplane@sha256:"+strings.Repeat("a", 64))}, - want: want{errSub: "Crossplane version or image tag not found"}, + args: args{deployments: []*appsv1.Deployment{deployment("crossplane", xp, + "xpkg.crossplane.io/crossplane/crossplane@sha256:"+strings.Repeat("a", 64))}}, + want: want{err: errNotFound}, }, "NoDeployments": { reason: "With no Deployments there is no version to find.", - want: want{errSub: "Crossplane version or image tag not found"}, + want: want{err: errNotFound}, }, "NoMatchingDeployments": { - reason: "Deployments without the app=crossplane label are ignored.", - deployments: []*appsv1.Deployment{deployment("other", map[string]string{"app": "other", "app.kubernetes.io/version": "2.0.0"}, "example.com/other:v1.0.0")}, - want: want{errSub: "Crossplane version or image tag not found"}, + reason: "Deployments without the app=crossplane label are ignored.", + args: args{deployments: []*appsv1.Deployment{deployment("other", map[string]string{"app": "other", "app.kubernetes.io/version": "2.0.0"}, "example.com/other:v1.0.0")}}, + want: want{err: errNotFound}, }, } @@ -119,7 +129,7 @@ func TestFetchCrossplaneVersionIntegration(t *testing.T) { t.Fatalf("cannot create namespace: %v", err) } - for _, d := range tc.deployments { + for _, d := range tc.args.deployments { if err := cluster.Client.Create(cluster.Context, d); err != nil { t.Fatalf("cannot create deployment %s: %v", d.GetName(), err) } @@ -135,20 +145,12 @@ func TestFetchCrossplaneVersionIntegration(t *testing.T) { got, err := FetchCrossplaneVersion(cluster.Context, kube.ImpersonationFlags{}) - if tc.want.errSub != "" { - if err == nil || !strings.Contains(err.Error(), tc.want.errSub) { - t.Fatalf("\n%s\nFetchCrossplaneVersion(): want error containing %q, got %q, %v", tc.reason, tc.want.errSub, got, err) - } - - return - } - - if err != nil { - t.Fatalf("\n%s\nFetchCrossplaneVersion(): %v", tc.reason, err) + if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" { + t.Errorf("\n%s\nFetchCrossplaneVersion(): -want error, +got error:\n%s", tc.reason, diff) } - if got != tc.want.version { - t.Errorf("\n%s\nFetchCrossplaneVersion(): want %q, got %q", tc.reason, tc.want.version, got) + if diff := cmp.Diff(tc.want.version, got); diff != "" { + t.Errorf("\n%s\nFetchCrossplaneVersion(): -want, +got:\n%s", tc.reason, diff) } }) } From df4e9d2bf341bd8bfdc1059069c9efd4170ffb41 Mon Sep 17 00:00:00 2001 From: Jonathan Ogilvie Date: Wed, 11 Nov 2026 11:34:55 -0500 Subject: [PATCH 5/6] Compare whole errors and isolate cases in xpkg install integration tests AlreadyExists creates its own Provider, so it runs alone, and the CRD deletion poll is bounded by a timeout. Co-Authored-By: Claude Sonnet 5.5 Signed-off-by: Jonathan Ogilvie --- .../xpkg/install_integration_test.go | 92 ++++++++++--------- 1 file changed, 51 insertions(+), 41 deletions(-) diff --git a/cmd/crossplane/xpkg/install_integration_test.go b/cmd/crossplane/xpkg/install_integration_test.go index 112fe669..d0ad6eb6 100644 --- a/cmd/crossplane/xpkg/install_integration_test.go +++ b/cmd/crossplane/xpkg/install_integration_test.go @@ -20,10 +20,10 @@ package xpkg import ( "bytes" + "context" "fmt" "os" "path/filepath" - "strings" "testing" "time" @@ -31,11 +31,16 @@ import ( "github.com/google/go-cmp/cmp" corev1 "k8s.io/api/core/v1" extv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + kerrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" + "github.com/crossplane/crossplane-runtime/v2/pkg/errors" "github.com/crossplane/crossplane-runtime/v2/pkg/logging" + "github.com/crossplane/crossplane-runtime/v2/pkg/test" v1 "github.com/crossplane/crossplane/apis/v2/pkg/v1" @@ -97,15 +102,16 @@ func TestInstallIntegration(t *testing.T) { spec v1.PackageSpec rcName string stdout string - errSub string + err error } auto, manual := v1.AutomaticActivation, v1.ManualActivation cases := map[string]struct { - reason string - args []string - want want + reason string + existing v1.Package + args []string + want want }{ "Provider": { reason: "A provider is created with automatic activation, no pull secrets and a zero history limit by default.", @@ -169,7 +175,7 @@ func TestInstallIntegration(t *testing.T) { "ConfigurationWithRuntimeConfig": { reason: "A configuration has no runtime, so a runtime config is refused before anything is created.", args: []string{"configuration", "xpkg.crossplane.io/acme/config:v1.0.0", "config-rc", "--runtime-config=rc"}, - want: want{errSub: "does not support runtime configuration"}, + want: want{err: errors.New("package kind *v1.Configuration does not support runtime configuration")}, }, "DefaultName": { reason: "Without a name the package is named for its repository, as a DNS label.", @@ -186,31 +192,31 @@ func TestInstallIntegration(t *testing.T) { }, }, "AlreadyExists": { - reason: "Installing a package under a name that is taken fails.", - args: []string{"provider", "xpkg.crossplane.io/crossplane-contrib/provider-nop:v0.4.0", "provider-a"}, - want: want{errSub: "already exists"}, + reason: "Installing a package under a name that is taken fails.", + existing: &v1.Provider{ObjectMeta: metav1.ObjectMeta{Name: "provider-taken"}, Spec: v1.ProviderSpec{PackageSpec: v1.PackageSpec{Package: "xpkg.crossplane.io/crossplane-contrib/provider-nop:v0.4.0"}}}, + args: []string{"provider", "xpkg.crossplane.io/crossplane-contrib/provider-nop:v0.4.0", "provider-taken"}, + want: want{err: errors.Wrap(kerrors.NewAlreadyExists(schema.GroupResource{Group: "pkg.crossplane.io", Resource: "providers"}, "provider-taken"), + "cannot create package")}, }, } - // Cases share a cluster and AlreadyExists depends on Provider, so they run in order. - for _, name := range []string{ - "Provider", "ConfigurationWithFlags", "FunctionWithRuntimeConfig", "ProviderWithRuntimeConfig", - "ConfigurationWithRuntimeConfig", "DefaultName", "AlreadyExists", - } { - tc := cases[name] + // Cases share a cluster but use distinct package names, so they don't depend on each other. + for name, tc := range cases { t.Run(name, func(t *testing.T) { - stdout, err := runInstall(t, tc.args...) - - if tc.want.errSub != "" { - if err == nil || !strings.Contains(err.Error(), tc.want.errSub) { - t.Fatalf("\n%s\nRun(): want error containing %q, got %v", tc.reason, tc.want.errSub, err) + if tc.existing != nil { + if err := cluster.Client.Create(cluster.Context, tc.existing); err != nil { + t.Fatalf("cannot create existing package %s: %v", tc.existing.GetName(), err) } + } - return + stdout, err := runInstall(t, tc.args...) + + if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" { + t.Fatalf("\n%s\nRun(): -want error, +got error:\n%s", tc.reason, diff) } - if err != nil { - t.Fatalf("\n%s\nRun(): %v", tc.reason, err) + if tc.want.err != nil { + return } if diff := cmp.Diff(tc.want.stdout, stdout); diff != "" { @@ -258,7 +264,7 @@ func TestInstallWaitIntegration(t *testing.T) { reason string name string healthy bool - wantErr bool + wantErr error }{ "BecomesHealthy": { reason: "Run returns success once the package's Healthy condition turns true.", @@ -268,7 +274,7 @@ func TestInstallWaitIntegration(t *testing.T) { "NeverHealthy": { reason: "Run fails when the package isn't healthy before the wait times out.", name: "provider-unhealthy", - wantErr: true, + wantErr: errors.Wrap(context.DeadlineExceeded, "Package did not become ready"), }, } @@ -294,16 +300,12 @@ func TestInstallWaitIntegration(t *testing.T) { start := time.Now() _, err := runInstall(t, "provider", "xpkg.crossplane.io/acme/prov:v1.0.0", tc.name, "--wait=5s") - if tc.wantErr { - if err == nil || !strings.Contains(err.Error(), "Package did not become ready") { - t.Fatalf("\n%s\nRun(): want a not-ready error, got %v", tc.reason, err) - } - - return + if diff := cmp.Diff(tc.wantErr, err, test.EquateErrors()); diff != "" { + t.Fatalf("\n%s\nRun(): -want error, +got error:\n%s", tc.reason, diff) } - if err != nil { - t.Fatalf("\n%s\nRun(): %v", tc.reason, err) + if tc.wantErr != nil { + return } if elapsed := time.Since(start); elapsed >= 5*time.Second { @@ -324,18 +326,26 @@ func TestInstallMissingCRDIntegration(t *testing.T) { t.Fatalf("cannot delete the Provider CRD: %v", err) } - for cluster.Client.Get(cluster.Context, client.ObjectKeyFromObject(crd), crd) == nil { - time.Sleep(100 * time.Millisecond) + ctx, cancel := context.WithTimeout(cluster.Context, 30*time.Second) + defer cancel() + + for cluster.Client.Get(ctx, client.ObjectKeyFromObject(crd), crd) == nil { + select { + case <-ctx.Done(): + t.Fatalf("the Provider CRD was not deleted: %v", ctx.Err()) + case <-time.After(100 * time.Millisecond): + } } // The client's RESTMapper fails before any request, so the error is a no-match, not the 404 warnIfNotFound - // annotates with a hint that the CLI might be out of date. + // annotates with a hint that the CLI might be out of date. Comparing the whole error checks both. _, err := runInstall(t, "provider", "xpkg.crossplane.io/acme/prov:v1.0.0", "provider-missing") - if err == nil || !strings.Contains(err.Error(), `no matches for kind "Provider"`) { - t.Fatalf("Run(): want a no-match error for the Provider kind, got %v", err) - } - if strings.Contains(err.Error(), "might be out of date") { - t.Errorf("Run(): warnIfNotFound unexpectedly annotated a no-match error: %v", err) + want := errors.Wrap(&meta.NoKindMatchError{ + GroupKind: schema.GroupKind{Group: "pkg.crossplane.io", Kind: "Provider"}, + SearchedVersions: []string{"v1"}, + }, "cannot create package") + if diff := cmp.Diff(want, err, test.EquateErrors()); diff != "" { + t.Errorf("Run(): -want error, +got error:\n%s", diff) } } From 2beb721edfd780b0435294ad108b1b85205b98f6 Mon Sep 17 00:00:00 2001 From: Jonathan Ogilvie Date: Wed, 11 Nov 2026 12:53:36 -0500 Subject: [PATCH 6/6] Use args/want tables in install wait and missing-CRD integration tests Co-Authored-By: Claude Sonnet 5.5 Signed-off-by: Jonathan Ogilvie --- .../xpkg/install_integration_test.go | 78 +++++++++++++------ 1 file changed, 55 insertions(+), 23 deletions(-) diff --git a/cmd/crossplane/xpkg/install_integration_test.go b/cmd/crossplane/xpkg/install_integration_test.go index d0ad6eb6..4c04a120 100644 --- a/cmd/crossplane/xpkg/install_integration_test.go +++ b/cmd/crossplane/xpkg/install_integration_test.go @@ -260,32 +260,39 @@ func TestInstallIntegration(t *testing.T) { func TestInstallWaitIntegration(t *testing.T) { cluster := startInstallCluster(t) - cases := map[string]struct { - reason string + type args struct { name string healthy bool - wantErr error + } + + type want struct { + err error + } + + cases := map[string]struct { + reason string + args args + want want }{ "BecomesHealthy": { - reason: "Run returns success once the package's Healthy condition turns true.", - name: "provider-healthy", - healthy: true, + reason: "Run returns success once the package's Healthy condition turns true.", + args: args{name: "provider-healthy", healthy: true}, }, "NeverHealthy": { - reason: "Run fails when the package isn't healthy before the wait times out.", - name: "provider-unhealthy", - wantErr: errors.Wrap(context.DeadlineExceeded, "Package did not become ready"), + reason: "Run fails when the package isn't healthy before the wait times out.", + args: args{name: "provider-unhealthy"}, + want: want{err: errors.Wrap(context.DeadlineExceeded, "Package did not become ready")}, }, } for name, tc := range cases { t.Run(name, func(t *testing.T) { - if tc.healthy { + if tc.args.healthy { // No package manager runs under envtest, so mark the package healthy once Run has created it. go func() { p := &v1.Provider{} for cluster.Context.Err() == nil { - if err := cluster.Client.Get(cluster.Context, client.ObjectKey{Name: tc.name}, p); err == nil { + if err := cluster.Client.Get(cluster.Context, client.ObjectKey{Name: tc.args.name}, p); err == nil { p.SetConditions(v1.Healthy()) if err := cluster.Client.Status().Update(cluster.Context, p); err == nil { return @@ -298,13 +305,13 @@ func TestInstallWaitIntegration(t *testing.T) { } start := time.Now() - _, err := runInstall(t, "provider", "xpkg.crossplane.io/acme/prov:v1.0.0", tc.name, "--wait=5s") + _, err := runInstall(t, "provider", "xpkg.crossplane.io/acme/prov:v1.0.0", tc.args.name, "--wait=5s") - if diff := cmp.Diff(tc.wantErr, err, test.EquateErrors()); diff != "" { + if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" { t.Fatalf("\n%s\nRun(): -want error, +got error:\n%s", tc.reason, diff) } - if tc.wantErr != nil { + if tc.want.err != nil { return } @@ -337,15 +344,40 @@ func TestInstallMissingCRDIntegration(t *testing.T) { } } - // The client's RESTMapper fails before any request, so the error is a no-match, not the 404 warnIfNotFound - // annotates with a hint that the CLI might be out of date. Comparing the whole error checks both. - _, err := runInstall(t, "provider", "xpkg.crossplane.io/acme/prov:v1.0.0", "provider-missing") + type args struct { + install []string + } + + type want struct { + err error + } + + // Deleting a CRD is cluster-wide setup the install cases can't share, so this scenario keeps its own table. + cases := map[string]struct { + reason string + args args + want want + }{ + "ProviderCRDMissing": { + // The client's RESTMapper fails before any request, so the error is a no-match, not the 404 + // warnIfNotFound annotates with a hint that the CLI might be out of date. Comparing the whole error + // checks both. + reason: "Installing a kind the cluster doesn't serve fails with a no-match error.", + args: args{install: []string{"provider", "xpkg.crossplane.io/acme/prov:v1.0.0", "provider-missing"}}, + want: want{err: errors.Wrap(&meta.NoKindMatchError{ + GroupKind: schema.GroupKind{Group: "pkg.crossplane.io", Kind: "Provider"}, + SearchedVersions: []string{"v1"}, + }, "cannot create package")}, + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + _, err := runInstall(t, tc.args.install...) - want := errors.Wrap(&meta.NoKindMatchError{ - GroupKind: schema.GroupKind{Group: "pkg.crossplane.io", Kind: "Provider"}, - SearchedVersions: []string{"v1"}, - }, "cannot create package") - if diff := cmp.Diff(want, err, test.EquateErrors()); diff != "" { - t.Errorf("Run(): -want error, +got error:\n%s", diff) + if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" { + t.Errorf("\n%s\nRun(): -want error, +got error:\n%s", tc.reason, diff) + } + }) } }