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

Initial test for resource timing #402

Conversation

plehegar
Copy link
Member

@plehegar plehegar commented Nov 9, 2013

This is just an initial test to adjust all of the tests before moving them all.

@hoppipolla-critic-bot
Copy link

Critic review: https://critic.hoppipolla.co.uk/r/393

This is an external review system which you may optionally use for the code review of your pull request.

@plehegar
Copy link
Member Author

plehegar commented Nov 9, 2013

test_resource_cached.html is generated inconsistent results (could be a Google canary error?)

@sideshowbarker
Copy link
Member

@plehegar there's been no action on this for 6 months, and you have review comments waiting over at https://critic.hoppipolla.co.uk/r/393

@sideshowbarker
Copy link
Member

@plehegar radio check

@toddreifsteck
Copy link
Contributor

@igrigorik @plehegar This PR needs some love. What are the next steps? This may be one where some assistance from Xiaquan

@plehegar
Copy link
Member Author

@siusin looked at this PR and concluded that it overlaps with what we have in the repo. Proposal is to close down the PR with no action. @igrigorik ?

@igrigorik
Copy link
Member

sgtm, closing.

@igrigorik igrigorik closed this Nov 15, 2016
@toddreifsteck toddreifsteck reopened this Nov 15, 2016
@toddreifsteck
Copy link
Contributor

These tests clearly have additional coverage missing from existing tests. They cover clearing the buffer which is something the existing tests don't cover. There is some overlap, but I'm not clear that is a good reason to shut this PR down.

@siusin
Copy link
Contributor

siusin commented Nov 17, 2016

Sorry, I was looking at a wrong branch when I did the review last time. My WPT repo got messed up after I merged these commits to the latest :(

@toddreifsteck is correct. I think it's reasonable to keep the following tests, although some of them need to be modified:

  • test_resource_cached.html
  • test_resource_iframe_self_navigation.html
  • test_resource_ignore_data_url.html
  • test_resource_ignore_failures.html
  • test_resource_script_types.html
  • test_resource_reparenting.html
  • test_resource_redirects.html
  • test_resource_connection_reuse.html
  • test_resource_frame_initiator_type.html
  • test_resource_dynamic_insertion.html

These two tests can be removed for overlaps:

  • test_resource_attribute_order.html (covered by test_resource_timing.html);
  • test_resource_initiator_types.html (covered by resource_timing.html)

I'll submit the updated tests later.

@plehegar
Copy link
Member Author

(goal is to close this PR and open one or more new ones with the proper test)

@siusin
Copy link
Contributor

siusin commented Nov 30, 2016

Updated as pull request #4266 :

  • test_resource_cached.html, updated as resource_cached.htm, to use python instead of php;
  • test_resource_iframe_self_navigation.html, reported inconsistent results but failed to find relevant detail in the spec.
  • test_resource_ignore_data_url.html is covered by resource-timing.html.
  • test_resource_ignore_failures.html, divided as resource_ignore_failures_level1 and resource_failures.
  • test_resource_script_types.html, ignored as edge cases.
  • test_resource_reparenting.html, couldn't find relevant detail in the spec.
  • test_resource_redirects.html is covered by resource-timing.html.
  • test_resource_connection_reuse.html, updated as resource_connection_reuse.html, to use python instead of php;
  • test_resource_frame_initiator_type.html is covered by resource-timing.html.
  • test_resource_dynamic_insertion.html, updated as resource_dynamic_insertion.html.

@igrigorik igrigorik closed this Dec 14, 2016
@plehegar
Copy link
Member Author

plehegar commented Dec 14, 2016

subsumed by other PRs

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.

6 participants