Raise default Eventually() timeout?

[vito]

Hi there!

So, Eventually() is pretty cool, but its default timeout is 1 second. When a machine is busy running other builds, this can fail for no good reason, because Mr. Computer can only do so much at once. It's been a constant battle over the past couple of years, chasing down naive Eventually() calls that don't set a higher timeout. Every time I see a timeout failure in CI my default assumption is that the CI worker was under more load at the time, not that the test failure is genuine.

I've gotten into the habit of bringing up Eventually() in at least one retro every few months (generally as engineers rotate through), and basically saying that if you use it without setting a pretty high timeout value, your test will eventually fail in CI for no good reason. I think it's almost always preferable to just not have a timeout, let the test hang if it's actually broken, and force engineers to fix it. This feels better than learning to ignore certain failures because they're "flakes" and retriggering builds all the time.

This time we set up an action item to actually bring it up, and here I am! I don't know what a reasonable timeout would be, but I at least wanted to kick off the discussion here and see if anyone else has experienced the same.

[xoebus]

I agree.

Having the suite hang until the global test timeout has the advantage that you can dump the stacks of the test process to see where the holdup is. I'm largely in favor of any practice which moves us further from blindly re-triggering builds.

[onsi]

It is possible to change the default globally on a per-suite basis. Yet another thing to remember to do per suite to be sure. But are there reasons that isn’t a good enough solution? With that said am open to changing the baked in default. The original value was somewhat arbitrary.

…

On Oct 24, 2018, at 10:30 AM, Christopher Brown ***@***.***> wrote: I agree. I don't think we're at the "Eventually Considered Harmful" stage, but having the suite hang until the global test timeout has the advantage that you can dump the stacks of the test process to see where the holdup is. I'm in favor of any practice which moves us further from blindly re-triggering builds. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

[vito]

@onsi True, and I've started doing that when I don't have time to fix individual tests. It's still something to fix each time engineers bootstrap new packages, so I don't think it resolves the root of the issue.

I'm with @xoebus in spirit, in that having no timeout would be preferable for our project, but that's almost definitely too big of a change for everyone else using Ginkgo. What about having a flag which applies across all suites? At least then I'd only have to remember to add the flag to our CI scripts once per script. Something like --defaultEventuallyTimeout=1h?

FWIW I also agree that bumping the default would help most Ginkgo users with pretty minimal drawbacks, perhaps in addition to the flag.

[williammartin]

Having been through this so many times on Garden, I've also ranted a fair few times on timeouts and the second order effects. There's so much value in Eventually and Consistently, but man does it hurt later when you don't know if something was slow or hanging forever.

[mangelajo]

👍

[onsi]

I'm working through the backlog of old Ginkgo issues - apologies as this issue is probably stale now. I'm going to close it for now and open an issue on Gomega - I think having Gomega honor an environment variable would help keep Ginkgo and Gomega decoupled and provide a way to solve this in one place for a project.