Refactor and clean up e2e framework utils, this patch handles test/e2e/framework/psp_util.go file

[WanLinghao]

/kind cleanup

What this PR does / why we need it:
Refactor and clean up e2e framework utils, this patch handles test/e2e/framework/perf_util.go file
ref:#76206

NONE

[WanLinghao]

/hold
wait for #77536 merged

[fejta]

Can you create OWNERS files for the psp dir please?
/assign @timothysc

[timothysc]

/approve

but you need add the generated files and OWNERs files.

[andrewsykim]

Overall lgtm, can you rebase please?

[alejandrox1]

Hi @WanLinghao any updates on this?
/assign

[WanLinghao]

@alejandrox1 Sorry for the delay, I have checked this, it is not ready to merge. We must take care of ExpectNoError functions before continue. Or it could cause package import cycle.

[alejandrox1]

@alejandrox1 Sorry for the delay, I have checked this, it is not ready to merge. We must take care of ExpectNoError functions before continue. Or it could cause package import cycle.

Sorry for the delay as well, there is actually been some debate about how to handle framework.ExpectNoError().

To get in the same page, framework.ExpectNoError() wraps a call to

framework.ExpectNoErrorWithOffset(1, err, explain...)

framework.ExpectNoErrorWithOffset() itself being a wrapper for

gomega.ExpectWithOffset(1, err).NotTo(gomega.HaveOccurred(), explain...)}

The first argument to gomega.ExpectWithOffset() represents the offset in the call stack.

For example, if you have a test, and you perform certain actions in your test through helper functions and an assertion in one of these helper function fails the line numbers provided by Gomega will point you to the line in the helper function that failed instead of showing you the line in your test that failed.

framework.ExpectNoError() will, by default, call gomega.ExpectWithOffset() with an offset equal to 1, which means that a failed assertion will be logged for the caller of the method where gomega.ExpectWithOffset() is used.

framework.ExpectNoError() was then implemented with the aim of using it within helper functions in e2e tests.
This way, if an assertion failed we would obtain information about the specific step within the e2e test that failed and would avoid having to backtrace what part of what test made a call to the given function.

With that in mind, in order to refactor code from the framework into its own package within the framework (i.e., e2e/framework/psp), we need to replace calls to framework.ExpectNoError() with either gomega.Expect() or gomega.ExectWithOffset().
The simplest way would be then to replace a framework.ExpectNoError() with the following:

if err != nil {
	e2elog.Logf("Unexpected error occurred: %v", err)
}
gomega.ExpectWithOffset(1+offset, err).NotTo(gomega.HaveOccurred(), explain...)

Which replaces the call to framework.ExpectNoError() with the code that it ends up calling.

[WanLinghao]

@alejandrox1 Hi, this patch is ready to review

[fejta]

/lgtm
/approve
/hold
Do we want an owners file for framework/psp?

[fejta]

/retest

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fejta, timothysc, WanLinghao

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:

• test/e2e/auth/OWNERS [fejta,timothysc]
• test/e2e/framework/OWNERS [fejta,timothysc]

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

[WanLinghao]

/test pull-kubernetes-e2e-gce-device-plugin-gpu

[WanLinghao]

@fejta Hi, I updated the patch to fix a golint error.

[alejandrox1]

the ExpectNoError from the framework calls gomega.ExpectWithOffset(1, err) not gomega.ExpectWithOffset(2, err), correct? If so, we should change this.
This patch changes the current behavior. Which is not wrong but we should add it in a separate PR and only focus on refactoring for this on. This way we can revert if something starts failing in CI.

/lgtm cancel

[WanLinghao]

@alejandrox1 Do you mean we should change gomega.ExpectWithOffset(2, err).NotTo(gomega.HaveOccurred(), explain...) to gomega.ExpectWithOffset(1, err).NotTo(gomega.HaveOccurred(), explain...)? But I believe the original definition should be gomega.ExpectWithOffset(2, err).NotTo(gomega.HaveOccurred(), explain...)

[alejandrox1]

Sorry, you are completely right!
Given https://github.com/kubernetes/kubernetes/blob/master/test/e2e/framework/util.go#L1321
a framework.NoExpectError is equivalent to gomega.ExpectWithOffset(2, err).NotTo(gomega.HaveOccurred(), explain...).

/lgtm
/hold cancel

[alejandrox1]

Do we want an owners file for framework/psp?

I believe the consensus thus far has to keep all e2e framework subpkgs under the same ownership.
/priority backlog

[fejta-bot]

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.