Skip to content
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: use smokehouse runner for test-bundle #9943

Merged
merged 4 commits into from
Dec 14, 2019
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ script:
- yarn test-clients
- yarn test-viewer
- yarn test-lantern
- yarn test-bundle || yarn test-bundle || yarn test-bundle
- yarn test-bundle
- yarn i18n:checks
- yarn dogfood-lhci
before_cache:
Expand Down
3 changes: 2 additions & 1 deletion build/build-bundle.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,8 @@ const isDevtools = file => path.basename(file).includes('devtools');
/** @param {string} file */
const isLightrider = file => path.basename(file).includes('lightrider');

const BANNER = `// lighthouse, browserified. ${VERSION} (${COMMIT_HASH})\n`;
const BANNER = `// lighthouse, browserified. ${VERSION} (${COMMIT_HASH})\n` +
'// @ts-nocheck\n'; // To prevent tsc stepping into any required bundles.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since dist/lighthouse-dt-bundle.js is required in smokehouse and smokehouse is type checked, tsc automatically starts checking the bundle code too. This is the only reliable way I could think of to stop it, but happy for alternatives :)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we just exclude the bundle in tsconfig?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we just exclude the bundle in tsconfig?

from my experience this isn't possible (include and exclude are just including and excluding root files that tsc starts walking from), but I would be happy if I was missing something :)

See https://www.typescriptlang.org/docs/handbook/tsconfig-json.html#details ("Any files that are referenced by files included...")

const DEBUG = false; // true for sourcemaps

/**
Expand Down
70 changes: 0 additions & 70 deletions build/tests/bundle-smoke-test.js

This file was deleted.

100 changes: 0 additions & 100 deletions build/tests/bundled-lighthouse-cli.js

This file was deleted.

13 changes: 6 additions & 7 deletions clients/devtools-entry.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,13 +50,12 @@ function listenForStatus(listenCallback) {
}

if (typeof module !== 'undefined' && module.exports) {
// export for require()ing (via browserify).
module.exports = {
runLighthouseInWorker,
listenForStatus,
registerLocaleData,
lookupLocale,
};
// Ideally this could be exposed via browserify's `standalone`, but it doesn't
// work for LH because of https://github.com/browserify/browserify/issues/968.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// work for LH because of https://github.com/browserify/browserify/issues/968.
// work for LH because of https://github.com/browserify/browserify/issues/968

(period broke link)

// Instead, since this file is only ever run in node for testing, expose a
// bundle entry point as global.
// @ts-ignore
global.runBundledLighthouse = lighthouse;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

originally was going to make runLighthouseInWorker global, but we need to substitute in a connection and config (instead of port and categoryIds). Rather than making the function unreadable with a bunch of options or requiring devtools to call the module differently (which we could still do in the future), exposing the inner lighthouse call seemed like the simplest fix.

I am slightly concerned about drift between how devtools calls runLighthouseInWorker and what happens inside that function and the default interface, but for now it's a really simple correspondence.

Copy link
Collaborator

@connorjclark connorjclark Nov 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This drift sounds fine to me. When I have the smoke tests run against the Audits panel, it will be at a higher level than this bundle, so the drift between runBundledLighthouse and runLighthouseInWorker is irrelevant (won't even used the former). I will need to add a config, but that's simple.

So while runBundledLighthouse happens to live here, it's got nothing to do with devtools. That is a little confusing - what about making a new client for this, bundle-entry.js?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I have the smoke tests run against the Audits panel, it will be at a higher level than this bundle

ah, good to know, I didn't realize that, though it makes sense.

So while runBundledLighthouse happens to live here, it's got nothing to do with devtools. That is a little confusing - what about making a new client for this, bundle-entry.js?

Agree it's kind of confusing, but the benefit is it's getting all the bundle customizations--all the isDevTools tweaks, for instance--so it's making sure the real bundles are working. I was thinking we could add a smoke test just for the pubAds integration, for instance.

Originally I had it something like

function runLighthouseInWorker() {
  // ...
  runLighthouse(...args);
}

/**
 * Intermediate function to expose for testing.
 */
