cli(output): Add ability to export results to CSV #4912

nourikhalass · 2018-04-02T22:32:53Z

These changes implement the ability to export results to an CSV file.
These changes address the idea proposed in #4325 and #4481.

Each row of the CSV file contains the information of one audit. Currently the following properties are exported:

Name of the category the audit belongs to
Name of the audit
Description of the audit
Score type used for the audit
Score value

These properties were selected arbitrarily and perhaps there are more / better ones to export. The implementation relies on type information about the result object that is passed to the Printer in lighthouse-cli. This information was lacking so it has been added in order to satisfy the typechecker. However, the added type information should probably be expanded further and used in other places to have better type safety in the complete code base.

Co-authored-by: Michiel van Spaendonck <mvanspaendonck@users.noreply.github.com>

Co-authored-by: Michiel van Spaendonck <mvanspaendonck@users.noreply.github.com> Co-authored-by: Kanav Anand <anandkanav92@users.noreply.github.com> Co-authored-by: Ernst Mulders <ernstmul@users.noreply.github.com>

These changes were made because the CSV generator requires type information in order to be 'correct'. It is very likely that parts of the codebase need to be annotated with these typings to increase type correctness. Co-authored-by: Michiel van Spaendonck <mvanspaendonck@users.noreply.github.com> Co-authored-by: Kanav Anand <anandkanav92@users.noreply.github.com> Co-authored-by: Ernst Mulders <ernstmul@users.noreply.github.com>

Co-authored-by: Michiel van Spaendonck <mvanspaendonck@users.noreply.github.com> Co-authored-by: Kanav Anand <anandkanav92@users.noreply.github.com> Co-authored-by: Ernst Mulders <ernstmul@users.noreply.github.com>

googlebot · 2018-04-02T22:33:01Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

nourikhalass · 2018-04-02T22:38:20Z

@mvanspaendonck, @anandkanav92, @ernstmul Y'all need to give Google the Good Ol' "I hereby give up my rights"!

mvanspaendonck · 2018-04-02T23:04:50Z

The cla should be signed by me already, but this is still aall fine with me.

anandkanav92 · 2018-04-02T23:06:08Z

Fine by me 🤷🏼‍♂️

ernstmul · 2018-04-03T08:14:35Z

Fine by me as well!

paulirish · 2018-04-03T18:41:26Z

lighthouse-cli/printer.js

+
+  // Possible TODO: tightly couple headers and row values
+  // This would make it easier to include new / other row values
+  const header = ['category', 'name', 'description', 'type', 'score'];


we're going to be changing 'description' to 'title' soon, so we might as well use 'title' in the csv now so we don't break you in a week or so. :)

paulirish · 2018-04-03T18:43:01Z

lighthouse-cli/test/cli/printer-test.js

@@ -35,6 +36,22 @@ describe('Printer', () => {
    assert.ok(/<html lang="en"/gim.test(htmlOutput));
  });

