Skip to content
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

Add test for float kerning, make disabled #11483

Closed
wants to merge 1 commit into from

Conversation

@mitchhentges
Copy link
Contributor

mitchhentges commented May 28, 2016

Make automated test for #11482

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

This change is Reviewable

],
"url": "/_mozilla/css/float_kerning_a.html"
}
],

This comment has been minimized.

Copy link
@mitchhentges

mitchhentges May 28, 2016

Author Contributor

There's a .items.reftest section and a .reftest_nodes section in MANIFEST.json. I'm not sure the pattern for where the test definition should go, so I put it in .items.reftest, and it's properly picked up by ./mach test-wpt, so I think it's happy

This comment has been minimized.

Copy link
@mitchhentges

mitchhentges May 29, 2016

Author Contributor

Yep, it needed to be added to both sections, now CI doesn't fail

@mitchhentges mitchhentges force-pushed the mitchhentges:11482-kerning-test branch from 2246650 to c8aff51 May 28, 2016
@highfive
Copy link

highfive commented May 28, 2016

New code was committed to pull request.

@mitchhentges
Copy link
Contributor Author

mitchhentges commented May 29, 2016

Am I not allowed to change the manifest?

$ bash etc/ci/manifest_changed.sh
diff --git a/tests/wpt/mozilla/meta/MANIFEST.json b/tests/wpt/mozilla/meta/MANIFEST.json
index c617a0f..42570f8 100644
--- a/tests/wpt/mozilla/meta/MANIFEST.json
+++ b/tests/wpt/mozilla/meta/MANIFEST.json
@@ -8516,6 +8516,18 @@
           "url": "/_mozilla/css/float_intrinsic_width_a.html"
         }
       ],
+      "css/float_kerning_a.html": [
+        {
+          "path": "css/float_kerning_a.html",
+          "references": [
+            [
+              "/_mozilla/css/float_kerning_ref.html",
+              "=="
+            ]
+          ],
+          "url": "/_mozilla/css/float_kerning_a.html"
+        }
+      ],
       "css/float_overflow_area_a.html": [
         {
           "path": "css/float_overflow_area_a.html",
The command "bash etc/ci/manifest_changed.sh" exited with 1.
@KiChjang
Copy link
Member

KiChjang commented May 29, 2016

Have you tried using ./mach create-wpt to create tests?

@mitchhentges mitchhentges force-pushed the mitchhentges:11482-kerning-test branch from c8aff51 to 40ab16d May 29, 2016
@highfive
Copy link

highfive commented May 29, 2016

New code was committed to pull request.

@mitchhentges
Copy link
Contributor Author

mitchhentges commented May 29, 2016

I re-did this according to ./mach create-wpt, but MANIFEST.json is still changed. However, now two places in the manifest have been updated. Maybe it will be happy now?

@KiChjang
Copy link
Member

KiChjang commented May 29, 2016

Yep, Travis CI doesn't complain now.

@bors-servo try

@bors-servo
Copy link
Contributor

bors-servo commented May 29, 2016

Trying commit 40ab16d with merge 8de46f3...

bors-servo added a commit that referenced this pull request May 29, 2016
Add test for float kerning, make disabled

<!-- Please describe your changes on the following line: -->
Make automated test for #11482
---
<!-- 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 add a test for #11482

<!-- 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="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11483)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented May 29, 2016

@Ms2ger
Copy link
Contributor

Ms2ger commented May 29, 2016

I'm not sure I see the point of this PR: you're adding a test, but not running it? Did you mean to mark it as failing instead?

@mitchhentges
Copy link
Contributor Author

mitchhentges commented May 29, 2016

Ah, yes, I want to add it as failing. Still figuring out how the wpt test configuration happens :)

@mitchhentges mitchhentges force-pushed the mitchhentges:11482-kerning-test branch from 40ab16d to 8765fd5 May 30, 2016
@highfive
Copy link

highfive commented May 30, 2016

New code was committed to pull request.

@Ms2ger
Copy link
Contributor

Ms2ger commented May 30, 2016

Looks good in terms of metadata; r? @mbrubeck for the test

@highfive highfive assigned mbrubeck and unassigned jdm May 30, 2016
@mbrubeck
Copy link
Contributor

mbrubeck commented May 31, 2016

Something is wrong; this test is not passing in Gecko or Blink. In both cases the horizontal position of the .float element in the test case is different than the position of the .fixed element in the reference. In Gecko the green/red/orange backgrounds are also showing; I think adding an explicit line-height: 1; fixes this.

I'm also not sure it will be possible to reproduce this bug using Ahem, because I don't think it has any kerning tables. :( This may depend on something like #8374 so we can use a known font other than Ahem.

@mitchhentges
Copy link
Contributor Author

mitchhentges commented Jun 1, 2016

Ah, yes, I think I got dinged by ./mach run not using Ahem and falling back to some other font, and for some reason I thought that the other font was Ahem. Oops for the confusion, I'll wait until more cross-platform servo fonts exist that are available in both ./mach run and ./mach test

@mitchhentges mitchhentges deleted the mitchhentges:11482-kerning-test branch Jun 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.