tests(smoke): check runner result is sensible before using

[connorjclark]

Once #14320 landed I then saw another error further down the report assertion function. So let's fix the root cause: we shouldn't just trust that lighthouseRunner returns a valid LH.Result. At least for the CLI runner, it's possible an error occurred (ex: config resolution failed) resulting in no LHR at all (just an empty object being returned. or maybe it was undefined).

[brendankenny]

I believe the intention was lighthouseRunner would either return a Promise<{lhr: LHResult, artifacts: Artifacts, log: string}> or throw an error. In the error case the catch will catch it and continue to the end of the loop, never running the assertions (stderr should be logged and the whole test counted as a failure).

Is the problem that the non-CLI versions are returning something in the error cases instead? The CLI has this check basically built in because it loads the LHR and artifacts from disk, so will throw if they're not around:

lighthouse/cli/test/smokehouse/lighthouse-runners/cli.js

Lines 101 to 110 in bb2bed6

	try {
	await fs.access(outputPath);
	} catch (e) {
	throw new ChildProcessError(`Lighthouse run failed to produce a report and exited with ${exitCode}.`, // eslint-disable-line max-len
	localConsole.getLog());
	}
	/** @type {LH.Result} */
	const lhr = JSON.parse(await fs.readFile(outputPath, 'utf8'));
	const artifacts = assetSaver.loadArtifacts(artifactsDirectory);

Can we do the same in bundle/devtools?

We could loosen the types for lighthouseRunner instead, but bundle already does a third of the check here, and devtools is writing possibly nonsense to disk here, so it seems like a benefit. In theory the check can grow to runner-specific checks like cli does as well (like checking that if there's a non-zero exit code there was also a lhr.runtimeError in the expectations), so e.g. devtools could log more specific errors due to build issues or pptr setup or whatever.

[paulirish]

mkay