tests: run most smoke tests on devtools #13456

connorjclark · 2021-12-02T23:00:44Z

Expanded yarn run-devtools:

Saves artifacts
Inject custom config
Some extra waiting to fix a bug where multiple launches of yarn run-devtools resulted in the script breaking

connorjclark · 2021-12-02T23:01:50Z

lighthouse-cli/test/smokehouse/lighthouse-runners/devtools.js

+  if (setupPromise) return setupPromise;
+
+  setupPromise = new Promise(resolve => {
+    // execFileSync('yarn', ['open-devtools']);


TODO: I thought about doing the setup for building ToT devtools here... would that be useful?

Yeah, otherwise we're only testing whatever Lighthouse is shipped in Chrome.

TODO: I thought about doing the setup for building ToT devtools here... would that be useful?

yeah, though it would be nice if it didn't attempt to re-update on every run. Is it currently only the content-shell we only update every X commits?

The GHA workflow already builds devtools before starting this smoke test as part of the layout tests ... so let's do something here but only for local runs.

export DEVTOOLS_PATH=".tmp/chromium-web-tests/devtools/devtools-frontend" bash lighthouse-core/test/chromium-web-tests/download-devtools.sh bash lighthouse-core/test/chromium-web-tests/roll-devtools.sh

(and then the local devtools smoke run would defer to w/e CHROME_PATH you have, not use the latest content shell ... unless thats your chrome path.)

connorjclark · 2021-12-02T23:02:04Z

lighthouse-core/scripts/open-devtools.sh

+# 
+# Run `yarn test-devtools` first to update the temporary devtools checkout.
+# Specify `$DEVTOOLS_PATH` to use a different devtools repo.
+


some driveby extra documentation

adamraine · 2021-12-02T23:34:49Z

lighthouse-cli/test/smokehouse/lighthouse-runners/devtools.js

+  if (setupPromise) return setupPromise;
+
+  setupPromise = new Promise(resolve => {
+    // execFileSync('yarn', ['open-devtools']);


Yeah, otherwise we're only testing whatever Lighthouse is shipped in Chrome.

.github/workflows/devtools.yml

adamraine · 2021-12-02T23:51:04Z

lighthouse-cli/test/smokehouse/lighthouse-runners/devtools.js

+  const args = [
+    'run-devtools',
+    url,
+    '--output-dir', outputDir,


We could use the custom DT frontend flag to test how PR changes affect DT.

Ah, I keep making this mistake, thinking that is the default behavior. It is for open-devtools! I want to followup with a change that does that.

Will add the arg here for now.

lighthouse-cli/test/smokehouse/lighthouse-runners/devtools.js

lighthouse-core/scripts/pptr-run-devtools.js

brendankenny · 2021-12-02T23:56:55Z

.github/workflows/devtools.yml

@@ -73,3 +73,18 @@ jobs:
      with:
        name: results
        path: ${{ github.workspace }}/lighthouse/.tmp/layout-test-results
+
+    # Run smoke tests via DevTools


Better in a new job?

How to avoid redoing all the work for building devtools?

How to avoid redoing all the work for building devtools?

Yeah it's not pretty. We could have dependent jobs (or cache the build, but dependent jobs seems workable)? I'm not sure of the best approach, but if the devtools CI job is normally going to take 30+ minutes we'll definitely want to split it up.

edit: if an already-built devtools was available, sharding would work as well

.github/workflows/devtools.yml

brendankenny · 2021-12-03T00:14:27Z

lighthouse-cli/test/smokehouse/lighthouse-runners/devtools.js

+  if (setupPromise) return setupPromise;
+
+  setupPromise = new Promise(resolve => {
+    // execFileSync('yarn', ['open-devtools']);


TODO: I thought about doing the setup for building ToT devtools here... would that be useful?

yeah, though it would be nice if it didn't attempt to re-update on every run. Is it currently only the content-shell we only update every X commits?

lighthouse-cli/test/smokehouse/frontends/smokehouse-bin.js

brendankenny · 2021-12-03T00:16:44Z

this is great!

lighthouse-core/scripts/pptr-run-devtools.js

adamraine · 2021-12-15T22:17:11Z

lighthouse-core/scripts/pptr-run-devtools.js

+  const targetManager =
+    SDK.targetManager || (SDK.TargetManager.TargetManager || SDK.TargetManager).instance();
+  if (targetManager.mainTarget() === null) {
+    if (targetManager.observeTargets) {


When would this be falsy? In an older version of DT?

Could be. It was pretty involved making sure a bunch of older chromes worked with this script, so I am doing without testing if it is necessary 😬

lighthouse-core/scripts/pptr-run-devtools.js

…tools smoke runner

adamraine

LGTM

adamraine · 2022-01-04T23:39:23Z

.github/workflows/devtools.yml

+      # TODO: run on all tests.
+      # - Current DevTools hangs on any page with a service worker.
+      #   https://github.com/GoogleChrome/lighthouse/issues/13396
+      # - Various other issues that needed investigation.


Can you create an issue to track this second bullet?

adamraine · 2022-01-04T23:55:18Z

lighthouse-cli/test/smokehouse/lighthouse-runners/devtools.js

+ * @return {Promise<{lhr: LH.Result, artifacts: LH.Artifacts, log: string}>}
+ */
+async function runLighthouse(url, configJson, testRunnerOptions = {}) {
+  const outputDir = fs.mkdtempSync(os.tmpdir() + '/lh-smoke-cdt-runner-');


Does this create multiple temp directories if the smokes are running concurrently?

yeah, fs.mkdtempSync handles that

brendankenny · 2022-01-05T00:17:07Z

lighthouse-cli/test/smokehouse/smokehouse.js

@@ -146,8 +151,19 @@ async function runSmokeTest(smokeTestDefn, testOptions) {

    // Run Lighthouse.
    try {
+      /** @type {NodeJS.Timeout} */
+      let timeout;


I mentioned this out of band, but for tracking here:

This adds some timeout redundancy here, since Lighthouse itself will also fail with an error after it times out, which leaves it somewhat unclear where modified timeouts should be specified, here or in the runners. It's not a huge deal, (100s is pretty long, so the core Lighthouse one will always end first), but simplicity-wise it might make sense to move this timeout into the new devtools.js and leave runs with the other runners unchanged.

This was mostly a nit before, but the introduction of beforeAll makes it a bigger win simplicity-wise, since devtools.js wouldn't need a beforeAll at that point, it could just call those lines itself and just exempt them from the overall timeout.

brendankenny · 2022-01-05T00:23:11Z

lighthouse-cli/test/smokehouse/lighthouse-runners/devtools.js

+    });
+    spawnHandle.on('error', reject);
+    spawnHandle.stdout.on('data', data => {
+      console.log(data.toString());


are these lines intended to stay or for debugging?

Yes, otherwise the cases where things hang or error aren't printed (the runner interface only allows returning successful logs). and it's nicer to see the output stream in.

just checking that it's intentional. The tradeoff is that concurrent runs will be streaming in stuff all interleaved with each other and hard to tell the source of each line, but that can still be helpful, especially when things are new and need to check that it's actually running, etc

lighthouse-core/scripts/pptr-run-devtools.js

lighthouse-cli/test/smokehouse/lighthouse-runners/devtools.js

brendankenny · 2022-01-05T18:40:08Z

lighthouse-cli/test/smokehouse/frontends/smokehouse-bin.js

@@ -183,7 +184,7 @@ async function begin() {
  if (argv.runner === 'bundle') {
    console.log('\n✨ Be sure to have recently run this: yarn build-all');
  }
-  const {runLighthouse} = await import(runnerPath);
+  const {runLighthouse, beforeAll} = await import(runnerPath);


can remove beforeAll (here and in types/smokehouse.d.ts)

brendankenny · 2022-01-05T18:43:03Z

lighthouse-core/scripts/pptr-run-devtools.js

+  if (self.runtime && self.runtime.loadLegacyModule) {
+    // This exposes TargetManager via self.SDK.
+    try {
+      await self.runtime.loadLegacyModule('core/sdk/sdk-legacy.js');


(I don't know the DT story here) does the "legacy" part of this path mean this is going to go away at some point?

Sure hope not, otherwise I found no way to access TargetManager!

brendankenny · 2022-01-05T18:51:37Z

lighthouse-core/scripts/pptr-run-devtools.js

@@ -155,6 +190,35 @@ async function testPage(page, browser, url) {
      .catch(reject);
  });

+  if (config) {
+    // Must attach to the Lighthouse worker target and override the `self.createConfig`


haha, this should maybe be a TODO as well, some easier way to set a config :)

brendankenny · 2022-01-05T18:56:05Z

lighthouse-cli/test/smokehouse/lighthouse-runners/devtools.js

+}
+
+/** @type {Promise<void>} */
+let beforeAllPromise;


maybe

Suggested change

let beforeAllPromise;

let buildDevtoolsPromise;

or something like that?

brendankenny · 2022-01-05T19:06:52Z

lighthouse-cli/test/smokehouse/lighthouse-runners/devtools.js

+
+/** @type {Promise<void>} */
+let beforeAllPromise;
+async function beforeAll() {


can you add a jsdoc like, "Build the lighthouse DevTools bundle and fetch and build the DevTools frontend using it" or whatever?

brendankenny · 2022-01-05T19:15:05Z

lighthouse-cli/test/smokehouse/lighthouse-runners/devtools.js

+}
+
+/**
+ * Launch Chrome and do a full Lighthouse run via DevTools.


I'm having trouble coming up with a non-awkward way to say this, but seems worth clarifying it's the current/local Lighthouse devtools bundle run in the latest DevTools frontend
vs
latest DevTools frontend with whatever Lighthouse version was last rolled into Chromium.

tests: run smokehouse on devtools

b1bf3ff

connorjclark requested a review from a team as a code owner December 2, 2021 23:00

connorjclark requested review from adamraine and removed request for a team December 2, 2021 23:00

google-cla bot added the cla: yes label Dec 2, 2021

connorjclark commented Dec 2, 2021

View reviewed changes

devtools-bot assigned adamraine Dec 2, 2021

devtools-bot added the waiting4reviewer label Dec 2, 2021

connorjclark added 2 commits December 2, 2021 15:29

tests: fix devtools build command

036c7f6

Merge branch 'dt-test-fix-roll' into smoke-devtools

119f17a

vercel bot deployed to Preview December 2, 2021 23:34 View deployment

adamraine reviewed Dec 3, 2021

View reviewed changes

brendankenny reviewed Dec 3, 2021

View reviewed changes

connorjclark added 2 commits December 2, 2021 17:23

Merge remote-tracking branch 'origin/master' into smoke-devtools

73735d4

fix path

29d5b9e

vercel bot deployed to Preview December 3, 2021 01:25 View deployment

update

397eff0

vercel bot deployed to Preview December 4, 2021 00:37 View deployment

custom devtools path

71cf0d0

vercel bot deployed to Preview December 4, 2021 00:40 View deployment

adamraine reviewed Dec 6, 2021

View reviewed changes

lighthouse-core/scripts/pptr-run-devtools.js Outdated Show resolved Hide resolved

connorjclark mentioned this pull request Dec 7, 2021

tests: clean up and add comments to cdt layout test scripts #13471

Merged

update

f2bd19a

vercel bot deployed to Preview December 15, 2021 18:54 View deployment

observeTargets

c947f8d

vercel bot deployed to Preview December 15, 2021 19:39 View deployment

adamraine reviewed Dec 15, 2021

View reviewed changes

tweak

9a47829

check for headless

5e4360c

vercel bot deployed to Preview December 31, 2021 16:31 View deployment

log

b26652c

vercel bot deployed to Preview December 31, 2021 16:50 View deployment

fix frontend path

9f17ee2

vercel bot deployed to Preview December 31, 2021 17:20 View deployment

remove bad apples

9ccb3f5

vercel bot deployed to Preview December 31, 2021 18:07 View deployment

ascii

32546f7

vercel bot deployed to Preview January 4, 2022 19:36 View deployment

comment

fd6937c

vercel bot deployed to Preview January 4, 2022 19:39 View deployment

add beforeAll to smoke runner options. build devtools locally for dev…

b79270d

…tools smoke runner

vercel bot deployed to Preview January 4, 2022 23:03 View deployment

adamraine approved these changes Jan 4, 2022

View reviewed changes

brendankenny reviewed Jan 5, 2022

View reviewed changes

adamraine reviewed Jan 5, 2022

View reviewed changes

lighthouse-core/scripts/pptr-run-devtools.js Show resolved Hide resolved

adamraine reviewed Jan 5, 2022

View reviewed changes

lighthouse-cli/test/smokehouse/lighthouse-runners/devtools.js Outdated Show resolved Hide resolved

update

b552400

vercel bot deployed to Preview January 5, 2022 18:23 View deployment

connorjclark changed the title ~~tests: run smokehouse on devtools~~ tests: run most smoke tests on devtools Jan 5, 2022

connorjclark mentioned this pull request Jan 5, 2022

Run smoke tests in devtools #13539

Closed

brendankenny reviewed Jan 5, 2022

View reviewed changes

pr

b056663

vercel bot deployed to Preview January 5, 2022 19:26 View deployment

ts

12a060c

vercel bot deployed to Preview January 5, 2022 19:47 View deployment

connorjclark merged commit 4116a7d into master Jan 5, 2022

connorjclark deleted the smoke-devtools branch January 5, 2022 20:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tests: run most smoke tests on devtools #13456

tests: run most smoke tests on devtools #13456

connorjclark commented Dec 2, 2021 •

edited

Loading

connorjclark Dec 2, 2021

adamraine Dec 2, 2021

brendankenny Dec 3, 2021

connorjclark Jan 4, 2022 •

edited

Loading

connorjclark Dec 2, 2021

adamraine Dec 2, 2021

adamraine Dec 2, 2021

connorjclark Dec 3, 2021

brendankenny Dec 2, 2021

connorjclark Dec 3, 2021 •

edited

Loading

brendankenny Jan 5, 2022 •

edited

Loading

brendankenny Dec 3, 2021

brendankenny commented Dec 3, 2021

adamraine Dec 15, 2021

connorjclark Dec 15, 2021

adamraine left a comment

adamraine Jan 4, 2022

adamraine Jan 4, 2022

brendankenny Jan 5, 2022 •

edited

Loading

brendankenny Jan 5, 2022

brendankenny Jan 5, 2022

connorjclark Jan 5, 2022

brendankenny Jan 5, 2022 •

edited

Loading

brendankenny Jan 5, 2022

brendankenny Jan 5, 2022

connorjclark Jan 5, 2022

brendankenny Jan 5, 2022

brendankenny Jan 5, 2022

brendankenny Jan 5, 2022

brendankenny Jan 5, 2022

tests: run most smoke tests on devtools #13456

tests: run most smoke tests on devtools #13456

Conversation

connorjclark commented Dec 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark Jan 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark Dec 3, 2021 • edited Loading

Choose a reason for hiding this comment

brendankenny Jan 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny commented Dec 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamraine left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny Jan 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny Jan 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark commented Dec 2, 2021 •

edited

Loading

connorjclark Jan 4, 2022 •

edited

Loading

connorjclark Dec 3, 2021 •

edited

Loading

brendankenny Jan 5, 2022 •

edited

Loading

brendankenny Jan 5, 2022 •

edited

Loading

brendankenny Jan 5, 2022 •

edited

Loading