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

Remove forked referrer policy tests #22786

Merged
merged 1 commit into from Feb 4, 2019
Merged

Remove forked referrer policy tests #22786

merged 1 commit into from Feb 4, 2019

Conversation

@georgeroman
Copy link
Contributor

georgeroman commented Jan 30, 2019 •
edited by SimonSapin

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #22663

This change is Reviewable
@highfive
Copy link

highfive commented Jan 30, 2019

Heads up! This PR modifies the following files:
@jdm
Copy link
Member

jdm commented Jan 31, 2019

Ok, first of all thank you for doing this work! I've spent some time looking at the changes, and I have a few comments:

  • we appear to be overwriting some of the intentional changes that were made when forking from upstream
  • we are losing some test coverage that was added in our forked version (such as link elements)

That being said, I've been looking at the history of changes to our forked copy (https://github.com/servo/servo/commits/master/tests/wpt/mozilla/tests/mozilla/referrer-policy), and I don't think there is a good reason to try to maintain it any longer. We have implemented missing features that were preventing us from running the upstream tests, and the upstream tests have added tests coverage for some of the features that we added in ours. I've filed web-platform-tests/wpt#15189 to track adding coverage upstream for link elements; given the current state, I think it will be more productive to simply remove our forked tests completely instead of keep maintaining them.
@jdm jdm assigned jdm and unassigned SimonSapin Jan 31, 2019
@georgeroman georgeroman changed the title Update forked referrer policy tests Remove forked referrer policy tests Feb 1, 2019
@georgeroman
Copy link
Contributor Author

georgeroman commented Feb 1, 2019

Thanks for the info! I modified the PR accordingly.
@jdm
jdm approved these changes Feb 1, 2019
View changes
Copy link
Member

jdm left a comment

You will need to run ./mach update-manifest as well.
@georgeroman
Remove forked referrer policy tests
Loading status checks…
e6b363d
@jdm
Copy link
Member

jdm commented Feb 4, 2019

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

bors-servo commented Feb 4, 2019

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

bors-servo commented Feb 4, 2019

⌛ Testing commit e6b363d with merge 871fe93...
bors-servo added a commit that referenced this pull request Feb 4, 2019
@bors-servo
Auto merge of #22786 - georgeroman:update_forked_referrer_policy_test…
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…
871fe93 
…s, r=jdm

Remove forked referrer policy tests

<!-- 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 #22663

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

bors-servo commented Feb 4, 2019

💔 Test failed - mac-rel-wpt4
@jdm
Copy link
Member

jdm commented Feb 4, 2019

@bors-servo retry
@bors-servo
Copy link
Contributor

bors-servo commented Feb 4, 2019

⚡ Previous build results for android-mac, arm32, arm64, linux-rel-css, linux-rel-wpt, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, magicleap, status-taskcluster are reusable. Rebuilding only mac-rel-wpt4...
@bors-servo
Copy link
Contributor

bors-servo commented Feb 4, 2019

☀️ Test successful - android-mac, arm32, arm64, linux-rel-css, linux-rel-wpt, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, magicleap, status-taskcluster
Approved by: jdm
Pushing 871fe93 to master...
@bors-servo bors-servo merged commit e6b363d into servo:master Feb 4, 2019
4 checks passed
4 checks passed
Taskcluster (pull_request) TaskGroup: success
Details
Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
@bors-servo bors-servo mentioned this pull request Feb 4, 2019
Merged
0 of 5 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jdm jdm

Assignees

@jdm jdm

Labels
S-tests-failed
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

Port referrer-policy test structure changes to forked tests
5 participants
@georgeroman @highfive @jdm @bors-servo @SimonSapin
You can’t perform that action at this time.