+  it('creates CSV for results', () => {
+    const mode = Printer.OutputMode.csv;
+    const path = './.test-file.csv';


'./.test-file.csv' => './.results-as-csv.csv'

paulirish · 2018-04-03T18:46:06Z

lighthouse-cli/test/cli/printer-test.js

+    return Printer
+      .write(sampleResults, mode, path)
+      .then(_ => csv(path, headers).catch(err => assert.fail(err)))
+      .then(_ => fs.unlinkSync(path));


i think we want this unlinkSync in an after block:

after(() => { fs.unlinkSync(path) });

doing this means you don't need that catch() anymore:

.then(_ => csv(path, headers));

As far as I understand that would require creating another describe(...). Is this what you had in mind?

describe('CSV report generation', () => { const mode = Printer.OutputMode.csv; const path = './.results-as-csv.csv'; const headers = { category: '', name: '', title: '', type: '', score: 42, }; before(() => { return Printer.write(sampleResults, mode, path); }); it('generates valid CSV output', () => { return csv(path, headers); }); after(() => { fs.unlinkSync(path); }); });

ah true. i was hoping after could be scoped to that test. whoops.

okay in that case, nevermind. :)

Is this working? assert.fail just re-throws the error and we never enter the unlinkSync block when there's a failure, no?

Maybe I'm just confused :)

patrickhulce

thanks for the contribution folks! yay teamwork! 🥇

patrickhulce · 2018-04-03T19:13:13Z

lighthouse-cli/printer.js

+
+  // Map every audit to its corresponding category
+  // which can be queried when needed
+  const map = {};


nit: let's give this a more descriptive name auditIdToCategoryNameMap?

patrickhulce · 2018-04-03T19:13:49Z

lighthouse-cli/printer.js

+ *  - the score type that is used for the audit
+ *  - the score value of the audit
+ * 
+ * @param {!LH.Results} results


nit: we don't need the ! anymore

patrickhulce · 2018-04-03T19:13:59Z

lighthouse-cli/printer.js

+  // The document describes how to deal with escaping commas and quotes etc.
+  const CRLF = '\r\n';
+  const separator = ',';
+  /** @param {!string} value @returns {string} */


patrickhulce · 2018-04-03T19:14:28Z

package.json

@@ -92,6 +92,7 @@
    "chrome-devtools-frontend": "1.0.422034",
    "chrome-launcher": "^0.10.2",
    "configstore": "^3.1.1",
+    "csv-validator": "^0.0.3",


let's move this to devDependencies

Co-authored-by: Michiel van Spaendonck <mvanspaendonck@users.noreply.github.com> Co-authored-by: Kanav Anand <anandkanav92@users.noreply.github.com> Co-authored-by: Ernst Mulders <ernstmul@users.noreply.github.com>

paulirish · 2018-04-04T02:03:08Z

wfm. over to patrick.

patrickhulce

just a few more small things but we're looking good!

patrickhulce · 2018-04-04T02:17:37Z

lighthouse-cli/test/cli/printer-test.js

+    return Printer
+      .write(sampleResults, mode, path)
+      .then(_ => csv(path, headers).catch(err => assert.fail(err)))
+      .then(_ => fs.unlinkSync(path));


Is this working? assert.fail just re-throws the error and we never enter the unlinkSync block when there's a failure, no?

Maybe I'm just confused :)

patrickhulce · 2018-04-04T02:19:42Z

typings/externs.d.ts

@@ -88,6 +88,19 @@ declare global {
      artifacts?: Object;
      initialUrl: string;
      fetchedAt: string;
+      reportCategories: [ReportCategory];


I think we want ReportCategory[], this is a type for an array of length one containing a ReportCategory :)

patrickhulce · 2018-04-04T02:19:48Z

typings/externs.d.ts

+    export interface ReportCategory {
+      name: string;
+      description: string;
+      audits: [ReportAudit];


brendankenny · 2018-04-04T06:04:35Z

lighthouse-cli/test/cli/printer-test.js

@@ -10,6 +10,7 @@ const Printer = require('../../printer.js');
 const assert = require('assert');
 const fs = require('fs');
 const sampleResults = require('../../../lighthouse-core/test/results/sample_v2.json');
+const csv = require('csv-validator');


nit: maybe const csvValidator so it's obvious what it's up to down in the test function?

brendankenny · 2018-04-04T06:07:54Z

lighthouse-cli/test/cli/printer-test.js

+    };
+    return Printer
+      .write(sampleResults, mode, path)
+      .then(_ => csv(path, headers).catch(err => assert.fail(err)))


since csv(path, headers) resolves with a json form of the CSV file, maybe also assert a row or two of the results, so it's not just checking their types?

brendankenny · 2018-04-04T06:20:25Z

lighthouse-cli/printer.js

+  // Possible TODO: tightly couple headers and row values
+  // This would make it easier to include new / other row values
+  const header = ['category', 'name', 'title', 'type', 'score'];
+  const table = Object


it looks like this could be turned into a single (nested) loop to skip making auditCategories and make this a bit simpler? Correct me if I'm wrong though :) Something like

const table = results.reportCategories.map(category => { return category.audits.map(catAudit => { const audit = results.audits[catAudit.id]; return [category.name, audit.name, audit.description, audit.scoreDisplayMode, audit.score] .map(value => value.toString()) .map(escape); }); })

then just need to smoosh before outputting

These arrays are so small it really comes down to style, though.

mvanspaendonck

@patrickhulce Maybe this makes it more clear what is going on and I have verified that this works correctly:

it('creates CSV for results', () => {
  const mode = Printer.OutputMode.csv;
  const path = './.results-as-csv.csv';
  const headers = {
    category: '',
    name: '',
    title: '',
    type: '',
    score: 42,
  };
  return Printer
    .write(sampleResults, mode, path)
    .then(async () => {
      try {
        await csvValidator(path, headers);
      } catch (err) {
        assert.fail('CSV parser error:\n' + err.join('\n'));
      } finally {
        fs.unlinkSync(path);
      }
    });
});

patrickhulce · 2018-04-04T16:11:43Z

ah yeah async/await cleans this up a lot! let's do it 👍 :)

Co-authored-by: Michiel van Spaendonck <mvanspaendonck@users.noreply.github.com> Co-authored-by: Kanav Anand <anandkanav92@users.noreply.github.com> Co-authored-by: Ernst Mulders <ernstmul@users.noreply.github.com>

patrickhulce

could probably async up the rest of the test too ;) but this LGTM!!

