Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upAdd wpt tests for send #9971
Add wpt tests for send #9971
Conversation
|
Review status: 0 of 5 files reviewed at latest revision, 2 unresolved discussions. a discussion (no related file): tests/wpt/mozilla/meta/MANIFEST.json, line 6110 [r1] (raw file): Comments from the review on Reviewable.io |
|
We're not supposed to add in WPT tests, per the discussion in the issue. Instead, we need unit tests, and they're written in Rust. |
|
No, this PR is doing what was requested in the original issue - we're adding servo-specific WPT tests that don't get upstreamed that allow us to test the things we care about without relying on features we don't implement yet. |
|
So what should I do about If licensing is a problem, I can always just make a new |
| "path": "mozilla/send-without-content-type-blob.htm", | ||
| "url": "/_mozilla/mozilla/send-without-content-type-blob.htm" | ||
| } | ||
| ], |
This comment has been minimized.
This comment has been minimized.
KiChjang
Mar 12, 2016
Member
Is this manually typed? Tidy doesn't like the indentation of the brackets.
This comment has been minimized.
This comment has been minimized.
dlrobertson
force-pushed the
dlrobertson:i9357
branch
3 times, most recently
from
7df25c2
to
2109a08
|
Just one small nit - why are the test files named -without-content-type-? |
|
Ah! Sorry, should have been send-without-response-type-*. Let me know if you prefer another name. Review status: 0 of 4 files reviewed at latest revision, 3 unresolved discussions. Comments from the review on Reviewable.io |
|
This still is a misnomer - the response being parsed is a text response. I suggest removing the -without-response-type altogether since this isn't relevant to the test that you have wrote. |
KiChjang
added
S-needs-code-changes
and removed
S-awaiting-answer
S-awaiting-review
labels
|
True, changed it to just |
| }; | ||
|
|
||
| xhr.open("POST", "./resources/content.py", true); | ||
| xhr.responseType = "text"; |
This comment has been minimized.
This comment has been minimized.
KiChjang
Mar 16, 2016
Member
We shouldn't be explicitly setting the responseType here - we should instead assert that the response we read from xhr is a text string.
|
I'm unsure about whether we should include tests for |
Add wpt tests for send that do not set the responseType and compare the result against a string.
Makes sense! I removed the |
|
I'm going to go ahead and merge this. Filed web-platform-tests/wpt#2709 for the missing test coverage. Thanks for working on this! @bors-servo r+ |
|
|
Add wpt tests for send Add wpt tests for send that do not set the responseType and compare the result against a string. Fixes #9357 r? @KiChjang <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9971) <!-- Reviewable:end -->
|
|
|
@bors-servo retry |
Add wpt tests for send Add wpt tests for send that do not set the responseType and compare the result against a string. Fixes #9357 r? @KiChjang <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9971) <!-- Reviewable:end -->
|
|
dlrobertson commentedMar 11, 2016
Add wpt tests for send that do not set the responseType and compare the result against a string.
Fixes #9357
r? @KiChjang
This change is