kubernetes-sigs
Mark E2E plumbing as test helpers
What type of PR is this?
/kind feature
What this PR does / why we need it:
In Crossplane we try to use reusable step functions in our E2E tests, so they end up looking like this:
func TestExample(t *testing.T) {
environment.Test(t,
features.New(t.Name()).
WithSetup("ExampleSetup", funcs.SomeSetupFn()).
Assess("ExampleAssess", funcs.SomeAssessFn()).
WithTeardown("ExampleTeardown", funcs.SomeTearDownFn()).
Feature(),
)
}
Since step functions are passed *testing.T we'll often call t.Error, t.Log, etc from our step functions. These testing calls include the call point (i.e. filename and line number) in their output. Without using t.Helper() the call point will be the file where the step function is defined (e.g. features.go) rather than the calling test.
We want to mark our step functions as test helpers using t.Helper so that the calling test is included in the output. For this to work we need to mark all the intermediary plumbing functions and methods in e2e-framework as helpers too.
Which issue(s) this PR fixes:
I didn't raise an issue tracking this.
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Internal machinery used to call step functions is now marked as test helpers. This means you can mark your step functions as a test helper using `t.Helper()`, and test logs will be include the filename and line number of `environment.Test()` call.
Additional documentation e.g., Usage docs, etc.:
Welcome @negz!
It looks like this is your first PR to kubernetes-sigs/e2e-framework 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.
You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.
You can also check if kubernetes-sigs/e2e-framework has its own contribution guidelines.
You may want to refer to our testing guide if you run into trouble with your tests not passing.
If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!
Thank you, and welcome to Kubernetes. :smiley:
Hi @negz. Thanks for your PR.
I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.
Once the patch is verified, the new status will be reflected by the ok-to-test label.
I understand the commands that are listed here.
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-sigs/prow repository.
Is this actually helping? t.Helper() just removes one frame of the stack. I.e. you see the file+line of the parent (or some ancestor) during execution of the closure returned by funcs.SomeAssessFn(), not during execution of funcs.SomeAssessFn. So you will not know which of the funcs.SomeAssessFn failed, but only the file (and test).
I see that you comment exactly that in https://github.com/crossplane/crossplane/pull/5722#issuecomment-2148427387. So this PR is better than nothing clearly.
@vladimirvivien Not that I know of. I believe it's pretty normal to call it repeatedly (e.g. if you have several layers of test helper functions).
@vladimirvivien Calling the t.Helper multiple times should be ok. It Should be using the current frames to do the analysis.
package abc
import (
"os"
"testing"
)
func TestMain(m *testing.M) {
os.Exit(m.Run())
}
func assertSomething(t *testing.T, a int) {
t.Helper()
t.Error("something failed")
}
func TestSomething(t *testing.T) {
data := []int{
1, 2, 3, 4, 5, 6, 7, 8, 9, 10,
}
for tc := range data {
assertSomething(t, tc)
}
}
This is failing consistently it seems, is there a regression ?
/retest
[APPROVALNOTIFIER] This PR is APPROVED
This pull-request has been approved by: cpanato, harshanarayana, negz
The full list of commands accepted by this bot can be found here.
The pull request process is described here
- ~~OWNERS~~ [cpanato,harshanarayana]
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment