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

Enable css-fonts WPT tests #19930

Merged
merged 1 commit into from Feb 1, 2018
Merged

Enable css-fonts WPT tests #19930

merged 1 commit into from Feb 1, 2018

Conversation

@jonleighton
Copy link
Contributor

jonleighton commented Feb 1, 2018 •
edited by SimonSapin

See #19928 (comment)

Note that some of these tests require you to install the fonts found in
tests/wpt/web-platform-tests/css/fonts.

r? @jdm

This change is Reviewable
@highfive
Copy link

highfive commented Feb 1, 2018

Heads up! This PR modifies the following files:
@jonleighton
Copy link
Contributor Author

jonleighton commented Feb 1, 2018

As discussed in IRC, this will need the relevant fonts to be installed on the builders. Let me know if I can do anything to help on that front.

Also, I can only test this on Linux. I wonder if there might be platform differences? (The original bug I was trying to fix only occurs on Linux...)
@jdm
Copy link
Member

jdm commented Feb 1, 2018

I bet the timeouts are caused by Servo not supporting document.fonts.ready.

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

bors-servo commented Feb 1, 2018

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

bors-servo commented Feb 1, 2018

⌛ Testing commit caae3eb with merge 089fb41...
bors-servo added a commit that referenced this pull request Feb 1, 2018
@bors-servo
Auto merge of #19930 - jonleighton:enable-wpt-css-fonts, 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…
089fb41 
Enable css-fonts WPT tests

See #19928 (comment)

Note that some of these tests require you to install the fonts found in
tests/wpt/web-platform-tests/css/fonts.

r? @jdm

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

bors-servo commented Feb 1, 2018

💔 Test failed - linux-rel-css
@jonleighton
Copy link
Contributor Author

jonleighton commented Feb 1, 2018

Sorry, my branch was not up to date with master (updating it now). That's the cause of the font-feature-settings-serialization-001.html one (which has been added recently). I don't know the cause of the other failure though...
@jdm
Copy link
Member

jdm commented Feb 1, 2018

Yeah, it's strange that that test would not fail for you locally since there are no special fonts involved, just a feature that servo doesn't support yet.
@jonleighton
Enable css-fonts WPT tests
Loading status checks…
f578ec9 
See #19928 (comment)

Note that some of these tests require you to install the fonts found in
tests/wpt/web-platform-tests/css/fonts.
@jonleighton jonleighton force-pushed the jonleighton:enable-wpt-css-fonts branch from caae3eb to f578ec9 Feb 1, 2018
@jonleighton
Copy link
Contributor Author

jonleighton commented Feb 1, 2018

@jdm I think this should merge now. font-variant-ligatures-11.html was passing for me because on my system, all of the special glyphs were rendering as a square "glyph not found" box, and therefore the test matched the ref. I manually added a metadata file to mark it failing.
@jdm
Copy link
Member

jdm commented Feb 1, 2018

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

bors-servo commented Feb 1, 2018

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

bors-servo commented Feb 1, 2018

⌛ Testing commit f578ec9 with merge ac9b1cf...
bors-servo added a commit that referenced this pull request Feb 1, 2018
@bors-servo
Auto merge of #19930 - jonleighton:enable-wpt-css-fonts, 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…
ac9b1cf 
Enable css-fonts WPT tests

See #19928 (comment)

Note that some of these tests require you to install the fonts found in
tests/wpt/web-platform-tests/css/fonts.

r? @jdm

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

bors-servo commented Feb 1, 2018

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: jdm
Pushing ac9b1cf to master...
@bors-servo bors-servo merged commit f578ec9 into servo:master Feb 1, 2018
3 of 4 checks passed
3 of 4 checks passed
Taskcluster (pull_request) TaskGroup: failure
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@jonleighton
Copy link
Contributor Author

jonleighton commented Feb 3, 2018

FWIW the reason we didn't run into missing font issues is due to web-platform-tests/wpt#9374
@jdm
Copy link
Member

jdm commented Feb 3, 2018

Nice find.
@jonleighton jonleighton mentioned this pull request Feb 3, 2018
Merged
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.

None yet

4 participants
@jonleighton @highfive @jdm @bors-servo
You can’t perform that action at this time.