Skip to content

🌱 Remove ginkgo from internal/controller unit tests #541

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Nov 17, 2023
Merged

🌱 Remove ginkgo from internal/controller unit tests #541

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0feae9d
Remove ginkgo from internal/controller unit tests
tmshort Sep 12, 2023
1461eca
Always allocate cl
tmshort Nov 14, 2023
6205da4
Move admissions tests to tables
tmshort Nov 14, 2023
af6d30b
Use random name for operators
tmshort Nov 14, 2023
bb80d16
Parallelize the admission tests
tmshort Nov 14, 2023
bd389d8
Consolidate client/reconciler initialization
tmshort Nov 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
328 changes: 187 additions & 141 deletions internal/controllers/admission_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@ package controllers_test

import (
"context"
"testing"

. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
"github.com/stretchr/testify/require"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

operatorsv1alpha1 "github.com/operator-framework/operator-controller/api/v1alpha1"
Expand All @@ -13,165 +13,211 @@ import (
func operator(spec operatorsv1alpha1.OperatorSpec) *operatorsv1alpha1.Operator {
return &operatorsv1alpha1.Operator{
ObjectMeta: metav1.ObjectMeta{
Name: "test-operator",
GenerateName: "test-operator",
},
Spec: spec,
}
}

var _ = Describe("Operator Spec Validations", func() {
var (
ctx context.Context
cancel context.CancelFunc
)
BeforeEach(func() {
ctx, cancel = context.WithCancel(context.Background())
})
AfterEach(func() {
cancel()
})
It("should fail if the spec is empty", func() {
err := cl.Create(ctx, operator(operatorsv1alpha1.OperatorSpec{}))
Expect(err).To(HaveOccurred())
Expect(err.Error()).To(ContainSubstring("spec.packageName in body should match '^[a-z0-9]+(-[a-z0-9]+)*$'"))
})
It("should fail if package name length is greater than 48 characters", func() {
err := cl.Create(ctx, operator(operatorsv1alpha1.OperatorSpec{
var operatorData = []struct {
spec *operatorsv1alpha1.Operator
comment string
errMsg string
}{
{
operator(operatorsv1alpha1.OperatorSpec{}),
"operator spec is empty",
"spec.packageName in body should match '^[a-z0-9]+(-[a-z0-9]+)*$'",
},
{
operator(operatorsv1alpha1.OperatorSpec{
PackageName: "this-is-a-really-long-package-name-that-is-greater-than-48-characters",
}))
Expect(err).To(HaveOccurred())
Expect(err.Error()).To(ContainSubstring("Too long: may not be longer than 48"))
})
It("should fail if version is valid semver but length is greater than 64 characters", func() {
err := cl.Create(ctx, operator(operatorsv1alpha1.OperatorSpec{
}),
"long package name",
"spec.packageName: Too long: may not be longer than 48",
},
{
operator(operatorsv1alpha1.OperatorSpec{
PackageName: "package",
Version: "1234567890.1234567890.12345678901234567890123456789012345678901234",
}))
Expect(err).To(HaveOccurred())
Expect(err.Error()).To(ContainSubstring("Too long: may not be longer than 64"))
})
It("should fail if an invalid semver is given", func() {
invalidSemvers := []string{
"1.2.3.4",
"1.02.3",
"1.2.03",
"1.2.3-beta!",
"1.2.3.alpha",
"1..2.3",
"1.2.3-pre+bad_metadata",
"1.2.-3",
".1.2.3",
"<<1.2.3",
">>1.2.3",
">~1.2.3",
"==1.2.3",
"=!1.2.3",
"!1.2.3",
"1.Y",
">1.2.3 && <2.3.4",
">1.2.3;<2.3.4",
"1.2.3 - 2.3.4",
}
for _, invalidSemver := range invalidSemvers {
}),
"long valid semver",
"spec.version: Too long: may not be longer than 64",
},
{
operator(operatorsv1alpha1.OperatorSpec{
PackageName: "package",
Channel: "longname01234567890123456789012345678901234567890",
}),
"long channel name",
"spec.channel: Too long: may not be longer than 48",
},
}

func TestOperatorSpecs(t *testing.T) {
t.Parallel()
ctx, cancel := context.WithCancel(context.Background())
t.Cleanup(cancel)

for _, od := range operatorData {
d := od
t.Run(d.comment, func(t *testing.T) {
t.Parallel()
cl := newClient(t)
err := cl.Create(ctx, d.spec)
require.Error(t, err)
require.ErrorContains(t, err, d.errMsg)
})
}
}

func TestOperatorInvalidSemver(t *testing.T) {
t.Parallel()
ctx, cancel := context.WithCancel(context.Background())
t.Cleanup(cancel)

invalidSemvers := []string{
"1.2.3.4",
"1.02.3",
"1.2.03",
"1.2.3-beta!",
"1.2.3.alpha",
"1..2.3",
"1.2.3-pre+bad_metadata",
"1.2.-3",
".1.2.3",
"<<1.2.3",
">>1.2.3",
">~1.2.3",
"==1.2.3",
"=!1.2.3",
"!1.2.3",
"1.Y",
">1.2.3 && <2.3.4",
">1.2.3;<2.3.4",
"1.2.3 - 2.3.4",
}
for _, sm := range invalidSemvers {
d := sm
t.Run(d, func(t *testing.T) {
t.Parallel()
cl := newClient(t)
err := cl.Create(ctx, operator(operatorsv1alpha1.OperatorSpec{
PackageName: "package",
Version: invalidSemver,
Version: d,
}))

Expect(err).To(HaveOccurred(), "expected error for invalid semver %q", invalidSemver)
require.Errorf(t, err, "expected error for invalid semver %q", d)
// Don't need to include the whole regex, this should be enough to match the MasterMinds regex
Expect(err.Error()).To(ContainSubstring("spec.version in body should match '^(\\s*(=||!=|>|<|>=|=>|<=|=<|~|~>|\\^)"))
}
})
It("should pass if a valid semver range given", func() {
validSemvers := []string{
">=1.2.3",
"=>1.2.3",
">= 1.2.3",
">=v1.2.3",
">= v1.2.3",
"<=1.2.3",
"=<1.2.3",
"=1.2.3",
"!=1.2.3",
"<1.2.3",
">1.2.3",
"~1.2.2",
"~>1.2.3",
"^1.2.3",
"1.2.3",
"v1.2.3",
"1.x",
"1.X",
"1.*",
"1.2.x",
"1.2.X",
"1.2.*",
">=1.2.3 <2.3.4",
">=1.2.3,<2.3.4",
">=1.2.3, <2.3.4",
"<1.2.3||>2.3.4",
"<1.2.3|| >2.3.4",
"<1.2.3 ||>2.3.4",
"<1.2.3 || >2.3.4",
">1.0.0,<1.2.3 || >2.1.0",
"<1.2.3-abc >2.3.4-def",
"<1.2.3-abc+def >2.3.4-ghi+jkl",
}
for _, validSemver := range validSemvers {
require.ErrorContains(t, err, "spec.version in body should match '^(\\s*(=||!=|>|<|>=|=>|<=|=<|~|~>|\\^)")
})
}
}

func TestOperatorValidSemver(t *testing.T) {
t.Parallel()
ctx, cancel := context.WithCancel(context.Background())
t.Cleanup(cancel)

validSemvers := []string{
">=1.2.3",
"=>1.2.3",
">= 1.2.3",
">=v1.2.3",
">= v1.2.3",
"<=1.2.3",
"=<1.2.3",
"=1.2.3",
"!=1.2.3",
"<1.2.3",
">1.2.3",
"~1.2.2",
"~>1.2.3",
"^1.2.3",
"1.2.3",
"v1.2.3",
"1.x",
"1.X",
"1.*",
"1.2.x",
"1.2.X",
"1.2.*",
">=1.2.3 <2.3.4",
">=1.2.3,<2.3.4",
">=1.2.3, <2.3.4",
"<1.2.3||>2.3.4",
"<1.2.3|| >2.3.4",
"<1.2.3 ||>2.3.4",
"<1.2.3 || >2.3.4",
">1.0.0,<1.2.3 || >2.1.0",
"<1.2.3-abc >2.3.4-def",
"<1.2.3-abc+def >2.3.4-ghi+jkl",
}
for _, smx := range validSemvers {
d := smx
t.Run(d, func(t *testing.T) {
t.Parallel()
cl := newClient(t)
op := operator(operatorsv1alpha1.OperatorSpec{
PackageName: "package",
Version: validSemver,
Version: d,
})
err := cl.Create(ctx, op)
Expect(err).NotTo(HaveOccurred(), "expected success for semver range '%q': %w", validSemver, err)
err = cl.Delete(ctx, op)
Expect(err).NotTo(HaveOccurred(), "unexpected error deleting valid semver '%q': %w", validSemver, err)
}
})
It("should fail if an invalid channel name is given", func() {
invalidChannels := []string{
"spaces spaces",
"Capitalized",
"camelCase",
"many/invalid$characters+in_name",
"-start-with-hyphen",
"end-with-hyphen-",
".start-with-period",
"end-with-period.",
}
for _, invalidChannel := range invalidChannels {
require.NoErrorf(t, err, "unexpected error for semver range %q: %w", d, err)
})
}
}

func TestOperatorInvalidChannel(t *testing.T) {
t.Parallel()
ctx, cancel := context.WithCancel(context.Background())
t.Cleanup(cancel)

invalidChannels := []string{
Copy link

Choose a reason for hiding this comment

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

General comment for this and the lists of semver ranges above - what are we testing? Is it validating admission? If so - there are straightforward ways to unit-test that logic that will be orders of magnitude faster than this. Lists such as these look like the wrong level of abstraction to test in integration tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a direct translation of the existing tests. I'm not looking to change the nature of the tests here.

Copy link
Contributor Author

@tmshort tmshort Nov 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

So, there's a set of validatior tests in a subdirectory of this, to which these can probably be moved.
That's a much smaller translation, so I'd be willing to add these tests to there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And my recollection is that the complex regex used to validate these semvers may be a bit lenient, so this is a more thorough test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That appears to be the case, not all of these can be easily validated by the admission validation (see #542 https://github.com/operator-framework/operator-controller/pull/542/files#diff-6af2fcdc11cc7d43e85d42a4926bfa2ffd47e2292b09ba20407ba78b0f34c3d1R74), so these need to be left here, as they are testing the parsing code during reconciliation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you saying we should not test a complex regex? Or are you saying there's a better way to test the regex?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ping @stevekuznetsov

Copy link

Choose a reason for hiding this comment

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

Sorry, missed this - let's keep it for now but open an issue to do it at the unit level. If you follow this code you can see how to generate a validator for a structural schema. Then you can run this a couple orders of magnitude faster and without network hops. I don't think it's good for anyone to run unit-level validations in e2e.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A reminder that this is unit tests. The e2e tests are in #545

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Issue: #549

"spaces spaces",
"Capitalized",
"camelCase",
"many/invalid$characters+in_name",
"-start-with-hyphen",
"end-with-hyphen-",
".start-with-period",
"end-with-period.",
}
for _, ch := range invalidChannels {
d := ch
t.Run(d, func(t *testing.T) {
t.Parallel()
cl := newClient(t)
err := cl.Create(ctx, operator(operatorsv1alpha1.OperatorSpec{
PackageName: "package",
Channel: invalidChannel,
Channel: d,
}))
Expect(err).To(HaveOccurred(), "expected error for invalid channel '%q'", invalidChannel)
Expect(err.Error()).To(ContainSubstring("spec.channel in body should match '^[a-z0-9]+([\\.-][a-z0-9]+)*$'"))
}
})
It("should pass if a valid channel name is given", func() {
validChannels := []string{
"hyphenated-name",
"dotted.name",
"channel-has-version-1.0.1",
}
for _, validChannel := range validChannels {
require.Errorf(t, err, "expected error for invalid channel %q", d)
require.ErrorContains(t, err, "spec.channel in body should match '^[a-z0-9]+([\\.-][a-z0-9]+)*$'")
})
}
}

func TestOperatorValidChannel(t *testing.T) {
t.Parallel()
ctx, cancel := context.WithCancel(context.Background())
t.Cleanup(cancel)

validChannels := []string{
"hyphenated-name",
"dotted.name",
"channel-has-version-1.0.1",
}
for _, ch := range validChannels {
d := ch
t.Run(d, func(t *testing.T) {
t.Parallel()
cl := newClient(t)
op := operator(operatorsv1alpha1.OperatorSpec{
PackageName: "package",
Channel: validChannel,
Channel: d,
})
err := cl.Create(ctx, op)
Expect(err).NotTo(HaveOccurred(), "unexpected error creating valid channel '%q': %w", validChannel, err)
err = cl.Delete(ctx, op)
Expect(err).NotTo(HaveOccurred(), "unexpected error deleting valid channel '%q': %w", validChannel, err)
}
})
It("should fail if an invalid channel name length", func() {
err := cl.Create(ctx, operator(operatorsv1alpha1.OperatorSpec{
PackageName: "package",
Channel: "longname01234567890123456789012345678901234567890",
}))
Expect(err).To(HaveOccurred(), "expected error for invalid channel length")
Expect(err.Error()).To(ContainSubstring("spec.channel: Too long: may not be longer than 48"))
})
})
require.NoErrorf(t, err, "unexpected error creating valid channel %q: %w", d, err)
})
}
}
Loading