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.

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

misc: update sample_v2 artifacts #8243

Merged
merged 1 commit into from
Apr 14, 2019
Merged

misc: update sample_v2 artifacts #8243

merged 1 commit into from
Apr 14, 2019

Conversation

brendankenny
Copy link
Member

I started writing a test that needed the settings on the artifacts that generate sample_v2.json and I noticed they weren't there. Did a little housecleaning on the easy to update artifacts (deleting EventListeners and WebSQL, finally adding RobotsTxt and TapTargets), though at some point (maybe for v5) we should still do a whole-hog update of the artifacts, trace, and devtools log.

At that point, surprisingly sample_v2.json still wasn't a valid LH.Result, which isn't super important but is kind of annoying. To fix we just had to

  • not delete lhr.configSettings.auditMode in the cleanup script (just hardcoded to true to sidestep the travis issue mentioned in the comment in that file) and
  • Artifacts.MeasureEntry (which makes up the Timing artifact) needed to not inherit from PerformanceEntry. PerformanceEntry has a toJSON method, but when we save artifacts as JSON methods aren't preserved, so the resulting object is no longer a PerformanceEntry. I just copied the properties over to the type definition rather than inherit now.

// 1) The string|boolean story for proto
// 2) Travis gets a absolute path during yarn diff:sample-json
delete lhr.configSettings.auditMode;
lhr.configSettings.auditMode = true;
Copy link
Member Author

Choose a reason for hiding this comment

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

this is valid, it's just as if we were running audit mode from the default location instead of from the saved artifacts

return !item.explanation.includes('Required RobotsTxt gatherer did not run') &&
!item.explanation.includes('Required TapTargets gatherer did not run');
});
assert.deepStrictEqual(unexpectedErrrors, [], 'Audit errors found within the report');
Copy link
Member Author

Choose a reason for hiding this comment

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

no longer necessary :)

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.

Much needed. Thanks!

@paulirish paulirish merged commit ca650db into master Apr 14, 2019
@paulirish paulirish deleted the samplejson branch April 14, 2019 23:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants