-
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
tests(build): use firehouse smoke test runner to test bundle #9791
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.
very clever stuff :) 💭 💡
const LH_ROOT = path.resolve(__dirname, '../..'); | ||
const corePath = `${LH_ROOT}/lighthouse-core/index.js`; | ||
|
||
// Oh yeahhhhh this works. Think of this as `requireBundled('../../lighthouse-core/index.js')`. |
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.
haha I don't know what this means but it's hilarious :)
ha, I was working on a similar change, though it's kind of the inverse of this, which allows unification of some stuff. Maybe we can combine powers. |
Is there a branch? |
I'll get this together now |
FWIW I don't think we should block #9193 on this, since I'd like to propose a bunch of changes here :) I probably won't finish my proposal today, though. Really the purpose of this PR is to automatically verify that we don't break the bundle, which isn't a super emergency since we'll be doing things like releasing to devtools in the next week or so, which will involve manual testing of the bundle. Alternatively, we could land this PR and consider my proposal to radically alter it afterwards. I don't have a strong preference. |
To be clear, the changes you want to propose will be related to the bundle test, not firehouse (the smokehouse test runner), correct? |
I think the bundled test is self-contained enough to just push this and consider your changes later. Also blocked by this is #9605 (although, can wait) and further integration tests (although, could break out the firehouse changes to continue on that front). |
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.
LGTM, we can pickup debate of firehouse API in Brendan's followup. With this landed we'll have an actual consumer we care about that must be satisfied too :)
both
My main issue is that if smokehouse has grown so many layers that it was easier to write a new script that does the same thing, and the replacement script is really simple (loop over runs, compare against expectations), the root problem should be pretty obvious. |
We could replace the core smoke test runner with the same, it'd just need a mechanism for batching, if we want to keep that. |
exactly :) (also github save comment state is apparently flaky :) |
This patch
firehouse
) to run smoke tests with options providing a hook forrunLighthouse
and ways to modify the expectations. This has already been done internally to test Lightrider - this is just polishing that work and bringing it home.firehouse
to run a few smoke tests on a nifty patched CLI that uses a bundledlighthouse-core
.I have a followup patch for building a standalone
firehouse
and rolling it to DevTools, with a layout test that usesfirehouse
with properly filtered expectations. It'll be a local test at first (run only during release procedures), but eventually it'll be used by whatever process we come up with to integration test our DevTools channel.Related:
#9501 (and #9501 (comment))
Need to land bundle tests before:
#9605 (changes to bundling script)
#9193 (removes only tests that uses bundled code)