IMO it doesn't make sense to automatically create a puppeteer page in timespan/snapshot mode since those modes assume the user wants to control what state the page is in. If we create the page here, there is no way for the user to access the page.

This makes sense, but pushing knowledge of hostname/port etc deeper into core starts mixing formerly separable concerns. Not fatal but it's nice to have browser connection abstracted outside of core. In classic mode that information was encapsulated in Connection, which didn't actually connect to anything until connect() was called, but that doesn't work for pptr.

What about instead pushing the connect call outward back to cli/? page can be required again here. And cli/run.js is already checking if it's gathering when deciding to launch Chrome or not:

so could just add an additional FR/legacy check there and create the Page at that point?

The root cause of #13863 is that we attempt to connect to the browser in audit mode when no browser exists but no browser is needed.

If we go with your suggestion, when will lighthouse() connect using puppeteer?

One other solution would be to push the-G/-A logic out to the CLI, but this would remove that capability from the node api.

That code is already checking shouldGather based on -G or -A for if it's going to launch Chrome. Hasty sketch would be something like

diff --git a/lighthouse-cli/run.js b/lighthouse-cli/run.js
index 9fee8a445..ea2bb0c44 100644
--- a/lighthouse-cli/run.js
+++ b/lighthouse-cli/run.js
@@ -13,6 +13,7 @@ import * as ChromeLauncher from 'chrome-launcher';
 import yargsParser from 'yargs-parser';
 import log from 'lighthouse-logger';
 import open from 'open';
+import puppeteer from 'puppeteer-core';
 
 import * as Printer from './printer.js';
 import lighthouse from '../lighthouse-core/index.js';
@@ -25,6 +26,9 @@ import URL from '../lighthouse-core/lib/url-shim.js';
 const _RUNTIME_ERROR_CODE = 1;
 const _PROTOCOL_TIMEOUT_EXIT_CODE = 67;
 
+const DEFAULT_HOSTNAME = '127.0.0.1';
+const DEFAULT_PORT = 9222;
+
 /**
  * exported for testing
  * @param {string|Array<string>} flags
@@ -213,6 +217,9 @@ async function runLighthouse(url, flags, config) {
   /** @type {ChromeLauncher.LaunchedChrome|undefined} */
   let launchedChrome;
 
+  /** @type {LH.Puppeteer.Page|undefined} */
+  let page;
+
   try {
     if (url && flags.auditMode && !flags.gatherMode) {
       log.warn('CLI', 'URL parameter is ignored if -A flag is used without -G flag');
@@ -225,6 +232,12 @@ async function runLighthouse(url, flags, config) {
       flags.port = launchedChrome.port;
     }
 
+    if (shouldGather && flags.fraggleRock) {
+      const {hostname = DEFAULT_HOSTNAME, port = DEFAULT_PORT} = flags;
+      const browser = await puppeteer.connect({browserURL: `http://${hostname}:${port}`});
+      page = await browser.newPage();
+    }
+
     if (flags.fraggleRock) {
       flags.channel = 'fraggle-rock-cli';
     } else {
@@ -232,7 +245,7 @@ async function runLighthouse(url, flags, config) {
     }
 
     const runnerResult = flags.fraggleRock ?
-       await lighthouse(url, flags, config) :
+       await lighthouse(url, flags, config, page) :
        await lighthouse.legacyNavigation(url, flags, config);
 
     // If in gatherMode only, there will be no runnerResult.

One other solution would be to push the-G/-A logic out to the CLI, but this would remove that capability from the node api.

The node api could just require the user to provide the Page object. I guess it would need to be optional after all for someone calling the non-UserFlow, standalone function navigation() and wanted -G/-A support (and just assert page in the gathering step rather than creating it).

Side question: should lighthouse() just be a re-export of navigation()? Seems weird to mostly but not quite do that.

That code is already checking shouldGather based on -G or -A for if it's going to launch Chrome. Hasty sketch would be something like

This code makes page a required parameter on lighthouse() because the page is created before lighthouse() is invoked in the CLI runner. Additionally, I'm not sure this will work if someone specifies gatherMode/auditMode in the config settings rather than the flags.

It seems like this is a by-product of us needing to maintain the gatherMode/auditMode logic in the config settings, so we need to handle this stuff after the config + flag overrides are resolved. If we only had -G/-A for the cli, then we could move this stuff out.

should lighthouse() just be a re-export of navigation()? Seems weird to mostly but not quite do that.

It could be, the reason I don't want to do this is because configContext is a bit messy and worthy of a refactor, but it's only exposed on our preview feature / user flow stuff. lighthouse() is just converting flags -> configContext, so we might want to go back to flags for user flow stuff.

This code makes page a required parameter on lighthouse() because the page is created before lighthouse() is invoked in the CLI runner.

It'll still be Page|undefined since there won't be a page (or a launched Chrome) in the falsy gatherMode, truthy auditMode case.

Additionally, I'm not sure this will work if someone specifies gatherMode/auditMode in the config settings rather than the flags.

Ugh, yeah, I see your point. In classic mode if auditMode is set in config settings, Chrome will be uselessly launched but ChromeProtocol won't ever be connected so it won't really matter. Not pretty but it's fine, and this replicates that behavior.

Since the config is initialized in navigationGather there's no way around it right now, but like configContext, we should take a shot at getting an interface we like for 10.0.

This makes sense, but pushing knowledge of hostname/port etc deeper into core starts mixing formerly separable concerns. Not fatal but it's nice to have browser connection abstracted outside of core. In classic mode that information was encapsulated in Connection, which didn't actually connect to anything until connect() was called, but that doesn't work for pptr.

What about instead pushing the connect call outward back to cli/? page can be required again here. And cli/run.js is already checking if it's gathering when deciding to launch Chrome or not:

so could just add an additional FR/legacy check there and create the Page at that point?

It seems like this should be unrelated to not connecting when the browser wasn't launched. Why is this needed? It seems like a big config filtering change.

This issue slipped by because I didn't duplicate the tests in index-test.js for the new fraggle rock runner. One of those tests runs lighthouse in audit mode with a config that has no categories, and asserts that the audits are still run.

It isn't related to the browser connection bit, but it does represent where legacy mode and FR are not doing the same thing. The test was related to audit mode, so I put it in this PR. Looking back I don't think the issue is specific to audit mode so yeah this probably makes sense in a new PR.

codecov is alarmed by the test coverage. Does #13867 make up for it?

codecov is alarmed by the test coverage. Does #13867 make up for it?

It should, but I don't trust codecov. It seems to flag lines of code that I guarantee are tested at some point (e.g. the line at the start of navigationGather)