From cb46517447a2eec0ec62fa79ea71ace8e63d3a00 Mon Sep 17 00:00:00 2001 From: TheDiveO <6920158+thediveo@users.noreply.github.com> Date: Fri, 26 Aug 2022 14:00:13 +0200 Subject: [PATCH] fix false positive gleaks when using ginkgo -p (#577) --- docs/index.md | 26 +++++++++++++++++----- gleak/ginkgo_parallel_client.go | 25 ++++++++++++++++++++++ gleak/gleak_suite_test.go | 7 ++++++ gleak/have_leaked_matcher.go | 38 +++++++++++++++++---------------- 4 files changed, 73 insertions(+), 23 deletions(-) create mode 100644 gleak/ginkgo_parallel_client.go diff --git a/docs/index.md b/docs/index.md index 31cd3df37..91dc3c75a 100644 --- a/docs/index.md +++ b/docs/index.md @@ -3056,11 +3056,27 @@ correctly terminate without triggering false positives. Please refer to the interval (which defaults to 1s) and the polling interval (which defaults to 10ms). -This form of goroutine leak test can cause false positives in situations where a -test suite or dependency module uses additional goroutines. This simple form -only looks at all goroutines _after_ a test has run and filters out all -_well-known_ "non-leaky" goroutines, such as goroutines from Go's runtime and -the testing frameworks (such as Go's own testing package and Gomega). +Please note that this simplest form of goroutine leak test can cause false +positives in situations where a test suite or dependency module uses additional +goroutines. This simple form only looks at all goroutines _after_ a test has run +and filters out all _well-known_ "non-leaky" goroutines, such as goroutines from +Go's runtime and the testing frameworks (such as Go's own testing package and +Gomega). + +### Ginkgo -p + +In case you intend to run multiple package tests in parallel using `ginkgo -p +...`, you'll need to update any existing `BeforeSuite` or add new `BeforeSuite`s +in each of your packages. Calling `gleak.IgnoreGinkgoParallelClient` at the +beginning of `BeforeSuite` ensures that `gleak` updates its internal ignore list +to ignore a background goroutine related to the communication between Ginkgo and +the parallel packages under test. + +```go +var _ = BeforeSuite(func() { + IgnoreGinkgoParallelClient() +}) +``` ### Using Goroutine Snapshots in Leak Testing diff --git a/gleak/ginkgo_parallel_client.go b/gleak/ginkgo_parallel_client.go new file mode 100644 index 000000000..9c02f4041 --- /dev/null +++ b/gleak/ginkgo_parallel_client.go @@ -0,0 +1,25 @@ +package gleak + +import "os" + +// IgnoreGinkgoParallelClient must be called in a BeforeSuite whenever a test +// suite is run in parallel with other test suites using "ginkgo -p". Calling +// IgnoreGinkgoParallelClient checks for a Ginkgo-related background go routine +// and then updates gleak's internal ignore list to specifically ignore this +// background go routine by its ("random") ID. +func IgnoreGinkgoParallelClient() { + ignoreCreator := "net/rpc.NewClientWithCodec" + if os.Getenv("GINKGO_PARALLEL_PROTOCOL") == "HTTP" { + ignoreCreator = "net/http.(*Transport).dialConn" + } + ignores := []Goroutine{} + for _, g := range Goroutines() { + if g.CreatorFunction == ignoreCreator { + ignores = append(ignores, g) + } + } + if len(ignores) == 0 { + return + } + standardFilters = append(standardFilters, IgnoringGoroutines(ignores)) +} diff --git a/gleak/gleak_suite_test.go b/gleak/gleak_suite_test.go index 30c828477..5da78ce6b 100644 --- a/gleak/gleak_suite_test.go +++ b/gleak/gleak_suite_test.go @@ -7,6 +7,13 @@ import ( . "github.com/onsi/gomega" ) +// In case this suite is run in parallel with other test suites using "ginkgo +// -p", then there is a Ginkgo-related background go routine that we need to +// ignore in all tests and that can be identified only by its random ID. +var _ = BeforeSuite(func() { + IgnoreGinkgoParallelClient() +}) + func TestGleak(t *testing.T) { RegisterFailHandler(Fail) RunSpecs(t, "Gleak Suite") diff --git a/gleak/have_leaked_matcher.go b/gleak/have_leaked_matcher.go index 9fb1c44c6..c07f00ec8 100644 --- a/gleak/have_leaked_matcher.go +++ b/gleak/have_leaked_matcher.go @@ -21,11 +21,11 @@ import ( // // That is, with ReportFilenameWithPath==false: // -// foo/bar.go:123 +// foo/bar.go:123 // // Or with ReportFilenameWithPath==true: // -// /home/goworld/coolprojects/mymodule/foo/bar.go:123 +// /home/goworld/coolprojects/mymodule/foo/bar.go:123 var ReportFilenameWithPath = false // standardFilters specifies the always automatically included no-leak goroutine @@ -48,6 +48,8 @@ var standardFilters = []types.GomegaMatcher{ gomega.And(IgnoringTopFunction("runtime.goexit1"), IgnoringCreator("github.com/onsi/ginkgo/v2/internal.(*Suite).runNode")), IgnoringTopFunction("github.com/onsi/ginkgo/v2/internal/interrupt_handler.(*InterruptHandler).registerForInterrupts..."), IgnoringTopFunction("github.com/onsi/ginkgo/internal/specrunner.(*SpecRunner).registerForInterrupts"), + IgnoringCreator("github.com/onsi/ginkgo/v2/internal.(*genericOutputInterceptor).ResumeIntercepting"), + IgnoringCreator("github.com/onsi/ginkgo/v2/internal.(*genericOutputInterceptor).ResumeIntercepting..."), // goroutines of Go's own testing package for its own workings... IgnoringTopFunction("testing.RunTests [chan receive]"), @@ -83,32 +85,32 @@ var standardFilters = []types.GomegaMatcher{ // Eventually's default timeout and polling interval settings, but these can be // overridden as usual: // -// // Remember to use "Goroutines" and not "Goroutines()" with Eventually()! -// Eventually(Goroutines).ShouldNot(HaveLeaked()) -// Eventually(Goroutines).WithTimeout(5 * time.Second).ShouldNot(HaveLeaked()) +// // Remember to use "Goroutines" and not "Goroutines()" with Eventually()! +// Eventually(Goroutines).ShouldNot(HaveLeaked()) +// Eventually(Goroutines).WithTimeout(5 * time.Second).ShouldNot(HaveLeaked()) // // In its simplest form, an expected non-leaky goroutine can be identified by // passing the (fully qualified) name (in form of a string) of the topmost // function in the backtrace. For instance: // -// Eventually(Goroutines).ShouldNot(HaveLeaked("foo.bar")) +// Eventually(Goroutines).ShouldNot(HaveLeaked("foo.bar")) // // This is the shorthand equivalent to this explicit form: // -// Eventually(Goroutines).ShouldNot(HaveLeaked(IgnoringTopFunction("foo.bar"))) +// Eventually(Goroutines).ShouldNot(HaveLeaked(IgnoringTopFunction("foo.bar"))) // // HaveLeak also accepts passing a slice of Goroutine objects to be considered // non-leaky goroutines. // -// snapshot := Goroutines() -// DoSomething() -// Eventually(Goroutines).ShouldNot(HaveLeaked(snapshot)) +// snapshot := Goroutines() +// DoSomething() +// Eventually(Goroutines).ShouldNot(HaveLeaked(snapshot)) // // Again, this is shorthand for the following explicit form: // -// snapshot := Goroutines() -// DoSomething() -// Eventually(Goroutines).ShouldNot(HaveLeaked(IgnoringGoroutines(snapshot))) +// snapshot := Goroutines() +// DoSomething() +// Eventually(Goroutines).ShouldNot(HaveLeaked(IgnoringGoroutines(snapshot))) // // Finally, HaveLeaked accepts any GomegaMatcher and will repeatedly pass it a // Goroutine object: if the matcher succeeds, the Goroutine object in question @@ -116,11 +118,11 @@ var standardFilters = []types.GomegaMatcher{ // built-in Goroutine filter matchers should hopefully cover most situations, // any suitable GomegaMatcher can be used for tricky leaky Goroutine filtering. // -// IgnoringTopFunction("foo.bar") -// IgnoringTopFunction("foo.bar...") -// IgnoringTopFunction("foo.bar [chan receive]") -// IgnoringGoroutines(expectedGoroutines) -// IgnoringInBacktrace("foo.bar.baz") +// IgnoringTopFunction("foo.bar") +// IgnoringTopFunction("foo.bar...") +// IgnoringTopFunction("foo.bar [chan receive]") +// IgnoringGoroutines(expectedGoroutines) +// IgnoringInBacktrace("foo.bar.baz") func HaveLeaked(ignoring ...interface{}) types.GomegaMatcher { m := &HaveLeakedMatcher{filters: standardFilters} for _, ign := range ignoring {