report: show axe version in runtime settings #10729

connorjclark · 2020-05-07T23:17:20Z

brendankenny · 2020-05-07T23:26:37Z

I thought we were talking in runtime settings?

patrickhulce · 2020-05-07T23:47:00Z

I also thought we decided on runtime settings, but I kinda like this too. I'm up for either if we can reach a quick consensus?

connorjclark · 2020-05-07T23:52:14Z

sorry, I have conflated these two parts of the report, you're right the consensus was on runtime settings ...

I do think like it better in the footer though :)

brendankenny · 2020-05-08T00:07:45Z

I mean the footer isn't the credits, it's telling the user what the thing they just read was (more or less a colophon). We consider the host UA part of the testing environment in the same way as axe would be.

patrickhulce

this was marked as waiting on me, so I gave a review, but I'm a little unclear as to what's been decided re: runtime settings vs. footer.

I see arguments for both:

Runtime Settings
Mostly what @brendankenny said. The footer isn't meant to be an all-encompassing credits and I'm not sure the axe version is critical enough to the entire report to be included there.

Footer
Runtime settings are mostly things that change from run to run and are under the influence of the runner whereas the axe version is just derived from the Lighthouse version and isn't really specific to the run we saw.

Anyone wanna concede? :)

patrickhulce · 2020-05-11T15:48:25Z

lighthouse-core/runner.js

+      // fallback to package json so the report will always have some version.
+      const axeVersion = artifacts.Accessibility ?
+        artifacts.Accessibility.version :
+        require('../package.json').dependencies['axe-core'];


nit: let's combine this import of package.json with the one for lhVersion rather than require twice

I believe package-json-versionify is going to strip out dependencies and make this a cannot read property 'axe-core' of undefined. We could drop that and just live with all of package.json in our bundle, but that is kind of unfortunate.

package-json-versionify

I had no idea that existed :) bummer too because there's no way that would've been caught either, smoke test of the bundle without running a11y?

the error case @brendankenny points out of axe failing probably means we'll need the fallback even when we do run axe, maybe we should just always state this version?

patrickhulce · 2020-05-11T15:49:28Z

lighthouse-core/runner.js

@@ -131,6 +137,7 @@ class Runner {
          benchmarkIndex: artifacts.BenchmarkIndex,
        },
        lighthouseVersion,
+        axeVersion,


does this belong in environment instead of the toplevel? I guess it might depend on our resolution to runtimesettings v. footer?

does this belong in environment instead of the toplevel? I guess it might depend on our resolution to runtimesettings v. footer?

+1 to moving to environment regardless. Not even axe puts the axe version number at the top level of their results object :)

+1. but how about a toplevel credits object though? or environment.credits ?

patrickhulce · 2020-05-11T15:49:52Z

types/lhr.d.ts