thanks again folks, nice work!! 👏 💯

off to you @brendankenny

brendankenny

LGTM

brendankenny · 2018-04-09T22:39:15Z

appveyor errors should be fixed by #4933, so good to go. Thanks for the PR!

nourikhalass and others added 7 commits March 30, 2018 21:59

Remove domhtml reference from README.md

c8e65df

Co-authored-by: Michiel van Spaendonck <mvanspaendonck@users.noreply.github.com>

Implement CSV report output

2dcb435

Co-authored-by: Michiel van Spaendonck <mvanspaendonck@users.noreply.github.com> Co-authored-by: Kanav Anand <anandkanav92@users.noreply.github.com> Co-authored-by: Ernst Mulders <ernstmul@users.noreply.github.com>

Merge remote-tracking branch 'upstream/master'

5bd37db

Add test for CSV report

5fb852f

Co-authored-by: Michiel van Spaendonck <mvanspaendonck@users.noreply.github.com> Co-authored-by: Kanav Anand <anandkanav92@users.noreply.github.com> Co-authored-by: Ernst Mulders <ernstmul@users.noreply.github.com>

Update docs

bc4e356

Co-authored-by: Michiel van Spaendonck <mvanspaendonck@users.noreply.github.com> Co-authored-by: Kanav Anand <anandkanav92@users.noreply.github.com> Co-authored-by: Ernst Mulders <ernstmul@users.noreply.github.com>

Update dependencies

bc0f4a4

Co-authored-by: Michiel van Spaendonck <mvanspaendonck@users.noreply.github.com> Co-authored-by: Kanav Anand <anandkanav92@users.noreply.github.com> Co-authored-by: Ernst Mulders <ernstmul@users.noreply.github.com>

nourikhalass requested review from patrickhulce and paulirish as code owners April 2, 2018 22:32

paulirish requested changes Apr 3, 2018

View reviewed changes

patrickhulce suggested changes Apr 3, 2018

View reviewed changes

Implement feedback

e6120ed

Co-authored-by: Michiel van Spaendonck <mvanspaendonck@users.noreply.github.com> Co-authored-by: Kanav Anand <anandkanav92@users.noreply.github.com> Co-authored-by: Ernst Mulders <ernstmul@users.noreply.github.com>

