tests: environement setup and other refactorings #3550

vdemeester · 2020-11-20T16:59:29Z

Changes

This update the e2e tests suite to use TestMain in order to set up a
environment object for the whole test. This object is meant to store
anything that could be useful for the test to gather information from
it and act based on it (skipping, getting the right image, …)

In addition, this extract the clients into their own package, as well
as the log helpers.

It also refactor a bit how multi-arch is handled, by porting most of
the feature into the environment.

There is a more improvements to do based on this, but this is a first step.

/kind misc
/area testing

/cc @barthy1 @imjasonh @mattmoor

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices
Release notes block has been filled in or deleted (only if no user facing changes)

See the contribution guide for more details.

Double check this list of stuff that's easy to miss:

If you are adding a new binary/image to the cmd dir, please update
the release Task to build and release this image.

Reviewer Notes

If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.

Release Notes

NONE

barthy1 · 2020-11-20T18:34:06Z

@vdemeester just ping me, when it's ready to run on s390x env :)

vdemeester · 2020-11-23T13:30:25Z

Note on possible follow-up:

Run more tests in the same namespace. We create a namespace for almost each and every test (even with table-driven ones). It costs a little bit of testing time.
Add some "requirement" or "skip" framework (not necessarly an external one)
Expose this to other projects (triggers, cli, …)

vdemeester · 2020-11-23T15:36:33Z

no child with platform linux/linux

Oups 😅

This update the e2e tests suite to use `TestMain` in order to set up a environment object for the whole test. This object is meant to store anything that could be useful for the test to gather information from it and act based on it (skipping, getting the right image, …) In addition, this extract the clients into their own package, as well as the log helpers. It also refactor a bit how multi-arch is handled, by porting most of the feature into the environment. Signed-off-by: Vincent Demeester <vdemeest@redhat.com>

vdemeester · 2020-11-25T09:14:53Z

It is ready for review @barthy1
/cc @afrittoli

barthy1 · 2020-11-25T12:59:34Z

test/helm_task_test.go

