🌱 Remove ginkgo from internal/controller unit tests #541

tmshort · 2023-11-13T20:52:31Z

Fix #190

This also puts back some of the post-testing cleanup.

Description

Reviewer Checklist

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

codecov · 2023-11-13T21:16:33Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (b55d472) 83.69% compared to head (bd389d8) 83.69%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #541   +/-   ##
=======================================
  Coverage   83.69%   83.69%           
=======================================
  Files          20       20           
  Lines         822      822           
=======================================
  Hits          688      688           
  Misses         92       92           
  Partials       42       42

Flag	Coverage Δ
e2e	`63.86% <ø> (ø)`
unit	`78.59% <ø> (ø)`

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.

internal/controllers/admission_test.go

stevekuznetsov · 2023-11-14T14:14:26Z

internal/controllers/admission_test.go

+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+
+	invalidChannels := []string{


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.

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

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.

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.

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.

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

ping @stevekuznetsov

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.

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

Issue: #549

internal/controllers/suite_test.go

tmshort · 2023-11-14T14:48:51Z

@stevekuznetsov I would much prefer to get this in as a direct translation, and then optimize later.

stevekuznetsov · 2023-11-14T15:20:17Z

Why? If we know we want the "optimizations", what are we (the group) gaining by merging this PR today as opposed to merging the PR + the comment responses tomorrow?

tmshort · 2023-11-14T15:34:36Z

TBH, I think it's easier to review, at least when making changes such as table-based tests. I can certainly remove the unnecessary clean up.

stevekuznetsov · 2023-11-14T15:43:32Z

I don't (personally) mind the review burden, as even with the current commit I had to read the diff side-by-side instead of interleaved. You could always put multiple commits in the PR if you wanted to document intermediate states.

tmshort · 2023-11-14T17:09:25Z

I don't (personally) mind the review burden, as even with the current commit I had to read the diff side-by-side instead of interleaved. You could always put multiple commits in the PR if you wanted to document intermediate states.

I'm ok with splitting it up into multiple commits, if you don't mind the review burden.

tmshort · 2023-11-14T19:29:15Z

@stevekuznetsov this should address your concerns.

internal/controllers/admission_test.go

stevekuznetsov · 2023-11-15T13:53:03Z

internal/controllers/admission_test.go

+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+
+	invalidChannels := []string{


Hm, I'd still wonder, though - if we are issuing a CREATE and expecting it to fail, what mechanism causes that? If it's validating admission from the structural schema on the CRD, the underlying mechanism should be that exact regex we've specified in the kubebuilder comment. I don't fully grok how this test can be more specific than a unit test of validating admission specifically.

stevekuznetsov · 2023-11-15T13:57:49Z

internal/controllers/operator_controller_test.go

+	t.Log("It results in the expected BundleDeployment")
+	bd := &rukpakv1alpha1.BundleDeployment{}
+	require.NoError(t, cl.Get(ctx, types.NamespacedName{Name: opKey.Name}, bd))
+	require.Equal(t, "core-rukpak-io-plain", bd.Spec.ProvisionerClassName)


The one further thing that would bring this more inline with common patterns would be to assert on a specific structure -

require.Empty(cmp.Diff(bd.Spec, operatorsv1alpha1.BundleDeploymentSpec{ ... }))

IMO this makes writing assertions more compact, and the output errors are about 10,000x better - right now if this line fails you have to go read the code to figure out what field "core-rukpak-io-plain" was supposed to be in.

require.Equal of the whole structure might be better than adding cmp.Diff

require.Empty+cmp.Diff is waaaaaay better than require.Equal. If you get a test failure, it is orders of magnitude easier to diagnose the difference using the cmp.Diff approach.

Learning something new! I'll look into it if I have time.

internal/controllers/operator_controller_test.go

stevekuznetsov · 2023-11-15T14:01:25Z

internal/controllers/operator_controller_test.go

-	})
-})
+	// this bypasses client/server-side CR validation and allows us to test the reconciler's validation
+	fakeClient := fake.NewClientBuilder().WithScheme(sch).WithObjects(operator).WithStatusSubresource(operator).Build()


Not something you need to change here but we might want to queue this up for discussion later, this test case seems a bit wild.

