test: Rename test names to follow a well defined pattern

[bpradipt]

Tests are named using the pattern Test This helps to filter tests and run specific categories as needed.

[bpradipt]

Folks, ptal a look and let me know if this approach sounds good. With the current code, it's really hard to run specific set of tests on need basis so I'm starting with a well-defined and opinionated patter for the names.
I haven't modified the gh actions yet.

[stevenhorsman]

Folks, ptal a look and let me know if this approach sounds good. With the current code, it's really hard to run specific set of tests on need basis so I'm starting with a well-defined and opinionated patter for the names. I haven't modified the gh actions yet.

Thanks for this - I think it's good, but I wonder how you are considering the aim of our to have a "BVT" suite for running on the nightly as I think we'd want to cross-cut multiple of these categories, but not run all the tests within these?

[bpradipt]

Folks, ptal a look and let me know if this approach sounds good. With the current code, it's really hard to run specific set of tests on need basis so I'm starting with a well-defined and opinionated patter for the names. I haven't modified the gh actions yet.

Thanks for this - I think it's good, but I wonder how you are considering the aim of our to have a "BVT" suite for running on the nightly as I think we'd want to cross-cut multiple of these categories, but not run all the tests within these?

One approach I am thinking is to have separate labels for each categories and then triggering a suite (say nightly or bvt etc) via grouping the labels. Do you think this approach will work with GHA ?

[stevenhorsman]

One approach I am thinking is to have separate labels for each categories and then triggering a suite (say nightly or bvt etc) via grouping the labels. Do you think this approach will work with GHA ?

Sorry, I don't understand this proposal? What do you mean by labels?

[bpradipt]

One approach I am thinking is to have separate labels for each categories and then triggering a suite (say nightly or bvt etc) via grouping the labels. Do you think this approach will work with GHA ?

Sorry, I don't understand this proposal? What do you mean by labels?

I meant the labels that you use today to trigger the e2e tests. Like test_e2e_docker

[stevenhorsman]

One approach I am thinking is to have separate labels for each categories and then triggering a suite (say nightly or bvt etc) via grouping the labels. Do you think this approach will work with GHA ?

Sorry, I don't understand this proposal? What do you mean by labels?

I meant the labels that you use today to trigger the e2e tests. Like test_e2e_docker

Magnus' proposal is that we run a cut down version of the e2e tests on nightly to improve the reliability (what I'm calling the BVT), but run the full e2e tests on PRs.

It sounds like you are working on an orthogonal idea and you don't want to run the full e2e tests on PRs, but only selective tests driven by a label? What's the reason behind this as it feels like we run full tests on PRs to try and ensure that we don't break the main branch?

[bpradipt]

One approach I am thinking is to have separate labels for each categories and then triggering a suite (say nightly or bvt etc) via grouping the labels. Do you think this approach will work with GHA ?

Sorry, I don't understand this proposal? What do you mean by labels?

I meant the labels that you use today to trigger the e2e tests. Like test_e2e_docker

Magnus' proposal is that we run a cut down version of the e2e tests on nightly to improve the reliability (what I'm calling the BVT), but run the full e2e tests on PRs.

It sounds like you are working on an orthogonal idea and you don't want to run the full e2e tests on PRs, but only selective tests driven by a label? What's the reason behind this as it feels like we run full tests on PRs to try and ensure that we don't break the main branch?

Sorry for the confusion. In this PR, my only intention is to make it easier to run group of tests via regular expression (via setting the RUN_TESTS variable) which is not possible today.
This name change doesn't impact the current way of running the tests. IOW, you continue to run the full set of tests for every PR.

However we do need a way to run some specific tests for some PR so as to not overload the runners. This is where I was thinking we can have GH labels which can be mapped to specific regular expressions to allow a subset of tests to run.
As for nightlies, since those are run via scheduled jobs, we can use regular expressions for the providers as needed.

Does this help ?

[stevenhorsman]

Does this help ?

Not really - I don't understand when we would use the labels? However, my naive approach for the BVT was going to be a relabel that clashes with what you did e.g. TestBVTCreateSimplePod, but we could just switch that to the end and then not clash e.g. TestBasicCreateSimplePodBVT, so I don't think this will get in the way.

[bpradipt]

Does this help ?

