Crash when trying to upgrade to v2

[KevinJCross]

Hi guys,
Im currently trying to upgrade to v2 on our cloudfoundry app-autoscaler project and have hit a significant blocker.

The issue will probably in more complex projects with loads of dependencies that use ginkgo from multiple versions i.e. a transient dependency (because go does not have a test dependency tree)

I believe there are at least 4 dependencies transitily including the v1 ginkgo

This means in our project we have 1.16.5 and 2.0.0 included.

       github.com/onsi/ginkgo v1.16.5 // indirect
       github.com/onsi/ginkgo/v2 v2.0.0

This does not work because of the init(){} functions in both included dependencies are run and they use the
flag.CommandLine at init time and modify it creating duplicate flags that causes panics with

/app-autoscaler-release/src/autoscaler/api/brokerserver/brokerserver.test flag redefined: ginkgo.seed
panic: /Users/kevincross/SAPDevelop/cf/app-autoscaler-release/src/autoscaler/api/brokerserver/brokerserver.test flag redefined: ginkgo.seed

goroutine 1 [running]:
flag.(*FlagSet).Var(0xc000218120, 0x1df5fc8, 0x2351f80, 0xc000357640, 0xb, 0x1cd87a3, 0x2a)
       /golang/1.16.8/go/src/flag/flag.go:871 +0x637
flag.(*FlagSet).Int64Var(...)
       /golang/1.16.8/go/src/flag/flag.go:682
github.com/onsi/ginkgo/v2/types.bindFlagSet(0xc0003cc000, 0x20, 0x21, 0x1bbd540, 0xc0003b86f0, 0x232f420, 0xd, 0xd, 0x0, 0x0, ...)
        /go/pkg/mod/github.com/onsi/ginkgo/v2@v2.0.0/types/flags.go:161 +0x15e5
github.com/onsi/ginkgo/v2/types.NewAttachedGinkgoFlagSet(...)
        /go/pkg/mod/github.com/onsi/ginkgo/v2@v2.0.0/types/flags.go:113
