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.

Sign up for GitHub

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

Jump to bottom

fix: save artifacts and assets in output path #1601

Merged
merged 8 commits into from
Feb 7, 2017
Merged

fix: save artifacts and assets in output path #1601

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
92fb9ab
fix: save artifacts in output path
patrickhulce Feb 2, 2017
415df64
feedback restructuring
patrickhulce Feb 3, 2017
6c707b8
remove stray title
patrickhulce Feb 3, 2017
c3e18ff
rename outputPath
patrickhulce Feb 3, 2017
407e409
Merge remote-tracking branch 'origin/master' into asset_path
patrickhulce Feb 3, 2017
e311a48
fix merge conflicts
patrickhulce Feb 3, 2017
a6b2d34
feedback
patrickhulce Feb 6, 2017
69ebf22
moar feedback
patrickhulce Feb 7, 2017
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
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ last-run-results.html
*.trace.json
*.screenshots.html
*.report.html
*.artifacts.log

closure-error.log

Expand Down
52 changes: 31 additions & 21 deletions lighthouse-cli/bin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,36 @@ function handleError(err: LighthouseError) {
}
}

function saveResults(results: Results,
flags: {output: any, outputPath: string, saveArtifacts: boolean, saveAssets: boolean}) {
let promise = Promise.resolve(results);
const cwd = process.cwd();
const configuredPath = !flags.outputPath || flags.outputPath === 'stdout' ?
Copy link
Member

Choose a reason for hiding this comment

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

it would be great to get some of the PR's first comment captured here or in a doc on the method (readme too?).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

assetSaver.getFilenamePrefix(results) :
flags.outputPath.replace(/\.\w{2,4}$/, '');
Copy link
Member

Choose a reason for hiding this comment

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

instead of this edit we could do...

const parsed = path.parse(flags.outputPath); 
parsed.base = parsed.base.replace(parsed.ext, '');
path.format(parsed);

but i don't know if that's really worth it. :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I like the explicitness of it but not a huge fan of the triple lines, haha :)

const resolvedPath = path.resolve(cwd, configuredPath);
const pathWithBasename = resolvedPath.includes(cwd) ?
resolvedPath.slice(cwd.length + 1) : resolvedPath;
Copy link
Member

Choose a reason for hiding this comment

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

maybe i'm missing something obvious, but what does slicing accomplish here?

Copy link
Member

Choose a reason for hiding this comment

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

my read is that resolvedPath always includes cwd because we just did path.resolve on it.
and so we'll always slice,
taking the absolute file path back to a relative one.

This is 1) the same as path.relative(cwd, resolvedPath) AFAICT. but 2) i don't know why we nuke it, since its a more absolute location.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just nuked because with the default options the logging seemed a bit verbose to restate your cwd, don't have particularly strong feelings about it though if we're fine with the absolute path being printed.


// delete artifacts from result so reports won't include artifacts.
const artifacts = results.artifacts;
Copy link
Member

Choose a reason for hiding this comment

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

maybe keep this in the caller and pass in artifacts as a separate argument to saveResults? Feels weird to remove them from the results in a function that's just saving things

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it feels weird have this part of the saving logic outside saveResults haha, compromise? results = Object.assign({}, results, {artifacts: undefined});

results.artifacts = undefined;

if (flags.saveArtifacts) {
assetSaver.saveArtifacts(artifacts, pathWithBasename);
Copy link
Member

Choose a reason for hiding this comment

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

as mentioned offline, we can remove --save-artifacts flag and the functions if we want. AFAIK, they havent been used for ages and are only for debugging.

}

if (flags.saveAssets) {
promise = promise.then(_ => assetSaver.saveAssets(artifacts, results.audits, pathWithBasename));
}

if (flags.output === Printer.OutputMode[Printer.OutputMode.pretty]) {
promise = promise.then(_ => Printer.write(results, 'html', `${pathWithBasename}.report.html`));
}

return promise.then(_ => Printer.write(results, flags.output, flags.outputPath));
Copy link
Member

Choose a reason for hiding this comment

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

you're inheriting this, but it seems kind of magical that we rely on Printer.write to return results. How do you feel about doing a resolve on results here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed the need for it to return anything at all

}