function runLighthouse() {
  lighthouse(...args)
}

global.runBundledLighthouse = runLighthouse;

that spelled it out a little more clearly but also seemed a bit silly.

We could change the runLighthouseInWorker interface to make it more flexible, too (like make it take a config and devtools code could call getDefaultConfigForCategories with the categoryIds itself and then pass the config back in), but I figured now's probably not the time for such changes :)

Other ways we could make things more explicit and clearer?

}

// Expose only in DevTools' worker
Expand Down
19 changes: 13 additions & 6 deletions lighthouse-cli/test/smokehouse/frontends/smokehouse-bin.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,15 @@ const log = require('lighthouse-logger');
const {runSmokehouse} = require('../smokehouse.js');
const {server, serverForOffline} = require('../../fixtures/static-server.js');

const cliLighthouseRunner = require('../lighthouse-runners/cli.js').runLighthouse;

const coreTestDefnsPath = require.resolve('../test-definitions/core-tests.js');

const runners = {
cli: cliLighthouseRunner,
/**
* Possible Lighthouse runners. Loaded dynamically so e.g. a CLI run isn't
* contingent on having built all the bundles.
*/
const runnerPaths = {
cli: '../lighthouse-runners/cli.js',
bundle: '../lighthouse-runners/bundle.js',
};

/**
Expand Down Expand Up @@ -76,7 +79,7 @@ async function begin() {
.alias({
'jobs': 'j',
})
.choices('runner', ['cli'])
.choices('runner', ['cli', 'bundle'])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should there be a --runner-path, so others could make use a custom runner w/o changing this file?

makes sense to just wait for someone to request this feature, but it's easy enough to do perhaps we should just do it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I will be one such user in the near future :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should there be a --runner-path, so others could make use a custom runner w/o changing this file?

Could we just add it when you need it? Agreed it fits in easily, these are more or less just shorthand forms of that, but I'd like to avoid messing with yargs more right now :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should there be a --runner-path, so others could make use a custom runner w/o changing this file?

though smokehouse.js can already take any custom runner without changes. I kind of figured that when we needed a really custom runner, we would also need firehouse's test filtering and so could just make a new frontend (smoke-mask :D). Like 80% of this file is parsing argv anyways.

.default('runner', 'cli')
.wrap(yargs.terminalWidth())
.argv;
Expand All @@ -85,7 +88,11 @@ async function begin() {
const jobs = argv.jobs !== undefined ? Number(argv.jobs) : undefined;
const retries = argv.retries !== undefined ? Number(argv.retries) : undefined;

const lighthouseRunner = runners[/** @type {keyof typeof runners} */ (argv.runner)];
const runnerPath = runnerPaths[/** @type {keyof typeof runnerPaths} */ (argv.runner)];
if (argv.runner === 'bundle') {
console.log('\n✨ Be sure to have recently run this: yarn build-all');
connorjclark marked this conversation as resolved.
Show resolved Hide resolved
}
const lighthouseRunner = require(runnerPath).runLighthouse;

// Find test definition file and filter by requestedTestIds.
let testDefnPath = argv.testsPath || coreTestDefnsPath;
Expand Down
54 changes: 54 additions & 0 deletions lighthouse-cli/test/smokehouse/lighthouse-runners/bundle.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
/**
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe this should be launch-chrome-and-bundle.js or something to differentiate from the pure bundle runner needed by #9873. We could bikeshed now or later

* @license Copyright 2019 Google Inc. All Rights Reserved.
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License.
*/
'use strict';

/**
* @fileoverview A runner that launches Chrome and executes Lighthouse via a
* bundle to test that bundling has produced correct and runnable code.
* Currently uses `lighthouse-dt-bundle.js`.
*/

const ChromeLauncher = require('chrome-launcher');
const ChromeProtocol = require('../../../../lighthouse-core/gather/connections/cri.js');

// Load bundle, which creates a `global.runBundledLighthouse`.
require('../../../../dist/lighthouse-dt-bundle.js');

