Ginkgo V2 migration

[Dave Chen]

Hello everyone, 

I have been working on the Ginkgo V2 migration for a while recently, the impact was far beyond my expectations when I first saw that something might benefit from the Ginkgo V2. e.g. the issue[1].

Seen from the Ginkgo website[2], the Ginkgo V1 was deprecated  and all the development will be on version2 only, we'd better to migrate to V2 accordingly so that 

- we can remove the workaround introduced by [1].

- benefit from the new features, e.g. there is no need to write custom reporter for generating JUnit reports only as this is already a built-in feature for Ginkgo V2.

Most of work items has been done with the PR[3], which is including,

1) Replace all the imports statement from Ginkgo V1 to V2.

2) Refactoring on the consts and parameters , such as "GINKGO_PANIC", "timeout" parameters.

3) Switch to the new way to config suite and report, for example, how to config "EmitSpecProgress" and "RandomizeAllSpecs" for test suite.

4) Define `SkipStrings` in `BeforeEach`, the old way to config `SkipStrings` doesn't work anymore.

5) Switch to use `SuiteConfig.ParallelProcess` to define the number of parallel processes.

6) Update the signature for the custom reporters. These custom reporters are non-op now and will be cleaned after the migration.

7) Support to build the Ginkgo V2 binary.

8) Cleanup all the tests around the custom reporters.

9) Workaround the spec dump with dry-run mode, see [5] (will upgrade the Ginkgo to the minor version once the release is out).

10) Support to generate conformance test spec with V2 as the stacktrace is dropped in V2, see [6].

11) Change the way to verify the test spec as the struct of test spec has been changed.

12) update the CI which are hard-coded with Ginkgo v1 binary, e.g. [4]

...

Here are some work items left to address,

1) "pull-kubernetes-e2e-aks-engine-windows-containerd", "pull-kubernetes-e2e-ubuntu-gce-network-policies", one testcase failed with timeout exception, need to investigate.

2) Cleanup all the custom reporters, this can be done later in a separate PR, the most important custom report "NewDetailsReporterFile" has been already implemented in the PR[3], junit custom report can be simply removed.

3) remove all the workaround introduced by V1.

Since the change is big and the impact is big too, at least "test-infra" and "kind" should update the Ginkgo binary from v1 to v2 (could be conditionally build the binary). I am wrote this email to hope get your feedback on this.

And last, is a KEP is needed for the change like this? If yes, I'd happy to wrote a KEP firstly for review. 

[1] https://github.com/onsi/ginkgo/issues/222

[2] https://onsi.github.io/ginkgo/MIGRATING_TO_V2

[3] https://github.com/kubernetes/kubernetes/pull/109111

[4] https://github.com/kubernetes-sigs/kind/pull/2719

[5] https://github.com/onsi/ginkgo/issues/935

[6] https://github.com/onsi/ginkgo/issues/960

Best Regards, 

Dave Chen

[Patrick Ohly]

Dave Chen <dave.j...@gmail.com> writes:

> Hello everyone,
>
> I have been working on the Ginkgo V2 migration for a while recently, the
> impact was far beyond my expectations when I first saw that something might
> benefit from the Ginkgo V2. e.g. the issue[1].

Ginkgo v2 also supports adding custom labels to a test. Long-term that
could replace our use of [Conformance] and [Feature:...] tags in the
text for a test (https://pkg.go.dev/github.com/onsi/ginkgo/v2#Label)

It improves support for running some tests serially
(https://pkg.go.dev/github.com/onsi/ginkgo/v2#pkg-constants). This can
replace invoking ginkgo twice (once for serial tests, once for parallel
ones).

But the important part is first to migrate. Then we can slowly start
using new features.

> Here are some work items left to address,
>
> 1) "pull-kubernetes-e2e-aks-engine-windows-containerd",
> "pull-kubernetes-e2e-ubuntu-gce-network-policies", one testcase failed with
> timeout exception, need to investigate.
> 2) Cleanup all the custom reporters, this can be done later in a separate
> PR, the most important custom report "NewDetailsReporterFile" has been
> already implemented in the PR[3], junit custom report can be simply removed.
> 3) remove all the workaround introduced by V1.
>
>
> Since the change is big and the impact is big too, at least "test-infra"
> and "kind" should update the Ginkgo binary from v1 to v2 (could be
> conditionally build the binary). I am wrote this email to hope get your
> feedback on this.

