-
Notifications
You must be signed in to change notification settings - Fork 9.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
build(smokehouse): bundle smoke test runner #9873
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a fear that all this firehouse stuff is going to be touchable exclusively by googlers :)
maybe we could get some fileoverviews so even if we're not able to make changes we can still understand why it shouldn't be deleted :)
agreed, and that makes the testing system more fragile and harder to change in the future. Doing some decoupling to make it so non-googlers don't have to care about the tests they can't see was part of the motivation for #9843, and I think we should consider making decisions on the architecture proposed there as a blocker for this. |
The first post was updated with the relevant doc (@patrickhulce, nothing has changed since you last reviewed the doc) |
I'm not sure I'm the best reviewer for this PR @connorjclark since I have no insight into the companion consumption code. Any googlers want to swap PRs for this one @exterkamp @paulirish ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of the smokehouse-bin generic changes seem fine to me, but I can't really speak to the consumption of the reworked firehouse frontend which seems to be the important part of this change
* @param {Smokehouse.FirehouseOptions} options | ||
*/ | ||
async function runFirehouse(options) { | ||
const {smokehouse, urlFilterRegex, skip, modify} = options; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these all get set by the person consuming firehouse, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup
build/build-firehouse.js
Outdated
const fs = require('fs'); | ||
const path = require('path'); | ||
|
||
const distDir = path.join(__dirname, '..', 'dist'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find it humorous how this one is carefully crafted with path
and then the others are just concated and then relative to cwd 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it isn't the only build file that does this :) if it hasn't annoyed anyone enough to fix it yet...
@exterkamp has seen the internal usage, so wdyt shane? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM. From an LR consumption perspective it seems good.
Co-Authored-By: Patrick Hulce <patrick.hulce@gmail.com>
… into firehouse-bundle
Bundles smokehouse for consumption in LR / DevTools.
Googlers: see cl/275909869 / doc