-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
review some initial tests for resource timing #4266
review some initial tests for resource timing #4266
Conversation
Notifying @haoxli, @igrigorik, @plehegar, @toddreifsteck, @yoavweiss, and @zqzhang. (Learn how reviewing works.) |
</head> | ||
<body> | ||
<h1>Description</h1> | ||
<p>This test validates that all of the initiator types are represented even when dynamically inserted.</p> |
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.
Nitpick: drop "all of the"? I don't think we want to enumerate all of the element types here, and implying all and testing a subset is slightly confusing.
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.
done!
|
||
test_equals(entries.length, 2, 'There should be 2 PerformanceEntries'); | ||
|
||
var entry = entries[1]; |
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.
Might be worth adding a comment above, explaining that we're counting on second request of same resource to reuse the socket..
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.
Yeah. Please look at commit #b1bbb10
Yikes, looks like we introduced some confusion here.
The end result, as of right now, is that L2 states that failed resources MUST be surfaced, but doesn't have the processing section updated.
I believe the reason we went with (i) at TPAC is that it would make existing L2 implementations (e.g. IE, which is not going to change?) incompatible... Which hints that we should stick with that. @toddreifsteck @plehegar wdyt? |
For L2, I'd rather use (i) since doing (ii) would probably require hooking ourselves into Fetch, which is for L3. |
I agree that we should go with (i) for L2 and push any further specification to L3. |
Shall we drop the test case for L1 (resource_ignore_failures_level1.html) in that case? I'm not sure if a test case for a "MAY" will make sense, knowing that it produces different test results among the major browsers. |
@siusin imho, we should drop it. Until we better define what it means in L3, it might be harmful to have a test related to resource failures in the test suite. |
+1 to removing those tests. Also, opened w3c/resource-timing#85 for spec update, ptal. @siusin other then removing the test, everything else looks good to me! |
Ok, thanks! |
LGTM |
review tests from pull request #402