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

XMLHttpRequest.responseXML returns null when a network error occurs #24342

Merged
merged 2 commits into from Oct 2, 2019
Merged

XMLHttpRequest.responseXML returns null when a network error occurs #24342

merged 2 commits into from Oct 2, 2019
+6 −38

Conversation

@petosorus
Copy link
Contributor

petosorus commented Oct 1, 2019 •
edited by SimonSapin

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #24285 (GitHub issue number if applicable)
  • There are tests for these changes OR
  • These changes do not require tests because tests ___

This change is Reviewable
@petosorus
XMLHttpRequest.responseXML returns null when a network error occurs
Loading status checks…
f089d16
@highfive
Copy link

highfive commented Oct 1, 2019

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

highfive commented Oct 1, 2019

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/xmlhttprequest.rs
  • @KiChjang: components/script/dom/xmlhttprequest.rs
@highfive
Copy link

highfive commented Oct 1, 2019

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
@jdm
Copy link
Member

jdm commented Oct 1, 2019

@bors-servo try=wpt
@bors-servo
Copy link
Contributor

bors-servo commented Oct 1, 2019

⌛ Trying commit f089d16 with merge fba4c13...
bors-servo added a commit that referenced this pull request Oct 1, 2019
@bors-servo
Auto merge of #24342 - petosorus:null_responseXml, r=<try>
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
fba4c13 
XMLHttpRequest.responseXML returns null when a network error occurs

<!-- Please describe your changes on the following line: -->

---
<!-- 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 #24285 (GitHub issue number if applicable)

<!-- Either: -->
- [x] There are tests for these changes OR
- [ ] These changes do not require tests because tests ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- 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/24342)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Oct 1, 2019

💔 Test failed - linux-rel-wpt
@jdm
Copy link
Member

jdm commented Oct 1, 2019

Look at all those passing tests! Please update the ini files for them to remove the expected failures that now pass.
@highfive highfive removed the S-tests-failed label Oct 2, 2019
@petosorus
Copy link
Contributor Author

petosorus commented Oct 2, 2019

@bors-servo try=wpt
@bors-servo
Copy link
Contributor

bors-servo commented Oct 2, 2019

@petosorus: 🔑 Insufficient privileges: not in try users
@petosorus
Copy link
Contributor Author

petosorus commented Oct 2, 2019

Those fail expectations are pretty neat (I'm also trying out the tooling :) )
@CYBAI
Copy link
Collaborator

CYBAI commented Oct 2, 2019

@bors-servo try=wpt
@bors-servo
Copy link
Contributor

bors-servo commented Oct 2, 2019

⌛ Trying commit 366e403 with merge 8cb6b89...
bors-servo added a commit that referenced this pull request Oct 2, 2019
@bors-servo
Auto merge of #24342 - petosorus:null_responseXml, r=<try>
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
8cb6b89 
XMLHttpRequest.responseXML returns null when a network error occurs

<!-- Please describe your changes on the following line: -->

---
<!-- 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 #24285 (GitHub issue number if applicable)

<!-- Either: -->
- [x] There are tests for these changes OR
- [ ] These changes do not require tests because tests ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- 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/24342)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Oct 2, 2019

💔 Test failed - linux-rel-wpt
@petosorus
Copy link
Contributor Author

petosorus commented Oct 2, 2019

From what I read, the modification I made has made Redirection tests pass.
I'm not really sure if it should, but I don't know enough. Reading the spec, the check is on the response body, not the response status.
Can we safely assume an error response status implies a null response body?
@nox nox assigned jdm and unassigned nox Oct 2, 2019
@jdm
Copy link
Member

jdm commented Oct 2, 2019

Yes, the specification states that error responses have a null body.
@petosorus
Copy link
Contributor Author

petosorus commented Oct 2, 2019

Nice. So, I'll be removing the relevant redirect tests.
If I look at this log https://build.servo.org/builders/linux-rel-wpt/builds/13631/steps/shell__4/logs/intermittents.log, there are a lot of apparently css tests that do not fail anymore, or do fail again. I'm not sure they are related to me. What do you think of these?
@jdm
Copy link
Member

