Basic Refresh header tests - #6606
Merged
Merged
Conversation
Collaborator
|
There are no owners for this pull request. Please reach out on W3C's irc server (irc.w3.org, port 6665) on channel #testing (web client) to get help with this. Thank you! |
Firefox (nightly)Testing web-platform-tests at revision da24bb7 All results4 tests ran/html/semantics/document-metadata/the-meta-element/pragma-directives/attr-meta-http-equiv-refresh/parsing.html
/http/refresh/navigate.window.html
/http/refresh/subresource.any.html
/http/refresh/subresource.any.worker.html
|
Sauce (safari)Testing web-platform-tests at revision da24bb7 Unstable results
All results4 tests ran/html/semantics/document-metadata/the-meta-element/pragma-directives/attr-meta-http-equiv-refresh/parsing.html
/http/refresh/navigate.window.html
/http/refresh/subresource.any.html
/http/refresh/subresource.any.worker.html
|
Chrome (unstable)Testing web-platform-tests at revision da24bb7 |
Sauce (MicrosoftEdge)Testing web-platform-tests at revision da24bb7 Unstable results
All results4 tests ran/html/semantics/document-metadata/the-meta-element/pragma-directives/attr-meta-http-equiv-refresh/parsing.html
/http/refresh/navigate.window.html
/http/refresh/subresource.any.html
/http/refresh/subresource.any.worker.html
|
Member
Author
|
The redness is more due to the Safari bot than this test, I think. |
domenic
reviewed
Aug 8, 2017
domenic
left a comment
domenic
left a comment
Member
There was a problem hiding this comment.
Generally looks good, but I think some tweaks would help a decent bit.
| if (test_obj.expected.length === 0) { | ||
| assert_equals(got, filename); | ||
| if (loadCount === 1) { | ||
| setTimeout(function() { |
Member
There was a problem hiding this comment.
I think these days the lint will yell at you and require step_timeout
Member
Author
There was a problem hiding this comment.
It seems lint.whitelist excludes all of html/ but I'll fix this.
| assert_equals(got, test_obj.expected[1]); | ||
| t.done(); | ||
| if (loadCount === 2) { | ||
| if(type === "Refresh header" && test_obj.expected[1] === "refresh.sub.html") { |
Member
There was a problem hiding this comment.
It might be better to use a sentinel like __filename__ and then do a replacement immediately inside the outer loop. Right now it privileges <meta> tests by choosing their filename as canonical, which kind of ruins the symmetry.
| frame.src = "resources/refresh.py" | ||
| frame.onload = t.step_func(() => { | ||
| // Could be better by verifying that resources/refresh.py loads too | ||
| if(frame.contentWindow.location.href === (new URL("resources/refreshed.txt?ÿ", self.location)).href) { |
Member
There was a problem hiding this comment.
Is this supposed to be testing the decoding? If so, a comment or test name string would be good. Also it might be good to verify that the bytes actually make it back through Python correctly. Although I think given Python's very naive Unicode handling that's probably true.
Member
Author
There was a problem hiding this comment.
Yeah, 0xFF becomes the U+00FF code point. I should probably tweak a bit more.
| @@ -0,0 +1,4 @@ | |||
| def main(request, response): | |||
| response.headers.set("Content-Type", "text/plain") | |||
| response.headers.set("Refresh", "0;./refreshed.txt?\xFF") | |||
Member
There was a problem hiding this comment.
Again a comment about what this \xFF is doing would be helpful.
Member
Author
|
You're okay with this also being in http/refresh? I'll add a README there for clarity pointing to the other tests. |
Member
Author
|
Failing is still happening due to Safari. |
domenic
approved these changes
Aug 9, 2017
domenic
left a comment
domenic
left a comment
Member
There was a problem hiding this comment.
LGTM. And yeah, having some in http/ makes sense, especially with the readme redirect.
Member
Author
|
I would like this merged with the commit message: |
annevk
added a commit
to whatwg/html
that referenced
this pull request
Aug 9, 2017
Fixes https://www.w3.org/Bugs/Public/show_bug.cgi?id=28339 and fixes https://www.w3.org/Bugs/Public/show_bug.cgi?id=28563. Tests: web-platform-tests/wpt#6606. Follow-up: #2900.
alice
pushed a commit
to alice/html
that referenced
this pull request
Jan 8, 2019
Fixes https://www.w3.org/Bugs/Public/show_bug.cgi?id=28339 and fixes https://www.w3.org/Bugs/Public/show_bug.cgi?id=28563. Tests: web-platform-tests/wpt#6606. Follow-up: whatwg#2900.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See https://www.w3.org/Bugs/Public/show_bug.cgi?id=28339.
Arguably this needs a lot more to be comprehensive. Such as parsing tests, sandboxing tests, etc.