tests: use test-specific about:blank timer

[patrickhulce]

recently had to work on gather-runner and was going nuts on 7s test times...

cuts overall unit test time by about half

Before

After

[brendankenny]

I've thought about this as well, but I also worry about the complexity of ever-expanding injected secret variables just for tests when we don't inject the value for the regular run (or worse, inject related but slightly different values).

Might be prejudiced here, though, because I also hate our poorly documented and inconsistent options object(s) :)

One interesting thing here is the old comment We do not `waitForLoad` on about:blank since a page load event is never fired on it. Technically it can be waitForLoad and just have the 300ms be the timeout and goToURL() won't wait around for the page load event. The main issue is the log.warn() in that case.

I wonder if there's some rearranging we can do here (maybe with #1672) to make this work more cleanly

[brendankenny]

Three timing values here: waiting for load timeout, waiting on about:blank timeout, and the time to wait after load.

Our current situation does make some sense:

• exposing the page load timeout as a top level option is also good
• the about:blank one can be somewhat hidden, as it's really only useful when trying to reduce test time
• Sam had a clear need to expose the time to wait after load for his own testing in Enable passing in a custom pauseAfterLoad option. #697, so that makes sense as a Lighthouse-as-a-module option, though not necessarily exposed on the CLI

So I guess my feelings here are mostly about being able to easily read and understand the control flow. I wonder if we can route both regular loadPage and loadBlank through driver.goToURL to unify this stuff

[patrickhulce]

I also worry about the complexity of ever-expanding injected secret variables just for tests

In the past to combat this I've hardened the interface of options such that they're clearly understood where they come from/how they flow or collected values into an internal config file that gets require'd and used something like proxyquire to inject whatever implementation was needed for testing.

I wonder if there's some rearranging we can do here (maybe with #1672) to make this work more cleanly

IMO loadBlank is not really worth refactoring for the simple reason that its logic is one line with exactly 1 control flow path and wiring up everything needed for _waitForFullyLoaded just to exercise the timeout is wasted effort. They both already use driver.gotoURL btw :)

Proposal: I move the value from options to a static class variable that has a setter just for testing?

[paulirish]

One alternative to passing this as a setting everywhere is to see if process.env.npm_lifecycle_event or process.env.CI are set. If so, we adjust that timeout.

[patrickhulce]

will not be needed soon enough when this becomes a hardened option, closing