jdm commented Oct 2, 2019

intermittents.log is a list of tests that had unexpected results but are known to do so intermittently (based on issues in our issue tracker that have the I-intermittent label applied). You should ignore them.
@petosorus petosorus force-pushed the petosorus:null_responseXml branch from 366e403 to 63a1735 Oct 2, 2019
@highfive highfive removed the S-tests-failed label Oct 2, 2019
@jdm
Copy link
Member

jdm commented Oct 2, 2019

@bors-servo r+
Thanks!
@bors-servo
Copy link
Contributor

bors-servo commented Oct 2, 2019

⌛ Testing commit 63a1735 with merge 1a13040...
bors-servo added a commit that referenced this pull request Oct 2, 2019
@bors-servo
Auto merge of #24342 - petosorus:null_responseXml, r=jdm
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
1a13040 
XMLHttpRequest.responseXML returns null when a network error occurs

<!-- Please describe your changes on the following line: -->

---
<!-- 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 #24285 (GitHub issue number if applicable)

<!-- Either: -->
- [x] There are tests for these changes OR
- [ ] These changes do not require tests because tests ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- 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/24342)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Oct 2, 2019

💔 Test failed - linux-rel-wpt
@jdm
Copy link
Member

jdm commented Oct 2, 2019

Looks like we missed one:

{
    "status": "PASS", 
    "group": "default", 
    "message": null, 
    "stack": null, 
    "subtest": "XMLHttpRequest: abort() after send()", 
    "test": "/xhr/abort-after-send.any.html", 
    "line": 282809, 
    "action": "test_result", 
    "expected": "FAIL"
}
Thomas Graviou @petosorus
remove unneeded fail expectations
Loading status checks…
bb26527
@petosorus petosorus force-pushed the petosorus:null_responseXml branch from 63a1735 to bb26527 Oct 2, 2019
@jdm
Copy link
Member

jdm commented Oct 2, 2019

@bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented Oct 2, 2019

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

bors-servo commented Oct 2, 2019

⌛ Testing commit bb26527 with merge 8c809f1...
bors-servo added a commit that referenced this pull request Oct 2, 2019
@bors-servo
Auto merge of #24342 - petosorus:null_responseXml, r=jdm
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
8c809f1 
XMLHttpRequest.responseXML returns null when a network error occurs

<!-- Please describe your changes on the following line: -->

---
<!-- 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 #24285 (GitHub issue number if applicable)

<!-- Either: -->
- [x] There are tests for these changes OR
- [ ] These changes do not require tests because tests ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- 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/24342)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Oct 2, 2019

💔 Test failed - linux-rel-wpt
@jdm
Copy link
Member

jdm commented Oct 2, 2019

@bors-servo retry
bors-servo added a commit that referenced this pull request Oct 2, 2019
@bors-servo
Auto merge of #24342 - petosorus:null_responseXml, r=jdm
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
d1b16af 
XMLHttpRequest.responseXML returns null when a network error occurs

<!-- Please describe your changes on the following line: -->

---
<!-- 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 #24285 (GitHub issue number if applicable)

<!-- Either: -->
- [x] There are tests for these changes OR
- [ ] These changes do not require tests because tests ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- 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/24342)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Oct 2, 2019

⌛ Testing commit bb26527 with merge d1b16af...
@bors-servo
Copy link
Contributor

bors-servo commented Oct 2, 2019

☀️ Test successful - linux-rel-css, linux-rel-wpt, status-taskcluster
Approved by: jdm
Pushing d1b16af to master...
@bors-servo bors-servo merged commit bb26527 into servo:master Oct 2, 2019
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
Taskcluster (pull_request) TaskGroup: success
Details
homu Test successful
Details
@petosorus petosorus deleted the petosorus:null_responseXml branch Oct 3, 2019
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.

XMLHttpRequest.responseXML should return null when a network error occurs
6 participants
@petosorus @highfive @jdm @bors-servo @CYBAI @nox
You can’t perform that action at this time.