Not really - I don't understand when we would use the labels? However, my naive approach for the BVT was going to be a relabel that clashes with what you did e.g. TestBVTCreateSimplePod, but we could just switch that to the end and then not clash e.g. TestBasicCreateSimplePodBVT, so I don't think this will get in the way.

Can you run the following as part of bvt.yaml GHA workflow for example ?

RUN_TESTS='^TestBasic' CLOUD_PROVIDER=libvirt make test-e2e

[bpradipt]

Looks like the docker e2e is broken due to some recent changes :-(. I'm looking into it. Will send PR soon.

[stevenhorsman]

Does this help ?

Not really - I don't understand when we would use the labels? However, my naive approach for the BVT was going to be a relabel that clashes with what you did e.g. TestBVTCreateSimplePod, but we could just switch that to the end and then not clash e.g. TestBasicCreateSimplePodBVT, so I don't think this will get in the way.

Can you run the following as part of bvt.yaml GHA workflow for example ?

RUN_TESTS='^TestBasic' CLOUD_PROVIDER=libvirt make test-e2e

I don't think your definition of the labels matches with what we'd want in the BVT. I'd expect us to have some of Basic like TestBasicAwsCreateSimplePod, TestBasicAwsCreateSimplePod for example but not all things like the nginx deployment that is fussy, or variants with configmap and secret tests. However I'd then expect some tests from other categories e.g. key release, image decrypt and maybe the golden path in Sec - TestSecDockerCreatePeerPodWithAuthenticatedImageWithImagePullSecretOnPod, but not the negative tests. Does that help, or do you think I've got the breakdown wrong?

BTW - thanks for the starting this conversation - it's really important!

[bpradipt]

Does this help ?

Not really - I don't understand when we would use the labels? However, my naive approach for the BVT was going to be a relabel that clashes with what you did e.g. TestBVTCreateSimplePod, but we could just switch that to the end and then not clash e.g. TestBasicCreateSimplePodBVT, so I don't think this will get in the way.

Can you run the following as part of bvt.yaml GHA workflow for example ?

RUN_TESTS='^TestBasic' CLOUD_PROVIDER=libvirt make test-e2e

I don't think your definition of the labels matches with what we'd want in the BVT. I'd expect us to have some of Basic like TestBasicAwsCreateSimplePod, TestBasicAwsCreateSimplePod for example but not all things like the nginx deployment that is fussy, or variants with configmap and secret tests. However I'd then expect some tests from other categories e.g. key release, image decrypt and maybe the golden path in Sec - TestSecDockerCreatePeerPodWithAuthenticatedImageWithImagePullSecretOnPod, but not the negative tests. Does that help, or do you think I've got the breakdown wrong?

BTW - thanks for the starting this conversation - it's really important!

Let's ignore the labels for now and see how we can run specific set of tests. I think that's the real issue. Or am I missing something?
Is there a way other than using regex in go test... ?

If not, then may be we can think about naming the tests in a way to include the category like BVT, Nightly etc. ?

[stevenhorsman]

Let's ignore the labels for now and see how we can run specific set of tests. I think that's the real issue. Or am I missing something? Is there a way other than using regex in go test... ?

If not, then may be we can think about naming the tests in a way to include the category like BVT, Nightly etc. ?

Yeah, AFAIK the regex is in the only way to do it, so I figured we just have a list of the tests and rename the cut down version TestBVT and then in the nightly gha with just set RUN_TESTS to be TestBVT?

[bpradipt]

Let's ignore the labels for now and see how we can run specific set of tests. I think that's the real issue. Or am I missing something? Is there a way other than using regex in go test... ?
If not, then may be we can think about naming the tests in a way to include the category like BVT, Nightly etc. ?

Yeah, AFAIK the regex is in the only way to do it, so I figured we just have a list of the tests and rename the cut down version TestBVT and then in the nightly gha with just set RUN_TESTS to be TestBVT?

When you get time, can you list down the tests you would want to run for BVT? I'll have a sample pattern and then we can iterate to see if it makes sense.

[stevenhorsman]

I created #2542 to cover this discuss and as a starting point. I hope this helps

[wainersm]

Hi @bpradipt @stevenhorsman !

Sorry, late for the party.

The problem with having the category (or grouping) in the test's name is you cannot easily share tests among different categories. The standard go testing module doesn't provide a tagging mechanism as you can find on other test frameworks, but I think we can emulate it by having test files per category and using go build labels.

For example, split in libvirt_basic_test.go and libvirt_bvt_test.go

// libvirt_basic_test.go
//go:build libvirt && basic

package e2e

import (
    "testing"
    _ "github.com/confidential-containers/cloud-api-adaptor/src/cloud-api-adaptor/test/provisioner/libvirt"
)

func TestLibvirtCreateSimplePod(t *testing.T) {
	assert := LibvirtAssert{}
	DoTestCreateSimplePod(t, testEnv, assert)
}
...
...
...

// libvirt_bvt_test.go
//go:build libvirt && bvt

package e2e

import (
    "testing"
    _ "github.com/confidential-containers/cloud-api-adaptor/src/cloud-api-adaptor/test/provisioner/libvirt"
)

func TestLibvirtCreateSimplePod(t *testing.T) {
	assert := LibvirtAssert{}
	DoTestCreateSimplePod(t, testEnv, assert)
}
...
...
...
func TestLibvirtKbsKeyRelease(t *testing.T) {
	if !isTestWithKbs() {
		t.Skip("Skipping kbs related test as kbs is not deployed")
	}

	testSecret := envconf.RandomName("coco-pp-e2e-secret", 25)
	resourcePath := "caa/workload_key/test_key.bin"
	err := keyBrokerService.SetSecret(resourcePath, []byte(testSecret))
	if err != nil {
		t.Fatalf("SetSecret failed with: %v", err)
	}
	err = keyBrokerService.EnableKbsCustomizedResourcePolicy("deny_all.rego")
	if err != nil {
		t.Fatalf("EnableKbsCustomizedResourcePolicy failed with: %v", err)
	}
	kbsEndpoint, err := keyBrokerService.GetCachedKbsEndpoint()
	if err != nil {
		t.Fatalf("GetCachedKbsEndpoint failed with: %v", err)
	}
	assert := LibvirtAssert{}
	t.Parallel()
	DoTestKbsKeyReleaseForFailure(t, testEnv, assert, kbsEndpoint, resourcePath, testSecret)
	if isTestWithKbsIBMSE() {
		t.Log("KBS with ibmse cases")
		// the allow_*_.rego file is created by follow document
		// https://github.com/confidential-containers/trustee/blob/main/deps/verifier/src/se/README.md#set-attestation-policy
		err = keyBrokerService.EnableKbsCustomizedAttestationPolicy("allow_with_wrong_image_tag.rego")
		if err != nil {
			t.Fatalf("EnableKbsCustomizedAttestationPolicy failed with: %v", err)
		}
		DoTestKbsKeyReleaseForFailure(t, testEnv, assert, kbsEndpoint, resourcePath, testSecret)
		err = keyBrokerService.EnableKbsCustomizedAttestationPolicy("allow_with_correct_claims.rego")
		if err != nil {
			t.Fatalf("EnableKbsCustomizedAttestationPolicy failed with: %v", err)
		}
		DoTestKbsKeyRelease(t, testEnv, assert, kbsEndpoint, resourcePath, testSecret)
	} else {
		t.Log("KBS normal cases")
		err = keyBrokerService.EnableKbsCustomizedResourcePolicy("allow_all.rego")
		if err != nil {
			t.Fatalf("EnableKbsCustomizedResourcePolicy failed with: %v", err)
		}
		DoTestKbsKeyRelease(t, testEnv, assert, kbsEndpoint, resourcePath, testSecret)
	}
}

And we tweak the Makefile so that the categories (list of go build labels) are passed in the call:

RUN_TEST_CATEGORIES ?= basic,net,security

.PHONY: test-e2e
test-e2e: ## Run end-to-end tests for single provider.
ifneq ($(CLOUD_PROVIDER),)
	go test -v -tags=$(CLOUD_PROVIDER),$(RUN_TEST_CATEGORIES) -timeout $(TEST_E2E_TIMEOUT) -count=1 -run $(RUN_TESTS) ./test/e2e
else
	$(error CLOUD_PROVIDER is not set)
endif

If I only want to run bvt ones make RUN_TEST_CATEGORIES=bvt test-e2e

One clear cons of that approach is that test function definitions are duplicated on many files and this can easily get out of sync. Some functions like TestLibvirtKbsKeyRelease aren't just thin wrappers to common_suite.go functions.