Skip to content

Conversation

connorjclark
Copy link
Collaborator

@connorjclark connorjclark commented Mar 27, 2020

  • --filter was broken from the last change
  • progress bar didn't use the correct count
  • allow collection to be resumed
  • organize LHRs by url directory, instead of all in root directory. Simplifies programmatic consumption

@connorjclark connorjclark changed the title scripts(compare-runs): fix filter, allow for resume, reorganize output misc(compare-runs): fix filter, allow for resume, reorganize output Mar 27, 2020
@@ -117,25 +117,32 @@ function round(value) {
* @return {string}
*/
function getProgressBar(i, total = argv.n * argv.urls.length) {
return new Array(Math.round(i * 40 / total)).fill('▄').join('').padEnd(40);
const bars = new Array(Math.round(i * 40 / total)).fill('▄').join('').padEnd(40);
return `${i} / ${total} [${bars}]`;
Copy link
Member

Choose a reason for hiding this comment

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

the math here is off. i is unique per URL and you want i+1

here's a good repro:

node lighthouse-core/scripts/compare-runs.js --name my-collectiondeleteme --gather -n 2 --urls https://www.example.com http://example.com

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 forgot to push this exact fix :(

}

if (includeFilter && !includeFilter.test(result.key)) {
if (includeFilter && !includeFilter.test(result.name)) {
Copy link
Member

Choose a reason for hiding this comment

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

keeping it on key meant you can do --filter=metric (which is documented and i really like)

we could use something else for metric/timing filtering vs this regex on name tho.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ah, just had to move this filter check before the property delete :)

Copy link
Member

@paulirish paulirish left a comment

Choose a reason for hiding this comment

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

resumable is greattttttt

nice stuff

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants