Better testing timeouts for less painful development

[botandrose]

Background

• Many tests assert that certain browser events occurred using a helper named nextEventNamed. To deal with asynchronicity, this helper waits until the specified event occurs. If the event never occurs, it loops forever until the the default Playwright test timeout is reached.
• The default Playwright test timeout is 30 seconds.
• Playwright is configured to retry failing tests for a max of 3 total runs.
• Playwright is configured to run tests on both Chromium and Firefox.

Pain

When developing Turbo and running tests frequently, if even one of these event assertion fails, we're looking at a minimum of 3 minutes before the test run completes! And that's the best-case scenario of running only one individual test... often we're running many, and if one test fails, often other tests are failing too.

Proposed resolution

Configure Playwright's global default to 10 seconds. This still seems high to me, to be honest, but I figured it was best to be conservative. If we're okay with lowering it further to perhaps 5 or even 3 seconds, I'm happy to do this. Note that if one or two tests truly need a longer timeout, they can be set individually with a simple test.setTimeout(ms) call at the beginning of the test.

Additional notes

While I was in the Playwright config file, I lowered some other seemingly excessive timeouts, also to 10 seconds. I figure if Chrome or Firefox doesn't start up within 10 seconds, its probably not going to start in 60 seconds either. The tiny node fixture testing server had a 2 minute startup timeout!

[botandrose]

As an alternative, or even as a follow-up, I think it makes sense to replace the infinite loop in nextEventNamed with a default timeout, perhaps 2 seconds to match Capybara's default. We could make this configurable via an optional argument, too.

[botandrose]

Well, 10 seconds was still too painful (still have to wait a full minute for failing test results), so I just went ahead and did what I described above with adding 2 second timeouts to the individual event helpers, and I've pushed it here, too. Renaming to Better testing timeouts for less painful development. I think this is a good combo of approaches. Sane global timeouts, and finer-grained timeouts for assertions.

What do you think?

[seanpdoyle]

Changing the global timeout is definitely an improvement. I'm skeptical that making event reading timeouts configurable is worthwhile. Adding a fourth positional argument to the next-prefixed methods awkward, and I think it's less likely to require one-off re-configurations.

Instead, what do you think about adding a 2000ms timeout directly to the readEventLogs and readBodyMutationLogs functions? If that works, maybe it'd be worthwhile to source the 2000ms value from a global configuration.

[botandrose]

@seanpdoyle Agreed on all counts.

The wrench in the gears is that this specific test is doing a weird waiting thing: "waits for some time, but renders if CSS takes too much to load". So it needs a specifically longer timeout just for this one event reading, so it looks to me like we have to make it configurable at this level.

Honestly, I think this whole nextEventNamed noNextEventNamed system needs to be reworked into explicit assertions, e.g. assert.eventNamed. The more I look at it, the more problems I see with it. The noNextEventNamed doesn't wait at all, so I'm seeing false positives when I KNOW that the event is later thrown. This is probably okay if you are careful to only call it after a positive event assertion, but it seems like a footgun at the very least.