kubernetes-sigs
⚠ Migrate to ginkgo v2
Revives #1792, thanks to @acumino for all your work on this.
Fixes: #1545
Important Note: -> Per node timeout has been removed to prevent the wrongly marking a test as failure which is caused by some other test. Read more about it here -> Custom reporters have been deprecated in ginkgo v2 and will be removed in future releases.
Note: This is a breaking change for two reasons:
- (we're not affected by this) ginkgo v1 and v2 have conflicting flags, so you can't import them both at the same time (see here). Thanks to @mboersma for pointing this out
- The PR removes two helpers in
pkg/envtest/printerthat were used with ginkgo v1. Since you can't use both versions at the same time, this isn't relevant though. In summary, all projects depending on controller-runtime will have to update to ginkgo v2 after this is merged and released. Upstream did so already: https://github.com/kubernetes/kubernetes/pull/109111
/cc @matteoolivi @camilamacedo86
@schrej: GitHub didn't allow me to request PR reviews from the following users: matteoolivi.
Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.
In response to this:
/cc @matteoolivi @camilamacedo86
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.
ginkgo v1 and v2 have conflicting flags, so you can't import them both at the same time (see https://github.com/onsi/ginkgo/issues/875#issuecomment-1036350659). Thanks to @mboersma for pointing this out
But we only import it in _test.go files and in the printer package, so this shouldn't matter for consumers, right?
But we only import it in _test.go files and in the printer package, so this shouldn't matter for consumers, right?
I just tested with the cluster-api project, manually replacing controller-runtime with this branch there. Tests can still run, even though CAPI is still on ginkgo v1. So you're right, it shouldn't matter to consumers in our case.
I'll revert the removal of the two helpers in pkg/envtest/printer, which should make this a non-breaking change then.
Ok. This is now using the ginkgo cli, which allows to remove the ginkgo helper to create unit test reports.
The apidiff is failing because of that.
I've adapted the test scripts to install the ginkgo cli automatically and to copy the unit test report to the correct location for prow to detect and visualise it.
One major drawback of the ginkgo cli is that it does not cache tests, and always runs the full suite.
@schrej the tests paniced but still passed: https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_controller-runtime/1977/pull-controller-runtime-test-master/1559847725640454144
@schrej the tests paniced but still passed: https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_controller-runtime/1977/pull-controller-runtime-test-master/1559847725640454144
Yeah. ~~Apparently ginkgo detects and reports panics even when they are intentional ("injected panic").~~
•••E0817 10:24:49.659771 23457 runtime.go:77] Observed a panic: injected panic
The k8s.io/klog package logs panics it observes to stderr, and Ginkgo apparently prints that even when the test passes.
The k8s.io/klog package logs panics it observes to stderr, and Ginkgo apparently prints that even when the test passes.
It's the runtime which calls klog.Error, which then seems to go to stderr.
Does the suite call klog.SetOutput(ginkgo.GinkgoWriter)? Then I think you can suppress that extra output to stderr by reconfiguring the "stderrthreshold". I have a pending commit which does that for the in-tree e2e.test:
https://github.com/kubernetes/kubernetes/pull/111023/commits/7d6af86168eede751865988664638a2037596e0d
I've adapted the test scripts to install the ginkgo cli automatically and to copy the unit test report to the correct location for prow
FYI, it is possible to enable JUnit output without using the ginkgo cli. You can either do go test ./<test dir> -args -ginkgo.junit-report=<output file> or add some code like https://github.com/kubernetes/kubernetes/blob/a1128e380c2cf1c2d7443694673d9f1dd63eb518/test/e2e/framework/test_context.go#L558-L607.
FYI, it is possible to enable JUnit output without using the ginkgo cli. You can either do go test ./
-args -ginkgo.junit=
The issue with that was that it made the tests always exit with rc0 even when there are failures
The issue with that was that it made the tests always exit with rc0 even when there are failures
Which of the two options? -ginkgo.junit or adding some custom GenerateJUnitReport invocation? Either way, that sounds like a bug that should be reported.
Hmm, both works for me (PMEM-CSI test/e2e, using ginkgo v2 v2.1.4).
FYI, I have to use this fork of the controller-runtime for that project because otherwise I would be stuck at ginkgo v1 and couldn't update to Kubernetes 1.25. I'll have to keep that in a WIP PR until this PR gets merged and released.
Which of the two options? -ginkgo.junit or adding some custom GenerateJUnitReport invocation? Either way, that sounds like a bug that should be reported.
It was reported here, maybe it doesn't happen anymore? https://github.com/onsi/ginkgo/issues/706
FYI, I have to use this fork of the controller-runtime for that project because otherwise I would be stuck at ginkgo v1 and couldn't update to Kubernetes 1.25. I'll have to keep that in a WIP PR until this PR gets merged and released.
Why? Shouldn't we only depend on ginkgo in _test.go files? If anyhow possible, we shouldn't force users to use either version
Why? Shouldn't we only depend on ginkgo in _test.go files? If anyhow possible, we shouldn't force users to use either version
Probably because we're currently importing ginkgo v1 in pkg/envtest/printer.
Probably because we're currently importing ginkgo v1 in pkg/envtest/printer.
But v2 is imported as github.com/onsi/ginkgo/v2 or not?
Yes, but importing both at the same time causes issues with flag parsing as mentioned in the PR description.
I'm almost done removing the ginkgo cli again. It solves the logging issue and seems to report status just fine.
All packages need to import _ "github.com/onsi/ginkgo/v2", even if they don't contain ginkgo tests when we want to run tests with the ginkgo.junit-report flag.
Which of the two options? -ginkgo.junit or adding some custom GenerateJUnitReport invocation? Either way, that sounds like a bug that should be reported.
It was reported here, maybe it doesn't happen anymore? onsi/ginkgo#706
That looks like a different problem to me: exit code was zero when it shouldn't be because of a failed test, but it's not clear that this was caused by using the JUnit reporter.
All packages need to import _ "github.com/onsi/ginkgo/v2", even if they don't contain ginkgo tests when we want to run tests with the ginkgo.junit-report flag.
If a package doesn't contain ginkgo tests, then how can ginkgo gather results for the tests in the package? Presumably those are plain func TestFoo(t *testing.T) tests? Can you give an example?
Importing ginkgo into such a Go test package adds the ginkgo command line flags, but they don't have any effect at runtime.
If a package doesn't contain ginkgo tests, then how can ginkgo gather results for the tests in the package? Presumably those are plain
func TestFoo(t *testing.T)tests? Can you give an example?Importing ginkgo into such a Go test package adds the ginkgo command line flags, but they don't have any effect at runtime.
It can't. But go test will complain about the undefined command line arguments if it's not imported.
In summary, all projects depending on controller-runtime will have to update to ginkgo v2 after this is merged and released.
This is really, really bad. Is there any way to fix ginkgo, has this issue bee raised there?
This is really, really bad. Is there any way to fix ginkgo, has this issue bee raised there?
This cannot be fixed. The Ginkgo API is designed around global variables and directly extending the default FlagSet, to simplify test startup. This could have been solved differently, but cannot be changed anymore without another breaking change.
Even if ginkgo v1 and v2 could co-exist, what would be the point? Tests registered with v1 Describe would be invisible to the test runner from ginkgo v2 and vice versa.
What you could do in controller-runtime is to limit the usage of ginkgo to _test.go files. Then consumers of the controller-runtime are free to choose whatever version of ginkgo they want to use in their tests. Actually, I am not sure anymore why controller-runtime seemed to pull in ginkgo v1 into PMEM-CSI. The only non-test files which use ginkgo are pkg/envtest/printer and those are not required. I've reverted the usage of this fork in PMEM-CSI and don't see any problems.
This is really, really bad. Is there any way to fix ginkgo, has this issue bee raised there?
That's outdated by now. I've tested it in the beginning and it was not necessary to upgrade everything to v2. As @pohly said, if we only import ginkgo in _test.go files there won't be a conflict, and we're currently doing that.
Okay, that is awesome. I have a really busy week atm, will try to look at this next week. If someone else has time for a first pass, that would be great :)
This is really, really bad. Is there any way to fix ginkgo, has this issue bee raised there?
That's outdated by now. I've tested it in the beginning and it was not necessary to upgrade everything to v2. As @pohly said, if we only import ginkgo in
_test.gofiles there won't be a conflict, and we're currently doing that.
+1. Just another data point (although in the other direction). We bumped to ginkgo v2 in ClusterAPI and are still using the regular controller-runtime v0.12.3 release without problems
[APPROVALNOTIFIER] This PR is APPROVED
This pull-request has been approved by: schrej, vincepri
The full list of commands accepted by this bot can be found here.
The pull request process is described here
- ~~OWNERS~~ [vincepri]
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
Should be the same issue as on https://github.com/kubernetes-sigs/controller-runtime/pull/1986
Essentially the next PR that goes in has to be a golangci-lint bump
(cause is probably that we bumped the jobs to use go 1.19 images)