@@ -41,6 +41,8 @@ declare global {
      fetchTime: string;
      /** The version of Lighthouse with which these results were generated. */
      lighthouseVersion: string;
+      /** The version of Axe with which these results were generated. */


Suggested change

/** The version of Axe with which these results were generated. */

/** The version of axe-core with which these results were generated. */

brendankenny · 2020-05-11T16:19:40Z

Runtime settings are mostly things that change from run to run and are under the influence of the runner whereas the axe version is just derived from the Lighthouse version and isn't really specific to the run we saw.

That's true, but that's partly just because we called it "runtime settings" and not the broader "environment" as it's named in the LHR :) If we ever switch to puppeteer as our chrome launcher and have a 1:1 Lighthouse version to Chrome version we probably still won't remove it from there.

brendankenny · 2020-05-11T16:30:03Z

lighthouse-core/runner.js

+      // fallback to package json so the report will always have some version.
+      const axeVersion = artifacts.Accessibility ?
+        artifacts.Accessibility.version :
+        require('../package.json').dependencies['axe-core'];


I believe package-json-versionify is going to strip out dependencies and make this a cannot read property 'axe-core' of undefined. We could drop that and just live with all of package.json in our bundle, but that is kind of unfortunate.

brendankenny · 2020-05-11T16:37:29Z

lighthouse-core/runner.js

+      // Use version from gathering stage. In case accessibility category doesn't run,
+      // fallback to package json so the report will always have some version.


Do we really care about including info on a library when we didn't run it? The larger question is probably should we be generalizing this to the equivalent of chrome://credits/ for all our libraries in the lhr but then selectively show in the report, but I also don't feel strongly.

brendankenny · 2020-05-11T16:38:04Z

lighthouse-core/runner.js

@@ -107,6 +107,12 @@ class Runner {
      // Entering: conclusion of the lighthouse result object
      const lighthouseVersion = require('../package.json').version;

+      // Use version from gathering stage. In case accessibility category doesn't run,
+      // fallback to package json so the report will always have some version.
+      const axeVersion = artifacts.Accessibility ?


The other failure mode is when artifacts.Accessibility exists but is an Error object because something threw.

let's just not show in runtime settings if we didn't find a version in the artifact

paulirish · 2020-06-08T18:42:56Z

Resolution from last group discussion: we'll add to Runtime settings as ~"axe-core version"

two additional things

we all recognize that this "runtime settings" block already includes items that belong in more of an "environment" grouping. we may explore this in the future
we may choose to expose other version numbers in the future, but right now there's no strong need. If we do, we may change the "axe-core version" item to be more of a "credits" or "dependency versions" item.

patrickhulce · 2020-06-23T14:25:38Z

FYI @connorjclark it looks like we settled discussion on this and decided to proceed in runtime settings. I'll flip this to waiting for committer status accordingly, but no rush.

patrickhulce · 2020-07-17T21:30:34Z

types/lhr.d.ts

@@ -41,6 +41,8 @@ declare global {
      fetchTime: string;
      /** The version of Lighthouse with which these results were generated. */
      lighthouseVersion: string;
+      /** The version of libraries with which these results were generated. Ex: axe-core. */
+      credits: Record<string, string>,


a new toplevel property for this still makes me cringe a little, do we really foresee ever surfacing anything else here?

I'm still in camp environment.axeVersion ducks

even after my suggestion of environment.credits ?

.environment.credits preferred to .credits for sure :)

I'm just hesitant on credits without any clue what would go into here that doesn't encourage us to start listing every dependency. is there any particular future lib you had in mind we might one day want to put into here?

I was thinking anything that informs the results of the report (so not, for example, lodash):

js-library-detector
third-party-web
robots-parser

(not exhaustive)

Not suggesting we show all in runtime settings. Would need different UI I think for more than one version. axe is the largest so am ok just showing that in runtime setting for now.

third-party-web

I see what you did there ;) I'm sold 👍 🤣 😆

connorjclark · 2020-07-17T22:37:30Z

lighthouse-core/report/html/renderer/report-renderer.js

-    ].forEach(runtime => {
-      if (!runtime.description) return;
+      ];
+    if (report.environment.credits && report.environment.credits['axe-core']) {


is this how to properly maintain backwards compat?

patrickhulce

LGTM

proto/lighthouse-result.proto

lighthouse-core/report/html/renderer/util.js

Co-authored-by: Brendan Kenny <bckenny@gmail.com>

proto/lighthouse-result.proto

… axe-version

report: show axe version in footer

c5c1c5d

connorjclark requested a review from a team as a code owner May 7, 2020 23:17

connorjclark requested review from patrickhulce and removed request for a team May 7, 2020 23:17

googlebot added the cla: yes label May 7, 2020

devtools-bot assigned patrickhulce May 7, 2020

devtools-bot added the waiting4reviewer label May 7, 2020

patrickhulce reviewed May 11, 2020

View reviewed changes

patrickhulce added needs-discussion and removed waiting4reviewer labels May 11, 2020

brendankenny reviewed May 11, 2020

View reviewed changes

patrickhulce added waiting4committer and removed needs-discussion labels Jun 23, 2020

paulirish assigned connorjclark and unassigned patrickhulce Jun 23, 2020

connorjclark added 2 commits July 17, 2020 16:20

Merge remote-tracking branch 'origin/master' into axe-version

a1d7d57

version

930a30c

vercel bot had a problem deploying to Preview July 17, 2020 21:21 Failure

connorjclark changed the title ~~report: show axe version in footer~~ report: show axe version in runtime settings Jul 17, 2020

patrickhulce reviewed Jul 17, 2020

View reviewed changes

env

baab66d

vercel bot deployed to Preview July 17, 2020 22:37 View deployment

connorjclark commented Jul 17, 2020

View reviewed changes

connorjclark added waiting4reviewer and removed waiting4committer labels Jul 20, 2020

Merge remote-tracking branch 'origin/master' into axe-version

f4a2d2f

vercel bot deployed to Preview August 18, 2020 18:20 View deployment

lint

66d4438

vercel bot deployed to Preview August 18, 2020 19:01 View deployment

connorjclark assigned patrickhulce and unassigned connorjclark Aug 26, 2020

proto

242458a

vercel bot deployed to Preview August 26, 2020 15:44 View deployment

patrickhulce approved these changes Aug 26, 2020

View reviewed changes

proto/lighthouse-result.proto Show resolved Hide resolved

patrickhulce assigned exterkamp and unassigned patrickhulce Aug 26, 2020

brendankenny approved these changes Aug 26, 2020

View reviewed changes

lighthouse-core/report/html/renderer/util.js Outdated Show resolved Hide resolved

Update lighthouse-core/report/html/renderer/util.js

b18fb66

Co-authored-by: Brendan Kenny <bckenny@gmail.com>

vercel bot deployed to Preview August 26, 2020 17:26 View deployment

exterkamp requested changes Aug 26, 2020

View reviewed changes

proto/lighthouse-result.proto Outdated Show resolved Hide resolved

proto/lighthouse-result.proto Show resolved Hide resolved

connorjclark added 2 commits August 26, 2020 12:36

casing

ad9c15c

Merge branch 'axe-version' of github.com:GoogleChrome/lighthouse into…

b60e954

… axe-version

vercel bot deployed to Preview August 26, 2020 17:41 View deployment

exterkamp approved these changes Aug 26, 2020

View reviewed changes

connorjclark added the land-when-ci-is-green label Aug 26, 2020

devtools-bot merged commit c54721a into master Aug 26, 2020

devtools-bot deleted the axe-version branch August 26, 2020 21:39

devtools-bot removed the land-when-ci-is-green label Aug 26, 2020

connorjclark mentioned this pull request Dec 10, 2020

Display plugins in footer of report #9934

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

report: show axe version in runtime settings #10729

report: show axe version in runtime settings #10729

connorjclark commented May 7, 2020 •

edited

Loading

brendankenny commented May 7, 2020

patrickhulce commented May 7, 2020

connorjclark commented May 7, 2020

brendankenny commented May 8, 2020

patrickhulce left a comment

patrickhulce May 11, 2020

brendankenny May 11, 2020 •

edited

Loading

patrickhulce May 11, 2020

patrickhulce May 11, 2020

patrickhulce May 11, 2020

brendankenny May 11, 2020

connorjclark Jul 17, 2020

patrickhulce May 11, 2020

brendankenny commented May 11, 2020

brendankenny May 11, 2020 •

edited

Loading

brendankenny May 11, 2020

brendankenny May 11, 2020 •

edited

Loading

connorjclark Jul 17, 2020

paulirish commented Jun 8, 2020 •

edited

Loading

patrickhulce commented Jun 23, 2020

patrickhulce Jul 17, 2020

connorjclark Jul 17, 2020

patrickhulce Jul 17, 2020

connorjclark Jul 17, 2020 •

edited

Loading

patrickhulce Jul 17, 2020 •

edited

Loading

connorjclark Jul 17, 2020

patrickhulce left a comment

	/** The version of Axe with which these results were generated. */
	/** The version of axe-core with which these results were generated. */

		// Use version from gathering stage. In case accessibility category doesn't run,
		// fallback to package json so the report will always have some version.

report: show axe version in runtime settings #10729

report: show axe version in runtime settings #10729

Conversation

connorjclark commented May 7, 2020 • edited Loading

brendankenny commented May 7, 2020

patrickhulce commented May 7, 2020

connorjclark commented May 7, 2020

brendankenny commented May 8, 2020

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny May 11, 2020 • 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

brendankenny commented May 11, 2020

brendankenny May 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny May 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulirish commented Jun 8, 2020 • edited Loading

patrickhulce commented Jun 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark Jul 17, 2020 • edited Loading

Choose a reason for hiding this comment

patrickhulce Jul 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce left a comment

Choose a reason for hiding this comment

connorjclark commented May 7, 2020 •

edited

Loading

brendankenny May 11, 2020 •

edited

Loading

brendankenny May 11, 2020 •

edited

Loading

brendankenny May 11, 2020 •

edited

Loading

paulirish commented Jun 8, 2020 •

edited

Loading

connorjclark Jul 17, 2020 •

edited

Loading

patrickhulce Jul 17, 2020 •

edited

Loading