tests: refactor smokehouse for extensibility #9843
Conversation
|
and as far as I can tell there's nothing here that would hinder movement to #8962, @patrickhulce :) |
|
|
||
| ## Test definitions | ||
|
|
||
| | Name | Type | Description | |
There was a problem hiding this comment.
gotta run prettier on these tables man, it's oh so good ;)
There was a problem hiding this comment.
gotta run prettier on these tables man, it's oh so good ;)
ha, didn't know it would format tables. I'll try it out to see what happens :)
There was a problem hiding this comment.
ping :)
well I tried it and it seemed to help on some tables and not others so...who knows :)
| | node +---------->+ smokehouse.js +-----+ | ||
| | | | | | | +--------------+ | ||
| +------------+ | +-------+-------+ | | | | ||
| | ^ +-->+ bundled+LH | |
There was a problem hiding this comment.
is bundledLhRunner the only other candidate for the -bin runner or are our other hopes for smokehouse/firehouse also going to be runners
There was a problem hiding this comment.
is
bundledLhRunnerthe only other candidate for the-binrunner or are our other hopes for smokehouse/firehouse also going to be runners
yes, the bundled/firehouse runner would be the only other runner I know of at this point, but we could conceivably have a dt vs lr bundle runner (or someday some kind of pptr runner), for instance.
|
@patrickhulce and @paulirish, do you want to take another look or are you 🍬 |
| * A class that maintains a concurrency pool to coordinate many jobs that should | ||
| * only be run `concurrencyLimit` at a time. | ||
| * API inspired by http://bluebirdjs.com/docs/api/promise.map.html, but | ||
| * independent callers of `concurrentMap()` share the same concurrency limit. |
There was a problem hiding this comment.
| * independent callers of `concurrentMap()` share the same concurrency limit. | |
| * independent callers of `pooledMap()` share the same concurrency limit. |
|
|
||
| ## Test definitions | ||
|
|
||
| | Name | Type | Description | |
| /** | ||
| * CLI entry point. | ||
| */ | ||
| async function cli() { |
There was a problem hiding this comment.
in LH's bin.js we have the equivalent function called begin()
that seems good
if not, i'd go with bin() before using cli()
|
|
||
| const assetSaver = require('../../../../lighthouse-core/lib/asset-saver.js'); | ||
| const LocalConsole = require('../local-console.js'); | ||
| const ChildProcessError = require('./child-process-error.js'); |
There was a problem hiding this comment.
move this into a smokehouse/lib/ folder?
| ### Frontends | ||
| - `frontends/smokehouse-bin.js` - runs smokehouse from the command line | ||
| - TODO: bundle-entry - simple entrypoint to smokehouse for bundling and running in a browser. | ||
| - `smokehouse.js` - smokehouse itself is runnable from node |
There was a problem hiding this comment.
having this here in this list but not in the folder threw me for a loop. also smokehouse.js listed here and in the next section. 😕
can you add a frontend/node.js that just module.exports = require('../smokehouse.js'); ?
that'd help my mental model of this thing.
| const cliLighthouseRunner = require('./lighthouse-runners/cli.js').runLighthouse; | ||
| const getAssertionReport = require('./report-assert.js'); | ||
| const LocalConsole = require('./local-console.js'); | ||
| const ConcurrentMapper = require('./concurrent-mapper.js'); |
There was a problem hiding this comment.
move concurrentmapper/localconsole/reportassert into a ./lib ?
| const smokeTests = require('./smoke-test-dfns.js'); | ||
| const {collateResults, report} = require('./smokehouse-report.js'); | ||
| const smokeTests = require('./test-definitions/core-tests.js'); | ||
| const report = require('./report-assert.js'); |
There was a problem hiding this comment.
(commenting here for no reason)
Firehouse should move into runners, right?
(i wouldnt mind changing the name now because i totally forget what its there for)
There was a problem hiding this comment.
--runner needs a concrete way to run Lighthouse, but Firehouse doesn't even provide that. You'd use it to create what would be a frontend, one capable of tweaking expectations.
AFAIU if a 'runner' could also tweak expectations like firehouse.js does, we can delete firehouse.js
There was a problem hiding this comment.
AFAIU if a 'runner' could also tweak expectations like firehouse.js does, we can delete firehouse.js
I'll follow up with my proposal to get all these needs addressed. Should be a small (hopefully negative) diff :)
There was a problem hiding this comment.
gotcha that makes sense. almost like a lib/expectation-filterer.js
or wait... EXPECTATION-SIEVE.JS
🔥
smokehouse has acquired a lot of layers over time, and it can be pretty difficult to reason about the exact control flow at times. This PR aims to rectify that, boiling it down to a simple structure (albeit with some fiddliness because nice logging is always fiddly in the end).
Basically all functionality and behavior is maintained except
yarn smoke || yarn smoke || yarn smoke. Try outyarn smoke -j 15 --retries 2:)bundle stuff (running a bundle, running as a bundle, aka firehouse) to come next from the end points established here, this just needed a cutoff point before it got too big.