Make lighthouse reports concurrent

[calebeby]

The number of lighthouse instances defaults to the number of CPU cores minus 1. You can override this with the --lighthouse-concurrency flag.

When lighthouse errors it gets displayed like this:

When there are too many pending pages to display them all, they are cut off at the bottom. The terminal height is retrieved dynamically and it correctly handles resizes:

General comments about this PR:
I am very pleased with how the CLI output turned out. I am a little less pleased with the code that makes that CLI output appear. Would it be worth separating some of the CLI output logic into a separate module? Open to suggestions/refactorings.

For review:

• Test it with a bunch of URLs, make sure it works as expected
• Test the --lighthouse-concurrency flag
• I didn't add any tests for this. This seems like it is really hard to test. I'm open to suggestions for how to test it.

It is probably worth comparing the generated reports if lighthouse-concurrency is set to a high number vs if it is set to 1, to see if any of the performance metrics in the reports change.

Known problems that I don't plan to solve:

• If the URLs are long enough to wrap, there is some weirdness with the output, especially if you scroll up. It is still usable though. I could solve this, or we could just say that it is not a big enough problem to solve.
• If you resize the terminal to make it vertically smaller while it is running, there is also some weirdness with the output if you scroll up. It is still usable.

[changeset-bot]

🦋 Changeset detected

Latest commit: ac6ceb8

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
lighthouse-parade	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[emersonthis]

I checked out this branch and ran it on a couple sites. It seems to work great. I didn't play with the concurrency flag much, but with the default n-1 cores in action it definitely seemed to run faster. I could also hear the cooling fan buzzing while it was chugging along.

If the URLs are long enough to wrap, there is some weirdness with the output, especially if you scroll up. It is still usable though. I could solve this, or we could just say that it is not a big enough problem to solve.

Definitely doesn't feel like a deal-breaker. Let's spin it off into a separate issue.

It is probably worth comparing the generated reports if lighthouse-concurrency is set to a high number vs if it is set to 1, to see if any of the performance metrics in the reports change.

Agreed. Please do conduct this test and let me know what you discover. I'd also like to know the difference in overall time. dfwfreeways.com is probably a good test for this. As you do this, please also compare some results with the report generated by the Chrome dev tools. This latter bit isn't specific to this PR, but seems like a good time to get some confidence about this... in the past, I've noticed what seemed like inconsistencies, but I haven't had time to investigate properly.

Would it be worth separating some of the CLI output logic into a separate module? Open to suggestions/refactorings.

I do agree with this idea. But I don't have strong feelings about whether it has to happen before this gets merged, or shortly after in a follow-up issue. I'll leave that decision to you.

I am a little wary about adding a lot of new code with no tests. But I also recognize that most of the new logic is the terminal output stuff. What about some simple tests that check that the expected events fire when they should? And as for concurrency, I wonder if instead of directly asserting what lighthouse-cli is doing, maybe we could infer the concurrency by mocking/spying downstream functions? Something like that? I don't want us to waste time writing unhelpful tests, but I do think it's worth taking an extra moment to think about how we might test this, and if we come to the conclusion that we just can't, then maybe that's a signal that we need to revisit our architecture.

[calebeby]

@emersonthis Last week I did a couple comparisons to see how different the results are with concurrency vs. without: https://docs.google.com/spreadsheets/d/17pMR3DbmO6H4dVa411dG2rpQqgJoJAwFNZXF23nGTYo/edit#gid=909074128

IMO these differences are reasonable, since multiple consecutive runs of lighthouse are likely to not yield quite the same results anyways. Is there anything that stands out to you as a major difference in the results between concurrent and serial?

[calebeby]

And in terms of comparing lighthouse scores generated by lighthouse-parade and by running reports in Chrome directly, I tested a couple pages on http://www.dfwfreeways.com/ and found that lighthouse-parade tends to give a higher performance score (by ~6-8 pts) than lighthouse in chrome devtools does.

[emersonthis]

Is there anything that stands out to you as a major difference in the results between concurrent and serial?

Nope. Those results look pleasantly expected.

lighthouse-parade tends to give a higher performance score (by ~6-8 pts) than lighthouse in chrome devtools does.

Hmm. I'm not super worried about it, but it's curious. One theory: maybe the version of Lighthouse bundled into Chrome differs from whatever LP is running. Is this an easy thing to check? If not, please create a new issue to look into this later. It shouldn't delay this merge, but I think it's worth some effort to ensure the fidelity of our results.

Before we merge this, what are your thoughts on the testing question(s)?

[calebeby]

@emersonthis Looks like the latest chrome version uses lighthouse 6.2.0 and lighthouse-parade uses lighthouse 6.4.0

[calebeby]

I have been spinning my wheels about how to test this, but I haven't gotten anywhere.

This PR modifies the outermost logic (CLI + logging) and the innermost logic (lighthouse wrapper), both of which are being stubbed out in testing. What logic specifically is being added in this PR that should be tested?

What about some simple tests that check that the expected events fire when they should?

I haven't yet stubbed out the aggregateReports logic for testing. In a follow up PR can I make that change, and then add tests to make sure that events from the lighthouse wrapper are getting forwarded up to the scan-task?

[emersonthis]

We just discussed spinning off two issues:

1. Investigation of the score discrepancy
2. Refactor to support testing
   ...in the mean time this is good to go!