Skip to content

/ servo

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

Fix loss of response type information in Fetch API #14868

Merged
merged 1 commit into from Jan 10, 2017
Merged

Fix loss of response type information in Fetch API #14868

merged 1 commit into from Jan 10, 2017

Conversation

@bd339
Copy link
Contributor

bd339 commented Jan 5, 2017 •
edited

Avoids mapping response types that are distinct according to the spec to fewer response types. Also updates test expectations to match that we now pass tests that check the response type.

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #14068
  • There are tests for these changes

This change is Reviewable
@highfive
Copy link

highfive commented Jan 5, 2017

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @asajeffrey (or someone else) soon.
@highfive
Copy link

highfive commented Jan 5, 2017

Heads up! This PR modifies the following files:

  • @fitzgen: components/script/dom/xmlhttprequest.rs, components/script/fetch.rs
  • @KiChjang: components/script/dom/xmlhttprequest.rs, components/net_traits/lib.rs, components/net_traits/lib.rs, components/script/fetch.rs, components/net_traits/response.rs, components/net_traits/response.rs
@bd339 bd339 force-pushed the bd339:iss14068 branch 2 times, most recently from 2a6d2de to 98be21a Jan 5, 2017
@bd339
Copy link
Contributor Author

bd339 commented Jan 7, 2017

I wonder why the Travis build failed. I can do ./mach test-stylo without errors locally.
@bd339
Copy link
Contributor Author

bd339 commented Jan 7, 2017

r?@jdm
@highfive highfive assigned jdm and unassigned asajeffrey Jan 7, 2017
@jdm
Copy link
Member

jdm commented Jan 7, 2017

That's a known failure tracked in #14723.
@jdm
Copy link
Member

jdm commented Jan 8, 2017

@bors-servo: r+
Great work! Glad to see so many tests passing.
@bors-servo
Copy link
Contributor

bors-servo commented Jan 8, 2017

📌 Commit 98be21a has been approved by jdm
@bors-servo
Copy link
Contributor

bors-servo commented Jan 8, 2017

⌛ Testing commit 98be21a with merge 4b3faf5...
bors-servo added a commit that referenced this pull request Jan 8, 2017
@bors-servo
Auto merge of #14868 - bd339:iss14068, r=jdm
Loading status checks…
4b3faf5 
Fix loss of response type information in Fetch API

<!-- Please describe your changes on the following line: -->
Avoids mapping response types that are distinct according to [the spec](https://fetch.spec.whatwg.org/#concept-response-type) to fewer response types. Also updates test expectations to match that we now pass tests that check the response type.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #14068

<!-- Either: -->
- [X] There are tests for these changes

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14868)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jan 8, 2017

💔 Test failed - mac-rel-wpt1
@bd339
Copy link
Contributor Author

bd339 commented Jan 8, 2017

Are these intermittent or test expectations in need of an update (that I for some reason couldn't see on my Linux build)?
@KiChjang
Copy link
Member

KiChjang commented Jan 9, 2017 

Ran 3420 tests finished in 797.0 seconds.
  • 3418 ran as expected. 396 tests skipped.
  • 2 tests had unexpected subtest results

Tests with unexpected results:
  ▶ Unexpected subtest result in /fetch/api/basic/request-headers-worker.html:
  └ PASS [expected FAIL] Fetch with GET

  ▶ Unexpected subtest result in /fetch/api/basic/request-headers-worker.html:
  └ PASS [expected FAIL] Fetch with HEAD

  ▶ Unexpected subtest result in /fetch/api/basic/request-headers-worker.html:
  └ PASS [expected FAIL] Fetch with GET and mode "cors" does not need an Origin header

  ▶ Unexpected subtest result in /fetch/api/basic/request-headers.html:
  └ PASS [expected FAIL] Fetch with GET

  ▶ Unexpected subtest result in /fetch/api/basic/request-headers.html:
  └ PASS [expected FAIL] Fetch with HEAD

  ▶ Unexpected subtest result in /fetch/api/basic/request-headers.html:
  └ PASS [expected FAIL] Fetch with GET and mode "cors" does not need an Origin header
@KiChjang
Copy link
Member

KiChjang commented Jan 9, 2017

It looks like you do need to update more test expectations.
@jdm
Copy link
Member

jdm commented Jan 9, 2017

Could you squash the commits into a single one without any merge commits?
@jdm jdm added S-needs-squash and removed S-awaiting-review labels Jan 9, 2017
bd339
Don't lose information in Fetch API
Loading status checks…
0f1eb13 
Also update test expectations.
@bd339 bd339 force-pushed the bd339:iss14068 branch from 946fa04 to 0f1eb13 Jan 9, 2017
@KiChjang
Copy link
Member

KiChjang commented Jan 9, 2017

@bors-servo r=jdm
@bors-servo
Copy link
Contributor

bors-servo commented Jan 9, 2017

📌 Commit 0f1eb13 has been approved by jdm
@bors-servo
Copy link
Contributor

bors-servo commented Jan 10, 2017

⌛ Testing commit 0f1eb13 with merge f1c82be...
bors-servo added a commit that referenced this pull request Jan 10, 2017
@bors-servo
Auto merge of #14868 - bd339:iss14068, r=jdm
Loading status checks…
f1c82be 
Fix loss of response type information in Fetch API

<!-- Please describe your changes on the following line: -->
Avoids mapping response types that are distinct according to [the spec](https://fetch.spec.whatwg.org/#concept-response-type) to fewer response types. Also updates test expectations to match that we now pass tests that check the response type.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #14068

<!-- Either: -->
- [X] There are tests for these changes

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14868)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jan 10, 2017

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-gnu-dev, windows-msvc-dev
@bors-servo bors-servo merged commit 0f1eb13 into servo:master Jan 10, 2017
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@jdm jdm

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

Make FetchMetadata reflect all possible response types
6 participants
@bd339 @highfive @jdm @bors-servo @KiChjang @asajeffrey
You can’t perform that action at this time.