function runLighthouse(url: string,
flags: {port: number, skipAutolaunch: boolean, selectChrome: boolean, output: any,
outputPath: string, interactive: boolean, saveArtifacts: boolean, saveAssets: boolean},
Expand All @@ -259,27 +289,7 @@ function runLighthouse(url: string,
.then(() => getDebuggableChrome(flags))
.then(chrome => chromeLauncher = chrome)
.then(() => lighthouse(url, flags, config))
.then((results: Results) => {
// delete artifacts from result so reports won't include artifacts.
const artifacts = results.artifacts;
results.artifacts = undefined;

if (flags.saveArtifacts) {
assetSaver.saveArtifacts(artifacts);
}
if (flags.saveAssets) {
return assetSaver.saveAssets(artifacts, results).then(() => results);
}
return results;
})
.then((results: Results) => Printer.write(results, flags.output, flags.outputPath))
.then((results: Results) => {
if (flags.output === Printer.OutputMode[Printer.OutputMode.pretty]) {
const filename = `${assetSaver.getFilenamePrefix(results)}.report.html`;
return Printer.write(results, 'html', filename);
}
return results;
})
.then((results: Results) => saveResults(results, flags))
.then((results: Results) => {
if (flags.interactive) {
return performanceXServer.hostExperiment({url, flags, config}, results);
Expand Down
43 changes: 22 additions & 21 deletions lighthouse-core/lib/asset-saver.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,13 +48,12 @@ function getFilenamePrefix(results) {
/**
* Generate basic HTML page of screenshot filmstrip
* @param {!Array<{timestamp: number, datauri: string}>} screenshots
* @param {!Results} results
* @return {!string}
*/
function screenshotDump(screenshots, results) {
function screenshotDump(screenshots) {
return `
<!doctype html>
<title>screenshots ${getFilenamePrefix(results)}</title>
<title>screenshots</title>
<style>
html {
overflow-x: scroll;
Expand Down Expand Up @@ -91,24 +90,24 @@ img {
/**
* Save entire artifacts object to a single stringified file
Copy link
Member

Choose a reason for hiding this comment

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

maybe add a line to the jsdoc that artifacts will be saved at pathWithBasename + '.artifacts.log'

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

* @param {!Artifacts} artifacts
* @param {!string} artifactsFilename
* @param {string} pathWithBasename
*/
// Set to ignore because testing it would imply testing fs, which isn't strictly necessary.
/* istanbul ignore next */
function saveArtifacts(artifacts, artifactsFilename) {
artifactsFilename = artifactsFilename || 'artifacts.log';
function saveArtifacts(artifacts, pathWithBasename) {
const fullPath = `${pathWithBasename}.artifacts.log`;
// The networkRecords artifacts have circular references
fs.writeFileSync(artifactsFilename, stringifySafe(artifacts));
log.log('artifacts file saved to disk', artifactsFilename);
fs.writeFileSync(fullPath, stringifySafe(artifacts));
log.log('artifacts file saved to disk', fullPath);
}

/**
* Filter traces and extract screenshots to prepare for saving.
* @param {!Artifacts} artifacts
* @param {!Results} results
* @param {!Audits} audits
* @return {!Promise<!Array<{traceData: !Object, html: string}>>}
*/
function prepareAssets(artifacts, results) {
function prepareAssets(artifacts, audits) {
const passNames = Object.keys(artifacts.traces);
const assets = [];

Expand All @@ -118,10 +117,10 @@ function prepareAssets(artifacts, results) {
return chain.then(_ => artifacts.requestScreenshots(trace))
.then(screenshots => {
const traceData = Object.assign({}, trace);
const html = screenshotDump(screenshots, results);
const html = screenshotDump(screenshots);

if (results && results.audits) {
const evts = new Metrics(traceData.traceEvents, results.audits).generateFakeEvents();
if (audits) {
const evts = new Metrics(traceData.traceEvents, audits).generateFakeEvents();
traceData.traceEvents.push(...evts);
}
assets.push({
Expand All @@ -136,19 +135,21 @@ function prepareAssets(artifacts, results) {
/**
* Writes trace(s) and associated screenshot(s) to disk.
* @param {!Artifacts} artifacts
* @param {!Results} results
* @param {!Audits} audits
* @param {string} pathWithBasename
Copy link
Member

Choose a reason for hiding this comment

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

same thing with jsdoc, pathWithBasename + '.trace.json' and screenshots (though I guess the index thing makes that slightly harder to understand :)

* @return {!Promise}
*/
function saveAssets(artifacts, results) {
return prepareAssets(artifacts, results).then(assets => {
function saveAssets(artifacts, audits, pathWithBasename) {
return prepareAssets(artifacts, audits).then(assets => {
assets.forEach((data, index) => {
const filenamePrefix = getFilenamePrefix(results);
const traceData = data.traceData;
fs.writeFileSync(`${filenamePrefix}-${index}.trace.json`, JSON.stringify(traceData, null, 2));
log.log('trace file saved to disk', filenamePrefix);
const traceFilename = `${pathWithBasename}-${index}.trace.json`;
fs.writeFileSync(traceFilename, JSON.stringify(traceData, null, 2));
log.log('trace file saved to disk', traceFilename);

fs.writeFileSync(`${filenamePrefix}-${index}.screenshots.html`, data.html);
log.log('screenshots saved to disk', filenamePrefix);
const screenshotsFilename = `${pathWithBasename}-${index}.screenshots.html`;
fs.writeFileSync(screenshotsFilename, data.html);
log.log('screenshots saved to disk', screenshotsFilename);
});
});
}
Expand Down
21 changes: 5 additions & 16 deletions lighthouse-core/test/lib/asset-saver-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,10 +46,6 @@ describe('asset-saver helper', () => {
});

it('generates HTML', () => {
const options = {
url: 'https://testexample.com',
generatedTime: '2016-05-31T23:34:30.547Z'
};
const artifacts = {
traces: {
[Audit.DEFAULT_PASS]: {
Expand All @@ -58,19 +54,12 @@ describe('asset-saver helper', () => {
},
requestScreenshots: () => Promise.resolve([]),
};
return assetSaver.prepareAssets(artifacts, options).then(assets => {
return assetSaver.prepareAssets(artifacts).then(assets => {
assert.ok(/<!doctype/gim.test(assets[0].html));
});
});

describe('saves files', function() {
const options = {
url: 'https://testexample.com/',
generatedTime: '2016-05-31T23:34:30.547Z',
flags: {
saveAssets: true
}
};
const artifacts = {
traces: {
[Audit.DEFAULT_PASS]: {
Expand All @@ -80,17 +69,17 @@ describe('asset-saver helper', () => {
requestScreenshots: () => Promise.resolve(screenshotFilmstrip)
};

assetSaver.saveAssets(artifacts, options);
assetSaver.saveAssets(artifacts, dbwResults.audits, process.cwd() + '/the_file');

it('trace file saved to disk with data', () => {
const traceFilename = assetSaver.getFilenamePrefix(options) + '-0.trace.json';
const traceFilename = 'the_file-0.trace.json';
const traceFileContents = fs.readFileSync(traceFilename, 'utf8');
assert.ok(traceFileContents.length > 3000000);
fs.unlinkSync(traceFilename);
});

it('screenshots file saved to disk with data', () => {
const ssFilename = assetSaver.getFilenamePrefix(options) + '-0.screenshots.html';
const ssFilename = 'the_file-0.screenshots.html';
const ssFileContents = fs.readFileSync(ssFilename, 'utf8');
assert.ok(/<!doctype/gim.test(ssFileContents));
assert.ok(ssFileContents.includes('{"timestamp":674089419.919'));
Expand All @@ -108,7 +97,7 @@ describe('asset-saver helper', () => {
requestScreenshots: () => Promise.resolve([]),
};
const beforeCount = countEvents(dbwTrace);
return assetSaver.prepareAssets(mockArtifacts, dbwResults).then(preparedAssets => {
return assetSaver.prepareAssets(mockArtifacts, dbwResults.audits).then(preparedAssets => {
const afterCount = countEvents(preparedAssets[0].traceData);
const metricsSansNavStart = Metrics.metricsDefinitions.length - 1;
assert.equal(afterCount, beforeCount + (2 * metricsSansNavStart), 'unexpected event count');
Expand Down