-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Make Server-Timing tests more resilient #13789
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM % nit
@@ -7,7 +7,8 @@ | |||
<head> | |||
<meta charset='utf-8' /> | |||
<script src="/resources/testharness.js"></script> | |||
<script src='/resources/testharnessreport.js'></script> | |||
<script src="/resources/testharnessreport.js"></script> | |||
<script src="/common/performance-timeline-utils.js"></script> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Including this once seems enough :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whoops! thx - fixed.
|
||
function delayedLoadListener(callback) { | ||
window.addEventListener('load', function() { | ||
// TODO(cvazac) Remove this setTimeout when spec enforces sync entries. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you file a bug for this to track it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per spec, the performance timeline is not guaranteed to have entries for blocking resources during
load
callbacks - which has led to intermittent failures. AddingsetTimeout
of zero should clean those up.