Do we have jobs that build ginkgo themselves, without the make rules
from k/k? I know that I am doing that in kubernetes-csi jobs to test CSI
drivers with the in-tree storage test suite.

The ginkgo binary has to match the ginkgo library used by the test
binary, so some extra care is needed in such jobs.

> And last, is a KEP is needed for the change like this? If yes, I'd happy to
> wrote a KEP firstly for review.

IMHO it doesn't need a KEP. But the PR should be marked as a breaking
change because consumers of the e2e.test binary (primarily users who run
conformance tests, but also CSI driver developers) must be aware of the
need to switch to the ginkgo v2 binary.

--
Best Regards

Patrick Ohly
Cloud Software Architect

Intel GmbH
System Software Engineering/Cloud Native
Usenerstr. 5a Phone: +49-228-2493652
53129 Bonn
Germany

[antonio.o...@gmail.com]

On Wednesday, 20 April 2022 at 08:59:31 UTC+2 patric...@intel.com wrote:

Dave Chen <dave.j...@gmail.com> writes:

> Hello everyone,
>
> I have been working on the Ginkgo V2 migration for a while recently, the
> impact was far beyond my expectations when I first saw that something might
> benefit from the Ginkgo V2. e.g. the issue[1].

Ginkgo v2 also supports adding custom labels to a test. Long-term that
could replace our use of [Conformance] and [Feature:...] tags in the
text for a test (https://pkg.go.dev/github.com/onsi/ginkgo/v2#Label)

It improves support for running some tests serially
(https://pkg.go.dev/github.com/onsi/ginkgo/v2#pkg-constants). This can
replace invoking ginkgo twice (once for serial tests, once for parallel
ones).

But the important part is first to migrate. Then we can slowly start
using new features.

I'm +1 on this 

> Here are some work items left to address,
>
> 1) "pull-kubernetes-e2e-aks-engine-windows-containerd",
> "pull-kubernetes-e2e-ubuntu-gce-network-policies", one testcase failed with
> timeout exception, need to investigate.

 

> 2) Cleanup all the custom reporters, this can be done later in a separate
> PR, the most important custom report "NewDetailsReporterFile" has been
> already implemented in the PR[3], junit custom report can be simply removed.

That is the part that I didn't get feedback when we discussed this Gingokv2 topic last year,

are these custom reporters used somewhere? 

 

> 3) remove all the workaround introduced by V1.
>

>
> Since the change is big and the impact is big too, at least "test-infra"
> and "kind" should update the Ginkgo binary from v1 to v2 (could be
> conditionally build the binary). I am wrote this email to hope get your
> feedback on this.

Do we have jobs that build ginkgo themselves, without the make rules
from k/k? I know that I am doing that in kubernetes-csi jobs to test CSI
drivers with the in-tree storage test suite.

I think that's the thing that we are going to find out during the process :-)

 

The ginkgo binary has to match the ginkgo library used by the test
binary, so some extra care is needed in such jobs.

> And last, is a KEP is needed for the change like this? If yes, I'd happy to
> wrote a KEP firstly for review.

IMHO it doesn't need a KEP. But the PR should be marked as a breaking
change because consumers of the e2e.test binary (primarily users who run
conformance tests, but also CSI driver developers) must be aware of the
need to switch to the ginkgo v2 binary.

The biggest drama should be on the people importing the e2e test framework,

I think that there are some places indicating that there are no guarantees and 

that should not be done, because it can break between versions,  anyway I

think that we should communicate it in advance, maybe this email thread is 

enough?