devtools-bot added the waiting4reviewer label Apr 3, 2018

paulirish approved these changes Apr 4, 2018

View reviewed changes

patrickhulce suggested changes Apr 4, 2018

View reviewed changes

brendankenny reviewed Apr 4, 2018

View reviewed changes

mvanspaendonck reviewed Apr 4, 2018

View reviewed changes

nourikhalass and others added 3 commits April 4, 2018 18:23

Fix typings & Improve test

03362c2

Co-authored-by: Michiel van Spaendonck <mvanspaendonck@users.noreply.github.com> Co-authored-by: Kanav Anand <anandkanav92@users.noreply.github.com> Co-authored-by: Ernst Mulders <ernstmul@users.noreply.github.com>

Merge remote-tracking branch 'upstream/master'

a58396b

Merge remote-tracking branch 'upstream/master'

a5451f3

patrickhulce approved these changes Apr 5, 2018

View reviewed changes

patrickhulce assigned brendankenny Apr 5, 2018

improved loop

209cc46

async the whole test.

71cc1a5

brendankenny approved these changes Apr 5, 2018

View reviewed changes

brendankenny merged commit a031e46 into GoogleChrome:master Apr 9, 2018

paulirish removed the waiting4reviewer label Apr 30, 2018

paulirish mentioned this pull request Aug 29, 2018

Export from report to CSV #4481

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cli(output): Add ability to export results to CSV #4912

cli(output): Add ability to export results to CSV #4912

nourikhalass commented Apr 2, 2018

googlebot commented Apr 2, 2018

nourikhalass commented Apr 2, 2018

mvanspaendonck commented Apr 2, 2018

anandkanav92 commented Apr 2, 2018

ernstmul commented Apr 3, 2018

paulirish Apr 3, 2018 •

edited

Loading

paulirish Apr 3, 2018

paulirish Apr 3, 2018

paulirish Apr 3, 2018

nourikhalass Apr 3, 2018

paulirish Apr 3, 2018

patrickhulce Apr 4, 2018

patrickhulce left a comment

patrickhulce Apr 3, 2018

patrickhulce Apr 3, 2018

patrickhulce Apr 3, 2018

patrickhulce Apr 3, 2018

paulirish commented Apr 4, 2018

patrickhulce left a comment

patrickhulce Apr 4, 2018

patrickhulce Apr 4, 2018

patrickhulce Apr 4, 2018

brendankenny Apr 4, 2018

brendankenny Apr 4, 2018 •

edited

Loading

brendankenny Apr 4, 2018

brendankenny Apr 4, 2018

mvanspaendonck left a comment •

edited

Loading

patrickhulce commented Apr 4, 2018

patrickhulce left a comment •

edited

Loading

brendankenny left a comment

brendankenny commented Apr 9, 2018

cli(output): Add ability to export results to CSV #4912

cli(output): Add ability to export results to CSV #4912

Conversation

nourikhalass commented Apr 2, 2018

googlebot commented Apr 2, 2018

nourikhalass commented Apr 2, 2018

mvanspaendonck commented Apr 2, 2018

anandkanav92 commented Apr 2, 2018

ernstmul commented Apr 3, 2018

paulirish Apr 3, 2018 • 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

patrickhulce left a 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

paulirish commented Apr 4, 2018

patrickhulce left a 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 Apr 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvanspaendonck left a comment • edited Loading

Choose a reason for hiding this comment

patrickhulce commented Apr 4, 2018

patrickhulce left a comment • edited Loading

Choose a reason for hiding this comment

brendankenny left a comment

Choose a reason for hiding this comment

brendankenny commented Apr 9, 2018

paulirish Apr 3, 2018 •

edited

Loading

brendankenny Apr 4, 2018 •

edited

Loading

mvanspaendonck left a comment •

edited

Loading

patrickhulce left a comment •

edited

Loading