I didn't originally write most of these tests! :)

Yep! Undersood :)

Fix operator-framework#190 Signed-off-by: Todd Short <[email protected]>

Signed-off-by: Todd Short <[email protected]>

tmshort requested a review from a team as a code owner November 13, 2023 20:52

tmshort force-pushed the test-controllers2 branch from bf9f30c to dced08b Compare November 13, 2023 21:10

stevekuznetsov reviewed Nov 14, 2023

View reviewed changes

internal/controllers/admission_test.go Outdated Show resolved Hide resolved

stevekuznetsov reviewed Nov 14, 2023

View reviewed changes

internal/controllers/admission_test.go Outdated Show resolved Hide resolved

stevekuznetsov reviewed Nov 14, 2023

View reviewed changes

tmshort force-pushed the test-controllers2 branch 2 times, most recently from ca58fef to 7c94479 Compare November 14, 2023 19:28

stevekuznetsov reviewed Nov 15, 2023

View reviewed changes

tmshort force-pushed the test-controllers2 branch from 30ca688 to 61783da Compare November 15, 2023 19:22

tmshort added 6 commits November 16, 2023 11:26

Remove ginkgo from internal/controller unit tests

0feae9d

Fix operator-framework#190 Signed-off-by: Todd Short <[email protected]>

Always allocate cl

1461eca

Signed-off-by: Todd Short <[email protected]>

Move admissions tests to tables

6205da4

Signed-off-by: Todd Short <[email protected]>

Use random name for operators

af6d30b

Signed-off-by: Todd Short <[email protected]>

Parallelize the admission tests

bb80d16

Signed-off-by: Todd Short <[email protected]>

Consolidate client/reconciler initialization

bd389d8

Signed-off-by: Todd Short <[email protected]>

tmshort force-pushed the test-controllers2 branch from a6ecb2a to bd389d8 Compare November 16, 2023 21:36

stevekuznetsov approved these changes Nov 17, 2023

View reviewed changes

stevekuznetsov added this pull request to the merge queue Nov 17, 2023

tmshort mentioned this pull request Nov 17, 2023

Update admission tests validation performance #549

Open

Merged via the queue into operator-framework:main with commit dfa515d Nov 17, 2023
15 checks passed

tmshort deleted the test-controllers2 branch July 16, 2024 19:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🌱 Remove ginkgo from internal/controller unit tests #541

🌱 Remove ginkgo from internal/controller unit tests #541

tmshort commented Nov 13, 2023

codecov bot commented Nov 13, 2023 •

edited

Loading

stevekuznetsov Nov 14, 2023

tmshort Nov 14, 2023

tmshort Nov 14, 2023 •

edited

Loading

tmshort Nov 14, 2023

tmshort Nov 14, 2023

tmshort Nov 16, 2023

tmshort Nov 17, 2023

stevekuznetsov Nov 17, 2023

tmshort Nov 17, 2023

tmshort Nov 17, 2023

tmshort commented Nov 14, 2023

stevekuznetsov commented Nov 14, 2023

tmshort commented Nov 14, 2023

stevekuznetsov commented Nov 14, 2023

tmshort commented Nov 14, 2023 •

edited

Loading

tmshort commented Nov 14, 2023

stevekuznetsov Nov 15, 2023

stevekuznetsov Nov 15, 2023

tmshort Nov 15, 2023

ncdc Nov 15, 2023

tmshort Nov 15, 2023

stevekuznetsov Nov 15, 2023

tmshort Nov 15, 2023

stevekuznetsov Nov 15, 2023

🌱 Remove ginkgo from internal/controller unit tests #541

🌱 Remove ginkgo from internal/controller unit tests #541

Conversation

tmshort commented Nov 13, 2023

Description

Reviewer Checklist

codecov bot commented Nov 13, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmshort Nov 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmshort commented Nov 14, 2023

stevekuznetsov commented Nov 14, 2023

tmshort commented Nov 14, 2023

stevekuznetsov commented Nov 14, 2023

tmshort commented Nov 14, 2023 • edited Loading

tmshort commented Nov 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 13, 2023 •

edited

Loading

tmshort Nov 14, 2023 •

edited

Loading

tmshort commented Nov 14, 2023 •

edited

Loading