From 15d4bdc9f8d4d43e1739e0862604d643e03ce2bf Mon Sep 17 00:00:00 2001 From: Paul Holzinger <45212748+Luap99@users.noreply.github.com> Date: Wed, 3 May 2023 16:50:03 +0200 Subject: [PATCH] fix hang with ginkgo -p (#1192) * integration: build interceptor binary automatically Trying to run this locally the interceptor binary did not exists. Lets just build this in SynchronizedBeforeSuite() automatically so users do not have to figure this out. Signed-off-by: Paul Holzinger * integration: make interceptor test parallel safe Using the same file name for all test cause conflicts when run in parallel, this causes the tests to fail for me. To fix this use a filename suffix with GinkgoParallelProcess() which prevents conflicts in the file name. Signed-off-by: Paul Holzinger * fix hang with ginkgo -p When you run any child process that stay around longer than the test suite ginkgo currently hangs. This is because the dup stdout/err fds are leaked into the child thus read() will block on it as there is at least one process still having the write pipe open. From ginkgos POV it looks like it is done, you see the ginkgo result output printed but then it just hangs and doe snot exit because of it. To fix it we set FD_CLOEXEC on the dup-ed fds, this prevents them from ever being leaking into other processes that could outlive the suite. There is a dup3() call the could be uses to set the CLOEXEC option directly but this seem linux only and thus is less portable. The fcntl call should be good enough here, we do not need to be worried about the race conditions described in the man page. Ideally we should do some error handling in that function for both the fcntl calls and the existing dup() above, however this seems like a rather big change. I am not so sure about how to test it properly, I added a test which just executes `ginkgo run -p` and a test which only starts `sleep 60`. Ginkgo then should exit right way and keep this process running. Then I just make sure the gingo exits in under 15 seconds. As long as it is below 60s it should fulfil the purpose. Fixes #1191 Signed-off-by: Paul Holzinger * Rearrange new interceptor hang tests and confirm they cover the issue in question --------- Signed-off-by: Paul Holzinger Co-authored-by: Onsi Fakhouri --- TODO | 60 +++++++++++++++++++ .../interceptor_fixture_suite_test.go | 25 ++++++++ integration/output_interceptor_test.go | 18 ++++++ internal/output_interceptor_unix.go | 11 ++++ 4 files changed, 114 insertions(+) create mode 100644 TODO create mode 100644 integration/_fixtures/interceptor_sleep_fixture/interceptor_fixture_suite_test.go diff --git a/TODO b/TODO new file mode 100644 index 000000000..8a44c4b69 --- /dev/null +++ b/TODO @@ -0,0 +1,60 @@ +# Backlog +[] @G fix hang with ginkgo -p + https://github.com/onsi/ginkgo/issues/1192 +[] @B biloba needs to support "McDonald's" +[] @G Document order of execution for setup nodes on the same level + https://github.com/onsi/ginkgo/issues/1172 +[] @G fail fast may cause Serial spec or cleanup Node interrupted + https://github.com/onsi/ginkgo/issues/1178 + +# Needs-Prioritization +[] @G Pull JUnit config out of code and into flags/config files + https://github.com/onsi/ginkgo/issues/1115 +[] @G -exec support for `ginkgo run` + https://github.com/onsi/ginkgo/issues/869 +[] @G Add `indent` to gcustom +[] @G HaveField should not panic in FailureMessage if the field is non-existant. It should return a meaningful failure message. +[] @G allow NodeTimeout and GracePeriod to live on containers and propagate down +[] @G Clean up ReportEntry decoding: + func (entry ReportEntry) DecodeValue(p interface{}) error { + // if raw is assignable to p, assign + // else - parse AsJSON + } +[] @B Biloba lets you get all innerHTML and emit it on failure +[] @B equivalent of puppeteer text selector? +[] @B how do we invoke async functions? what does await look like for those? maybe time to actually read? goal: remove the separate muhasibPostLogin function. +[] @B https://github.com/onsi/biloba/issues/2 +[] @B right now polling an element fails if the browser if ollowing redirects. so i'm using Eventually(b.Location) - instead of just Eventually("#el").Should(b.Exist()). I think we need a more robust way to ensure biloba. +[] @B biloba support for gettign "SelectedOption" instead of just "Value" for select inputs (e.g. b.OptionByName(...) instead of value?) +[] @B ginkgo interrupt should not show the whole stacktrace. it's just too much! +[] @B add cookie support + chromedp.Run(b.Context, chromedp.ActionFunc(func(ctx context.Context) error { + return storage.ClearDataForOrigin("*", "all").Do(ctx) + })) + + chromedp.Run(b.Context, chromedp.ActionFunc(func(ctx context.Context) error { + expr := cdp.TimeSinceEpoch(time.Date(2091, 28, 3, 1, 40, 45, 0, time.UTC)) + return storage.SetCookies([]*network.CookieParam{{ + Name: "rallly-session", + Value: "Fe26.2*1*66a5cae1dd8728fc7be37a1a3c485557606e526b16b472329be78168ad4d48c2*Yb_O9pN2K3APF6LXt9S3zg*IZQ_c5aukJzt-AIW__lL19igVhpFMGH9cK0PyFenF-2ti94BgBsLDf325DB2rsKE*3825906104968*a06f3cfe6ef65db1a30b5177cb767c914ca38c8fc3e2456de89d5bea5641611e*6HNCfDeEzgfeQO88IRJ8TfdG5IDzDQtt6WaoGAg5i98~2", Expires: &expr, + Domain: "rallly.co", + Path: "/", + HTTPOnly: true, + Secure: true, + SameSite: network.CookieSameSiteLax, + }}).WithBrowserContextID(b.BrowserContextID()).Do(ctx) + })) +[] @Ω Gomega should have an error returning mode, then tell pohly +[] @Ω Gomega submatcher description interface and bare-element interface (the former for any sort of matcher that takes a submatcher; the latter specifically for matchers like Consistently etc. that would replace equalMatchersToElements. + + +[] @B JSSelector (is a function that returns one or many nodes) +[] @B ScrollTo etc. +[] @B support for esbuild? or something? consider the auth_login_scripts tests - what might make them better? + +# Long-term Backlog +- VSCode Extension +- Ginkgo WebView +- Suite configuration +- Suite parallelization + - With the special subcase of supporting shared singleton resources diff --git a/integration/_fixtures/interceptor_sleep_fixture/interceptor_fixture_suite_test.go b/integration/_fixtures/interceptor_sleep_fixture/interceptor_fixture_suite_test.go new file mode 100644 index 000000000..61704ce28 --- /dev/null +++ b/integration/_fixtures/interceptor_sleep_fixture/interceptor_fixture_suite_test.go @@ -0,0 +1,25 @@ +package main_test + +import ( + "fmt" + "os" + "os/exec" + "testing" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +func TestInterceptorSleepFixture(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "TestInterceptorSleepFixture Suite") +} + +var _ = Describe("Ensuring that ginkgo -p does not hang when output is intercepted", func() { + It("ginkgo -p should not hang on this spec", func() { + fmt.Fprintln(os.Stdout, "Some STDOUT output") + fmt.Fprintln(os.Stderr, "Some STDERR output") + cmd := exec.Command("sleep", "60") + Ω(cmd.Start()).Should(Succeed()) + }) +}) diff --git a/integration/output_interceptor_test.go b/integration/output_interceptor_test.go index 71175efc5..ca93e92e6 100644 --- a/integration/output_interceptor_test.go +++ b/integration/output_interceptor_test.go @@ -2,9 +2,11 @@ package integration_test import ( "os/exec" + "time" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "github.com/onsi/gomega/gbytes" "github.com/onsi/gomega/gexec" ) @@ -47,4 +49,20 @@ var _ = Describe("OutputInterceptor", func() { Ω(report.SpecReports[0].CapturedStdOutErr).Should(Equal("CAPTURED OUTPUT A\nCAPTURED OUTPUT B\n")) }) }) + + Context("ensuring Ginkgo does not hang when a child process does not exit: https://github.com/onsi/ginkgo/issues/1191", func() { + BeforeEach(func() { + fm.MountFixture("interceptor_sleep") + }) + + It("exits without hanging", func() { + sess := startGinkgo(fm.PathTo("interceptor_sleep"), "--no-color", "--procs=2") + Eventually(sess).WithTimeout(time.Second * 15).Should(gexec.Exit(0)) + + Ω(sess).Should(gbytes.Say("Captured StdOut/StdErr Output >>")) + Ω(sess).Should(gbytes.Say("Some STDOUT output")) + Ω(sess).Should(gbytes.Say("Some STDERR output")) + Ω(sess).Should(gbytes.Say("<< Captured StdOut/StdErr Output")) + }) + }) }) diff --git a/internal/output_interceptor_unix.go b/internal/output_interceptor_unix.go index f5ae15b8b..8a237f446 100644 --- a/internal/output_interceptor_unix.go +++ b/internal/output_interceptor_unix.go @@ -26,6 +26,17 @@ func (impl *dupSyscallOutputInterceptorImpl) CreateStdoutStderrClones() (*os.Fil stdoutCloneFD, _ := unix.Dup(1) stderrCloneFD, _ := unix.Dup(2) + // Important, set the fds to FD_CLOEXEC to prevent them leaking into childs + // https://github.com/onsi/ginkgo/issues/1191 + flags, err := unix.FcntlInt(uintptr(stdoutCloneFD), unix.F_GETFD, 0) + if err == nil { + unix.FcntlInt(uintptr(stdoutCloneFD), unix.F_SETFD, flags|unix.FD_CLOEXEC) + } + flags, err = unix.FcntlInt(uintptr(stderrCloneFD), unix.F_GETFD, 0) + if err == nil { + unix.FcntlInt(uintptr(stderrCloneFD), unix.F_SETFD, flags|unix.FD_CLOEXEC) + } + // And then wrap the clone file descriptors in files. // One benefit of this (that we don't use yet) is that we can actually write // to these files to emit output to the console even though we're intercepting output