@@ -42,13 +45,17 @@ var (
 // TestHelmDeployPipelineRun is an integration test that will verify a pipeline build an image
 // and then using helm to deploy it
 func TestHelmDeployPipelineRun(t *testing.T) {
+	if testEnv.Platform == "linux/s390x" {
+		t.Skipf("skip for s390x architecture")
+	}


May be it's better to have 1 entry point for different e2e tests to skip, which can be extended for other architectures? For instance in setupWithCleanup?

So I want to make the skip very explicit per tests. Aka, if a test needs to be skip for whatever reason, it needs to be known when reading the test. In docker/docker we used the concept of requirements (requirements.IsNetworkAvailable, …)

ok, I have to say I like to keep related things in one place, just not to spam the code with if statements for different cases (and many archs in the future), however I'd like to believe that all tests will work for all archs 🤞 and there won't be many exclusions required..

example in docker/docker: https://github.com/moby/moby/blob/master/integration/container/create_test.go#L74

barthy1 · 2020-11-25T13:00:35Z

test/internal/environment/environment.go

+	return &Execution{
+		Clients:     c,
+		Info:        v,
+		Platform:    v.Platform,


I really like this option to get platform from actual target k8s cluster, so no need any more to use TEST_RUNTIME_ARCH, in case when tests are initiated on machine of one arch, but will be executed on k8s cluster with another arch 👍
Can you also remove mention about this env variable from doc ?

we can yes 🙃

barthy1 · 2020-11-25T13:07:18Z

test/examples_test.go

 // koCreate wraps the ko binary and invokes `ko create` for input within
 // namespace
 func koCreate(input []byte, namespace string) ([]byte, error) {
-	cmd := exec.Command("ko", "create", "--platform", "linux/"+getTestArch(), "-n", namespace, "-f", "-")
+	cmd := exec.Command("ko", "create", "--platform", testEnv.Platform, "-n", namespace, "-f", "-")


After test case "initiate tests on amd64, execute on s390x", learnt that --kubeconfig should be added here. However it probably can be separate PR, as this functionality is not available in the initial code.

@barthy1 I am not sure I understand 🤔

If go test is running with --kubeconfig, ko create won't have knowledge about the special kubeconfig and will be targeted to default cluster. So kubeconfig should be propagated to ko create

@barthy1 ahhh very good point ! Yeah this can be done in a follow-up or now, I'll see if I've got some time to fix

barthy1 · 2020-11-25T13:09:20Z

the e2e and examples tests passed for s390x. Provided just a few suggestions from my side.

vdemeester · 2020-12-01T09:58:08Z

/cc @tektoncd/core-maintainers

tekton-robot · 2020-12-03T13:13:57Z

@vdemeester: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

ghost · 2020-12-15T14:58:37Z

test/artifact_bucket_test.go

-	if configFilePath == "" {
-		t.Skip("GCP_SERVICE_ACCOUNT_KEY_PATH variable is not set.")
+	if !testEnv.IsRunningOnGCP() {
+		t.Skip("This tests only runs on GCP.\n GCP_SERVICE_ACCOUNT_KEY_PATH variable is not set.")


Nit: we're refererring to GCP_SERVICE_ACCOUNT_KEY_PATH here as well as in IsRunningOnGCP(). Could IsRunningOnGCP return an error of form errors.New("GCP_SERVICE_ACCOUNT_KEY_PATH variable not set") instead and then this code could just do t.Skip(fmt.Sprintf("This test only runs on GCP.\n%v", err)). This way the "knowledge" of GCP_SERVICE_ACCOUNT_KEY_PATH is kept entirely with the IsRunningOnGCP func.

Might even be worth putting "GCP_SERVICE_ACCOUNT_KEY_PATH" into a named constant in testEnv or alternatively create a func that looks it up? Then we could use it on line 64?

tekton-robot · 2020-12-15T17:26:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbwsg

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [sbwsg]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

tekton-robot · 2021-03-11T15:30:12Z

@vdemeester: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-tekton-pipeline-unit-tests	`7aac53b`	link	`/test tekton-pipeline-unit-tests`
pull-tekton-pipeline-go-coverage	`7aac53b`	link	`/test pull-tekton-pipeline-go-coverage`
pull-tekton-pipeline-build-tests	`7aac53b`	link	`/test pull-tekton-pipeline-build-tests`
pull-tekton-pipeline-integration-tests	`7aac53b`	link	`/test pull-tekton-pipeline-integration-tests`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

vdemeester · 2021-03-11T15:30:59Z

/close
I'll rework this at some point.

tekton-robot · 2021-03-11T15:31:00Z

@vdemeester: Closed this PR.

In response to this:

/close
I'll rework this at some point.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

tekton-robot requested review from barthy1, imjasonh and mattmoor November 20, 2020 16:59

vdemeester force-pushed the e2e-env-and-skip branch from f6739b8 to 49da777 Compare November 23, 2020 13:21

vdemeester force-pushed the e2e-env-and-skip branch 4 times, most recently from a43eef5 to 9ce12f8 Compare November 23, 2020 14:24

vdemeester force-pushed the e2e-env-and-skip branch from 9ce12f8 to 7aac53b Compare November 23, 2020 16:26

vdemeester changed the title ~~[wip] tests: environement setup and other refactorings~~ tests: environement setup and other refactorings Nov 25, 2020

tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 25, 2020

tekton-robot requested a review from afrittoli November 25, 2020 09:14

barthy1 reviewed Nov 25, 2020

View reviewed changes

tekton-robot requested a review from a team December 1, 2020 09:58

vdemeester mentioned this pull request Dec 1, 2020

v1alpha1 e2e tests not being updated along with v1beta1 tests #3163

Closed

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 3, 2020

vdemeester mentioned this pull request Dec 14, 2020

Adding power (ppc64le) architecture image mappings #3630

Merged

4 tasks

ghost approved these changes Dec 15, 2020

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 15, 2020

Base automatically changed from master to main March 11, 2021 15:28

tekton-robot closed this Mar 11, 2021

vdemeester deleted the e2e-env-and-skip branch July 3, 2024 07:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tests: environement setup and other refactorings #3550

tests: environement setup and other refactorings #3550

vdemeester commented Nov 20, 2020

barthy1 commented Nov 20, 2020

vdemeester commented Nov 23, 2020

vdemeester commented Nov 23, 2020

vdemeester commented Nov 25, 2020

barthy1 Nov 25, 2020

vdemeester Nov 25, 2020

barthy1 Nov 25, 2020

vdemeester Nov 25, 2020

barthy1 Nov 25, 2020

vdemeester Nov 25, 2020

barthy1 Nov 25, 2020

vdemeester Nov 25, 2020

barthy1 Nov 25, 2020 •

edited

Loading

vdemeester Nov 25, 2020

barthy1 commented Nov 25, 2020

vdemeester commented Dec 1, 2020

tekton-robot commented Dec 3, 2020

ghost Dec 15, 2020

tekton-robot commented Dec 15, 2020

tekton-robot commented Mar 11, 2021

vdemeester commented Mar 11, 2021

tekton-robot commented Mar 11, 2021

tests: environement setup and other refactorings #3550

tests: environement setup and other refactorings #3550

Conversation

vdemeester commented Nov 20, 2020

Changes

Submitter Checklist

Reviewer Notes

Release Notes

barthy1 commented Nov 20, 2020

vdemeester commented Nov 23, 2020

vdemeester commented Nov 23, 2020

vdemeester commented Nov 25, 2020

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

barthy1 Nov 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barthy1 commented Nov 25, 2020

vdemeester commented Dec 1, 2020

tekton-robot commented Dec 3, 2020

Choose a reason for hiding this comment

tekton-robot commented Dec 15, 2020

tekton-robot commented Mar 11, 2021

vdemeester commented Mar 11, 2021

tekton-robot commented Mar 11, 2021

barthy1 Nov 25, 2020 •

edited

Loading