github.com/onsi/ginkgo/v2/types.BuildTestSuiteFlagSet(0x2351f80, 0x23519a0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
        /go/pkg/mod/github.com/onsi/ginkgo/v2@v2.0.0/types/config.go:346 +0x6e8
github.com/onsi/ginkgo/v2.init.0()
        /github.com/onsi/ginkgo/v2@v2.0.0/core_dsl.go:47 +0x8f
ginkgo run failed

After finding this I updated the v2 module to make a copy of the command line changing v2 to using a copy... this stops the panic
/v2/ginkgo/types/config.go:346

return NewAttachedGinkgoFlagSet(flag.NewFlagSet(os.Args[0], flag.ExitOnError), flags, bindings, FlagSections, extraGoFlagsSection)

but the v1 reports: flag provided but not defined: -ginkgo.randomize-all somehow 😕
I tried looking a bit further but it all gets a bit wierd with the 2nd layer of compliation 🤷

Ive also tried using the

exclude (
	github.com/onsi/ginkgo v1.16.5
)

in the go mod file but too no avail.

to replicate this error try checkout the autoscaler project https://github.com/cloudfoundry/app-autoscaler-release
checkout the branch ginkgo-v2
and run make test
this should replicate this issue easily (after a bunch of docker ect.)

[onsi]

hey @KevinJCross - presumably updating all the dependencies to v2 will resolve this but I imagine there's complexity around that. I looked at autoscaler-release/src/autoscaler/api/brokerserver to try to understand what's going on - my guess is the issue is that your updated version imports github.com/onsi/ginkgo/v2 whereas the ifrit/ginkgomon dependency imports github.com/onsi/ginkgo

I could imagine doing something ugly in Ginkgo (either v1 or v2) to sidestep flag initialization and avoid the panic. But. The reality is that you'd still be using two different major versions of Ginkgo - basically two completely different Go packages as far as Go is concerned. Those two major versions aren't going to be compatible. On the most basic level, they'll have different instances of the global suite object that Ginkgo uses to provide the DSL - but, deeper than that, the interfaces and implementations under the hood will have changed such that there are no guarantees that the two major versions can interoperate in this way.

I'm afraid the cleanest (and most sustainable) way out of this is to work towards updating all the dependencies toward support Ginkgo v2. In the case of ginkgomon I could imagine adding a new ginkgomon/v2 in-situ (or potentially pulling out a new/different library depending on how active ifrit maintenance is these days).

[onsi]

FWIW I'd be happy to try to help update a few dependencies - if you can point me at them I can spend some time submitting PRs

[KevinJCross]

Ok lets give it a go but I think Its not going to be a small task and will probably require some forks due to owner not being contactable any more.

[KevinJCross]

There are at least 5 in our deps using v1 in the project and who knows how many thay have too.

[onsi]

Can you list out and share the five?

i dare say that "this is the work" and it will be better to deal with it sooner rather than later.

[blgm]

There are situations in which it does seem to work. One of my projects has:

github.com/onsi/ginkgo/v2 v2.0.0
github.com/onsi/ginkgo v1.16.5 // indirect

And does not crash when testing. I'm not clear why it would work in one project and not another. Maybe the Go version or something makes a difference?

[onsi]

I don't think it's the Go version. I think it has to do with how the dependency is used. @KevinJCross - things may not be quite as dire as they seem:

If a dependency uses Ginkgo for it's own tests then it's fine for it to use a different major version of Ginkgo. The 1.x version will appear in go.mod but that's OK, Go will still only use v2 to run your tests. In fact, different packages within the same repo can use different version of Ginkgo.

The problem only appears if a dependency package (not it's magic test package, but the package itself) that is imported into your v2 tests uses Ginkgo v1. In the case above, ifrit's ginkgomon is a test helper that is imported into the test suite and itself imports Ginkgo v1. Now you have two versions of Ginkgo linked to the same package and assumptions about interoperability go out the window.

Best I can tell, for the autoscaler-release/src/autoscaler/api/brokerserver tests at least, it's just code.cloudfoundry.org/lager/lagertest and github.com/tedsuo/ifrit/ginkgomon that will need to be updated. And both look relatively straightforward to implement new github.com/tedsuo/ifrit/ginkgomon/v2 and code.cloudfoundry.org/lager/lagertest/v2 packages of.

(Caveat: I think code.cloudfoundry.org/lager/lagertest/v2 is legal, but it might have to be code.cloudfoundry.org/lager/lagertest_v2 - @blgm would probably know. His knowledge of all things go module/packaging related runs very deep 😉 )

[KevinJCross]

@onsi thanks for having a look ill look into this a bit more too.
so your saying if its in the project and not in the *_test package it will cause problems.
Ill have a look into this next week ... its all a bit tricky and kinda started herting my head last time I looked. 🤯

[onsi]

No, I think it's a bit different. Let me try again:

(a) If an individual package imports both Ginkgo V2 and Ginkgo V1 (either directly or via some dependency) then 💥
(b) I believe that, in your case, the only time this happens is when your code's test package (i.e. any files ending in _test.go) imports a test helper (e.g. ginkgomon or lagertest) that, itself, imports Ginkgo V1.
(c) That means you don't have to worry about all the very many dependencies out there that still use Ginkgo V1 for their own tests. You don't have to upgrade those test, you don't have to worry about having V1 in your go.mod...
(d) ...you just need to worry about packages that you import that themselves import Ginkgo V1. Which I think will just be those few test helper packages that are out there.
(e) lastly, to fix this, you just need to update those test helper packages. No need to migrate all of ifrit or all of lager. Just add a new ginkgomon_v2 and lagertest_v2 and you're unblocked.

[onsi]

ah, sorry, I think I misunderstood you:

so your saying if its in the project and not in the *_test package it will cause problems.

you mean "if it's in the dependency and not in the *_test package it will cause problems". Yes that's what I mean. If it's in the dependency (specifically in a package you import into your test suite) it will cause problems.

[thediveo]

Maybe as a note of another potential source of 💥: I had an import for github.com/onsi/ginkgo/config in my own package tests which I missed to update, or rather, to migrate. This triggered a CLI flag double registration panic as well as the indirect dependency.

[onsi]

Thanks @thediveo - i'm adding an FAQ to the migration guide to capture some of this subtlety.

@KevinJCross ive opened tedsuo/ifrit#35 and will work on it this week.

[onsi]

I've updated the migration docs to capture this issue and give folks a bit more guidance. Adding that link here so others can find it and in case you'll find it helpful @KevinJCross

Gonna work on Ifrit in the next few days so should have a PR for Ted to pull in soon.