cluster-api
cluster-api copied to clipboard
kubernetes-sigs
🌱 Update tests to Ginkgo v2
What this PR does / why we need it:
Updates CAPI to Ginkgo v2.
Kubernetes just migrated in kubernetes/kubernetes#109111. I attempted to update Ginkgo in CAPZ but can't because of test framework depencies here in CAPI.
The reporting framework has changed significantly: essentially, custom reporters are out. The --junit-report and related flags should give us similar output files, but let's see.
Which issue(s) this PR fixes:
Fixes #4897
cc: @vladimirvivien
I think we should only do this if there is a good reason why we have to use ginkgo v2 in CAPI v1.2
+1, it does not seem to be the case at the moment.
/hold
In today's CAPI office hours meeting, we agreed to merge this in a month, because we assume providers are still integrating the v1.2.0 release. I will send an email to the list announcing that timeline so providers have time to prepare for this breaking change.
Looks like it's counting SynchronizedBeforeSuite as a spec in the Junit reporting, not sure if that's expected or not
cc @schrej If you have some time can you please take a look at this PR as well, given your experience working on the Ginkgo v2 bump in CR.
Switching imports and replacing the custom JUnit reporter with command line flags is what we've done on CR as well, so /lgtm
Should maybe be a :warning: in the title though?
Should we add a note to the 1.2->1.3 guide mentioning this?
Yes. We should definitely mention:
- that the ginkgo & gomega dependencies were upgraded and thus all consumers have to ugprade as well
- the default timeout has been changed to 1h (https://onsi.github.io/ginkgo/MIGRATING_TO_V2#timeout-behavior)
- -juni-report has to be used instead of the junit reporter
- xref to this PR for an example-upgrade & to https://onsi.github.io/ginkgo/MIGRATING_TO_V2
@sbueringer: The following commands are available to trigger required jobs:
-
/test pull-cluster-api-build-main -
/test pull-cluster-api-e2e-main -
/test pull-cluster-api-test-main -
/test pull-cluster-api-test-mink8s-main -
/test pull-cluster-api-verify-main
The following commands are available to trigger optional jobs:
-
/test pull-cluster-api-apidiff-main -
/test pull-cluster-api-e2e-full-main -
/test pull-cluster-api-e2e-informing-ipv6-main -
/test pull-cluster-api-e2e-informing-main -
/test pull-cluster-api-e2e-workload-upgrade-1-21-1-22-main -
/test pull-cluster-api-e2e-workload-upgrade-1-24-latest-main
Use /test all to run the following jobs that were automatically triggered:
-
pull-cluster-api-apidiff-main -
pull-cluster-api-build-main -
pull-cluster-api-e2e-informing-ipv6-main -
pull-cluster-api-e2e-informing-main -
pull-cluster-api-e2e-main -
pull-cluster-api-test-main -
pull-cluster-api-test-mink8s-main -
pull-cluster-api-verify-main
In response to this:
/test ?
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.
/test pull-cluster-api-e2e-full-main /test pull-cluster-api-e2e-workload-upgrade-1-21-1-22-main /test pull-cluster-api-e2e-workload-upgrade-1-24-latest-main
Want to see if there are any side effects on the upgrade jobs which are running the upstream conformance tests
Otherwise lgtm from my side
Should we add a note to the 1.2->1.3 guide mentioning this?
👍🏻 I see that we have mentioned test framework changes before under the Other: section in version migration docs, so I'll add that.
/test pull-cluster-api-e2e-full-main /test pull-cluster-api-e2e-workload-upgrade-1-21-1-22-main /test pull-cluster-api-e2e-workload-upgrade-1-24-latest-main
I assume otherwise we're basically ready to merge (considering the mail in which we wrote we want to merge today :)).
cc @CecileRobertMichon @fabriziopandini
Looks like it's counting SynchronizedBeforeSuite as a spec in the Junit reporting, not sure if that's expected or not
@CecileRobertMichon I see that changed, but I haven't been able to track down where and why. I assume it's part of the new --junit-report implementation. It seems benign, but I can look into it more deeply if we don't like this behavior.
Looks like it's counting SynchronizedBeforeSuite as a spec in the Junit reporting, not sure if that's expected or not
@CecileRobertMichon I see that changed, but I haven't been able to track down where and why. I assume it's part of the new
--junit-reportimplementation. It seems benign, but I can look into it more deeply if we don't like this behavior.
It would be interesting to know, but definitely not a showstopper for me.
[APPROVALNOTIFIER] This PR is APPROVED
This pull-request has been approved by: CecileRobertMichon
The full list of commands accepted by this bot can be found here.
The pull request process is described here
- ~~OWNERS~~ [CecileRobertMichon]
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
I see that changed, but I haven't been able to track down where and why. I assume it's part of the new --junit-report implementation. It seems benign, but I can look into it more deeply if we don't like this behavior.
definitely not a blocker, just seemed interesting
/hold cancel
I think this is ready to merge. Thanks everyone for the reviews and the help!