Skip to content

Support for test run labels/selectors #483

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
Aug 31, 2023
Merged

Support for test run labels/selectors #483

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
a49fb32
Add TestFile type.
porridge Aug 9, 2023
4affac7
make generate
porridge Aug 9, 2023
02d6009
Implementation.
porridge Aug 9, 2023
e7d8400
Tests.
porridge Aug 21, 2023
0f0255a
Review comment.
porridge Aug 31, 2023
4de436c
Merge branch 'main' into test-run-labels
kensipe Aug 31, 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
13 changes: 13 additions & 0 deletions pkg/apis/testharness/v1beta1/test_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,19 @@ type RestConfig struct {

// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object

// TestFile contains attributes of a single test file.
type TestFile struct {
// The type meta object, should always be a GVK of kuttl.dev/v1beta1/TestFile.
metav1.TypeMeta `json:",inline"`
// Set labels or the test suite name.
metav1.ObjectMeta `json:"metadata,omitempty"`

// Which test runs should this file be used in. Empty selector matches all test runs.
TestRunSelector *metav1.LabelSelector `json:"testRunSelector,omitempty"`
Copy link
Member

@kensipe kensipe Aug 29, 2023

Choose a reason for hiding this comment

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

I'm curious why we use this structure... this doesn't feel like a selector... the selector is on the command line of running kuttl. This feels more like a label (that a selector would select on) which makes me wonder why we wouldn't use the standard k8s model which would look more like:

==> tt/01-assert-cat.yaml <==
apiVersion: kuttl.dev/v1beta1
kind: TestFile
metadata:
    labels:
        flavor: cat

thoughts?

Copy link
Member Author

@porridge porridge Aug 31, 2023

Choose a reason for hiding this comment

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

This particular arrangement of which side labels vs selectors go was @eddycharly 's idea, so maybe he can chime in on where it came from. Maybe it's following some pattern seen elsewhere?

I guess you can think about this in two ways:

  1. (this implementation): a "test run" (i.e. a kuttl invocation) comes with a set of labels attached. These labels represent some characteristics of this test invocation. In my use case, there would would be a single label with just two possible values:openshift=true or openshift=false, but you could imagine multiple labels and values for a more elaborate test matrix (such as the cartesian product of plaform={openshift,vanilla,eks},k8s-version={1.28,1.29,1.30}). Then individual files in the test suite provide an optional label selector, which needs to match the label set in order for the file to be considered for the test run. Obviously the selector can contain a subset of label keys that the test run has. For example a given file might only select one or more k8s-version values and be neutral w.r.t. platform. The notable edge case is a file that (be default) has no selector attached - an empty selector matches all possible label sets, meaning this feature is fully backwards-compatible - a file without a TestFile is included in all test invocations, just like before this change.
  2. (I haven't thought about this case earlier so I'm inventing things as I go.) Each test file comes with an optional label set (empty by default). Let's say that some file tests some API introduced in version 1.29 but also present in 1.30. Since a label set can only specify a single value for a given label key, we'd either have to allow multiple label sets per file (eek!) or introduce a copy of the file per k8s version (ouch) or instead of labeling by k8s version, label by feature availability feature-foo={true,false}. Then, each test run can have a label selector attached, and this selector would need to select the APIs supported. This could quickly create an explosion of labels and combinations. What's worse, files which are agnostic about a given API would have to somehow advertise that fact, in order to be selected by a selector which does mention a given API.

So it looks to me that only solution 1 is in fact realistic.

}

// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object

// TestSuite configures which tests should be loaded.
type TestSuite struct {
// The type meta object, should always be a GVK of kuttl.dev/v1beta1/TestSuite or kuttl.dev/v1beta1/TestSuite.
Expand Down
32 changes: 32 additions & 0 deletions pkg/apis/testharness/v1beta1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions pkg/kuttlctl/cmd/test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ func newTestCmd() *cobra.Command { //nolint:gocyclo
reportName := "kuttl-report"
namespace := ""
suppress := []string{}
var runLabels labelSetValue

options := harness.TestSuite{}

Expand Down Expand Up @@ -229,6 +230,7 @@ For more detailed documentation, visit: https://kuttl.dev`,
harness := test.Harness{
TestSuite: options,
T: t,
RunLabels: runLabels.AsLabelSet(),
}

harness.Run()
Expand Down Expand Up @@ -257,6 +259,7 @@ For more detailed documentation, visit: https://kuttl.dev`,
testCmd.Flags().StringVar(&reportName, "report-name", "kuttl-report", "Name for the report. Report location determined by --artifacts-dir and report file type determined by --report.")
testCmd.Flags().StringVarP(&namespace, "namespace", "n", "", "Namespace to use for tests. Provided namespaces must exist prior to running tests.")
testCmd.Flags().StringSliceVar(&suppress, "suppress-log", []string{}, "Suppress logging for these kinds of logs (events).")
testCmd.Flags().Var(&runLabels, "test-run-labels", "Labels to use for this test run.")
// This cannot be a global flag because pkg/test/utils.RunTests calls flag.Parse which barfs on unknown top-level flags.
// Putting it here at least does not advertise it on a level where using it is impossible.
test.SetFlags(testCmd.Flags())
Expand Down
30 changes: 30 additions & 0 deletions pkg/kuttlctl/cmd/values.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
package cmd
Copy link
Member

@kensipe kensipe Aug 29, 2023

Choose a reason for hiding this comment

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

not sure about name of file... values could be anything... seems like labels or something similar is more readable IMO

Copy link
Member Author

@porridge porridge Aug 31, 2023

Choose a reason for hiding this comment

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

I called it values because the type implements apflag.Value interface and I thought it might be a good name if we add more types that can be used via that interface. Maybe I should have inserted a comment about this...

But sure, this can be labels.go too if we go for one file per type. Let me know which one you prefer.


import (
"fmt"

"k8s.io/apimachinery/pkg/labels"
)

type labelSetValue labels.Set

func (v *labelSetValue) String() string {
return labels.Set(*v).String()
}

func (v *labelSetValue) Set(s string) error {
l, err := labels.ConvertSelectorToLabelsMap(s)
if err != nil {
return fmt.Errorf("cannot parse label set: %w", err)
}
*v = labelSetValue(l)
return nil
}

func (v *labelSetValue) Type() string {
return "labelSet"
}

func (v labelSetValue) AsLabelSet() labels.Set {
return labels.Set(v)
}
17 changes: 10 additions & 7 deletions pkg/test/case.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
eventsbeta1 "k8s.io/api/events/v1beta1"
k8serrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/labels"
"k8s.io/apimachinery/pkg/util/wait"
"k8s.io/client-go/discovery"
"sigs.k8s.io/controller-runtime/pkg/client"
Expand All @@ -40,6 +41,7 @@ type Case struct {
SkipDelete bool
Timeout int
PreferredNamespace string
RunLabels labels.Set

Client func(forceNew bool) (client.Client, error)
DiscoveryClient func() (discovery.DiscoveryInterface, error)
Expand Down Expand Up @@ -458,13 +460,14 @@ func (t *Case) LoadTestSteps() error {

for index, files := range testStepFiles {
testStep := &Step{
Timeout: t.Timeout,
Index: int(index),
SkipDelete: t.SkipDelete,
Dir: t.Dir,
Asserts: []client.Object{},
Apply: []client.Object{},
Errors: []client.Object{},
Timeout: t.Timeout,
Index: int(index),
SkipDelete: t.SkipDelete,
Dir: t.Dir,
TestRunLabels: t.RunLabels,
Asserts: []client.Object{},
Apply: []client.Object{},
Errors: []client.Object{},
}

for _, file := range files {
Expand Down
128 changes: 116 additions & 12 deletions pkg/test/case_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
package test

import (
"fmt"
"testing"

"github.com/stretchr/testify/assert"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/labels"
"sigs.k8s.io/controller-runtime/pkg/client"

harness "github.com/kudobuilder/kuttl/pkg/apis/testharness/v1beta1"
Expand All @@ -18,10 +20,12 @@ import (
func TestLoadTestSteps(t *testing.T) {
for _, tt := range []struct {
path string
runLabels labels.Set
testSteps []Step
}{
{
"test_data/with-overrides/",
"test_data/with-overrides",
labels.Set{},
[]Step{
{
Name: "with-test-step-name-override",
Expand Down Expand Up @@ -52,7 +56,8 @@ func TestLoadTestSteps(t *testing.T) {
"qosClass": "BestEffort",
}),
},
Errors: []client.Object{},
Errors: []client.Object{},
TestRunLabels: labels.Set{},
},
{
Name: "test-assert",
Expand Down Expand Up @@ -96,7 +101,8 @@ func TestLoadTestSteps(t *testing.T) {
"qosClass": "BestEffort",
}),
},
Errors: []client.Object{},
Errors: []client.Object{},
TestRunLabels: labels.Set{},
},
{
Name: "pod",
Expand Down Expand Up @@ -124,7 +130,8 @@ func TestLoadTestSteps(t *testing.T) {
"qosClass": "BestEffort",
}),
},
Errors: []client.Object{},
Errors: []client.Object{},
TestRunLabels: labels.Set{},
},
{
Name: "name-overridden",
Expand Down Expand Up @@ -164,12 +171,14 @@ func TestLoadTestSteps(t *testing.T) {
"restartPolicy": "Never",
}),
},
Errors: []client.Object{},
Errors: []client.Object{},
TestRunLabels: labels.Set{},
},
},
},
{
"test_data/list-pods",
labels.Set{},
[]Step{
{
Name: "pod",
Expand Down Expand Up @@ -217,15 +226,110 @@ func TestLoadTestSteps(t *testing.T) {
},
},
},
Errors: []client.Object{},
TestRunLabels: labels.Set{},
},
},
},
{
"test_data/test-run-labels",
labels.Set{},
[]Step{
{
Name: "",
Index: 1,
TestRunLabels: labels.Set{},
Apply: []client.Object{},
Asserts: []client.Object{},
Errors: []client.Object{},
},
},
},
{
"test_data/test-run-labels",
labels.Set{"flavor": "a"},
[]Step{
{
Name: "create-a",
Index: 1,
TestRunLabels: labels.Set{"flavor": "a"},
Apply: []client.Object{
&unstructured.Unstructured{
Object: map[string]interface{}{
"apiVersion": "v1",
"kind": "ConfigMap",
"metadata": map[string]interface{}{
"name": "test",
},
"data": map[string]interface{}{
"flavor": "a",
},
},
},
},
Asserts: []client.Object{
&unstructured.Unstructured{
Object: map[string]interface{}{
"apiVersion": "v1",
"kind": "ConfigMap",
"metadata": map[string]interface{}{
"name": "test",
},
"data": map[string]interface{}{
"flavor": "a",
},
},
},
},
Errors: []client.Object{},
},
},
},
{
"test_data/test-run-labels",
labels.Set{"flavor": "b"},
[]Step{
{
Name: "create-b",
Index: 1,
TestRunLabels: labels.Set{"flavor": "b"},
Apply: []client.Object{
&unstructured.Unstructured{
Object: map[string]interface{}{
"apiVersion": "v1",
"kind": "ConfigMap",
"metadata": map[string]interface{}{
"name": "test",
},
"data": map[string]interface{}{
"flavor": "b",
},
},
},
},
Asserts: []client.Object{
&unstructured.Unstructured{
Object: map[string]interface{}{
"apiVersion": "v1",
"kind": "ConfigMap",
"metadata": map[string]interface{}{
"name": "test",
},
"data": map[string]interface{}{
"flavor": "b",
},
},
},
},
Errors: []client.Object{},
},
},
},
} {
tt := tt

t.Run(tt.path, func(t *testing.T) {
test := &Case{Dir: tt.path, Logger: testutils.NewTestLogger(t, tt.path)}
t.Run(fmt.Sprintf("%s/%s", tt.path, tt.runLabels), func(t *testing.T) {
test := &Case{Dir: tt.path, Logger: testutils.NewTestLogger(t, tt.path), RunLabels: tt.runLabels}

err := test.LoadTestSteps()
assert.Nil(t, err)
Expand All @@ -238,11 +342,11 @@ func TestLoadTestSteps(t *testing.T) {
assert.Equal(t, len(tt.testSteps), len(testStepsVal))
for index := range tt.testSteps {
tt.testSteps[index].Dir = tt.path
assert.Equal(t, tt.testSteps[index].Apply, testStepsVal[index].Apply)
assert.Equal(t, tt.testSteps[index].Asserts, testStepsVal[index].Asserts)
assert.Equal(t, tt.testSteps[index].Errors, testStepsVal[index].Errors)
assert.Equal(t, tt.testSteps[index].Step, testStepsVal[index].Step)
assert.Equal(t, tt.testSteps[index].Dir, testStepsVal[index].Dir)
assert.Equal(t, tt.testSteps[index].Apply, testStepsVal[index].Apply, "apply objects need to match")
assert.Equal(t, tt.testSteps[index].Asserts, testStepsVal[index].Asserts, "assert objects need to match")
assert.Equal(t, tt.testSteps[index].Errors, testStepsVal[index].Errors, "error objects need to match")
assert.Equal(t, tt.testSteps[index].Step, testStepsVal[index].Step, "step object needs to match")
assert.Equal(t, tt.testSteps[index].Dir, testStepsVal[index].Dir, "dir needs to match")
assert.Equal(t, tt.testSteps[index], testStepsVal[index])
}
})
Expand Down
3 changes: 3 additions & 0 deletions pkg/test/harness.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
volumetypes "github.com/docker/docker/api/types/volume"
docker "github.com/docker/docker/client"
"gopkg.in/yaml.v2"
"k8s.io/apimachinery/pkg/labels"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/client-go/discovery"
"k8s.io/client-go/rest"
Expand Down Expand Up @@ -53,6 +54,7 @@ type Harness struct {
stopping bool
bgProcesses []*exec.Cmd
report *report.Testsuites
RunLabels labels.Set
}

// LoadTests loads all of the tests in a given directory.
Expand Down Expand Up @@ -85,6 +87,7 @@ func (h *Harness) LoadTests(dir string) ([]*Case, error) {
Dir: filepath.Join(dir, file.Name()),
SkipDelete: h.TestSuite.SkipDelete,
Suppress: h.TestSuite.Suppress,
RunLabels: h.RunLabels,
})
}

Expand Down
Loading