// Import the main lighthouse() signature for easy type verification.
/** @type {import('../../../../lighthouse-core/index.js')} */
// @ts-ignore - not worth giving test global an actual type.
const lighthouse = global.runBundledLighthouse;

/**
* Launch Chrome and do a full Lighthouse run via the Lighthouse CLI.
* @param {string} url
* @param {LH.Config.Json=} configJson
* @param {{isDebug?: boolean}=} testRunnerOptions
* @return {Promise<{lhr: LH.Result, artifacts: LH.Artifacts, log: string}>}
*/
async function runLighthouse(url, configJson, testRunnerOptions = {}) {
// Launch and connect to Chrome.
const launchedChrome = await ChromeLauncher.launch();
const port = launchedChrome.port;
const connection = new ChromeProtocol(port);

// Run Lighthouse.
const logLevel = testRunnerOptions.isDebug ? 'info' : undefined;
const runnerResult = await lighthouse(url, {port, logLevel}, configJson, connection);
if (!runnerResult) throw new Error('No runnerResult');

// Clean up and return results.
await launchedChrome.kill();
return {
lhr: runnerResult.lhr,
artifacts: runnerResult.artifacts,
log: '', // TODO: if want to run in parallel, need to capture lighthouse-logger output.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is a way to control where debug output goes, but I'm skipping looking into the complications of that (singletons! :shakes_fist:) for now since I don't think we're going to want to run the bundled version multiple times in the same process. If it's only ever run with -j=1, the output will always be properly ordered.

};
}

module.exports = {
runLighthouse,
};
14 changes: 7 additions & 7 deletions lighthouse-cli/test/smokehouse/readme.md
Original file line number Diff line number Diff line change
Expand Up @@ -79,20 +79,20 @@ The different frontends launch smokehouse with a set of tests to run. Smokehouse
Smokehouse Frontends Lighthouse Runners
+------------+
| |
| bin (CLI) +----+ +--------------+
| bin.js +----+ +--------------+
| | | | |
+------------+ | +-->+ cli |
+------------+ | +-->+ cli.js |
| | | |
+------------+ | +---------------+ | +--------------+
| | | testDefns> | | config> |
| node +---------------->+ smokehouse.js +<---------+
| node.js +---------------->+ smokehouse.js +<---------+
| | | | | <lhr | +--------------+
+------------+ | +-------+-------+ | | |
| ^ +-->+ bundled |
| ^ +-->+ bundle.js |
+------------+ | | | |
| | | | +--------------+
|bundle+entry+----+ v
| | +--------+--------+
| (TODO) | +--------+--------+
+------------+ | |
| report/assert |
| |
Expand All @@ -111,8 +111,8 @@ Smokehouse Frontends Lighthouse Runners

### Lighthouse runners

- `runners/cli.js` - the original test runner, exercising the Lighthouse CLI from command-line argument parsing to the results written to disk on completion.
- TODO: bundle runner - a smoke test runner that operates on an already-bundled version of Lighthouse for end-to-end testing of that version.
- `lighthouse-runners/cli.js` - the original test runner, exercising the Lighthouse CLI from command-line argument parsing to the results written to disk on completion.
- `lighthouse-runners/bundle.js` - a smoke test runner that operates on an already-bundled version of Lighthouse for end-to-end testing of that version.

## Custom smoke tests (for plugins et al.)

Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
"debug": "node --inspect-brk ./lighthouse-cli/index.js",
"start": "node ./lighthouse-cli/index.js",
"test": "yarn diff:sample-json && yarn lint --quiet && yarn unit && yarn type-check",
"test-bundle": "node build/tests/bundle-smoke-test.js",
"test-bundle": "yarn smoke --runner bundle -j=1 --retries=2 dbw byte",
"test-clients": "jest \"clients/\"",
"test-viewer": "yarn unit-viewer && jest lighthouse-viewer/test/viewer-test-pptr.js",
"test-lantern": "bash lighthouse-core/scripts/test-lantern.sh",
Expand Down