Test generation for Servo_* bindings in `components/style/gecko_bindings/bindings.rs` #13603

Rafagd · 2016-10-05T18:59:04Z

Added a python script that takes all Servo_* bindings generated in components/style/gecko_bindings/bindings.rs and generates a new test file that checks if they all have the same signature as the bindings on ports/geckolib/glue.rs.

Issue asks for both files to be created at the same folder components/style/gecko_bindings/bindings.rs is located.

#13598

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #13598 (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because they are tests.

This change is


        Script that creates a test for Servo_* bindings

highfive · 2016-10-05T18:59:10Z

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

highfive · 2016-10-05T18:59:11Z

Heads up! This PR modifies the following files:

@bholley: components/style/gecko_bindings/test_bindings.rs, components/style/gecko_bindings/check_bindings.py

highfive · 2016-10-05T18:59:12Z

Warning

These commits modify style code, but no tests are modified. Please consider adding a test!

emilio · 2016-10-05T19:06:29Z

r? @wafflespeanut for the python bits.

aneeshusa · 2016-10-05T19:06:58Z

components/style/gecko_bindings/check_bindings.py

+tests.write("}\n")
+
+bindings.close()
+tests.close()


Please use with instead of manually calling close().

emilio · 2016-10-05T19:08:01Z

components/style/gecko_bindings/test_bindings.rs

+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+


You'll need to include this module somewhere, otherwise it wont be compiled. And you'll need to generate the relevant use statements too.

A line like:

mod test_bindings;

in gecko_bindings/lib.rs would suffice for the first part.

(Sorry, I meant gecko_bindings/mod.rs, my fault).

We don't want to include it in gecko_bindings, we want to include (include!(), not pub mod) it in tests/unit/stylo. The glue functions don't get defined till later. I plan on doing this myself later.

Should I move the script back to the same folder as 'bindings.rs`?

emilio · 2016-10-05T19:09:30Z

components/style/gecko_bindings/check_bindings.py

+
+tests.write("/* This Source Code Form is subject to the terms of the Mozilla Public\n")
+tests.write(" * License, v. 2.0. If a copy of the MPL was not distributed with this\n")
+tests.write(" * file, You can obtain one at http://mozilla.org/MPL/2.0/. */\n\n")


A warning about the file being an autogenerated file would be nice as well :)

Probably also maybe making the destination file components/style/generated/check_bindings.rs would be helpful, in addition to that.

In that case, you'll need to do something like:

#[path = "path/to/generated/check_bindings.rs"] mod check_bindings;

Also, please hook this to the components/style/binding_tools/regen.py script, the same way that regen_atoms.py is hooked in, and ideally put this script in that directory too :)

Thanks for doing this!

Did I do the last part correctly?

You did the hooking up to regen correctly 😄

As I said elsewhere, don't worry about including it as a mod just yet.

aneeshusa · 2016-10-05T19:50:46Z

components/style/binding_tools/check_bindings.py

+
+def build(objdir, verbose=False):
+    with open(objdir + "/bindings.rs", "r") as bindings:
+        with open(objdir + "/check_bindings.rs", "w+") as tests:


Please use os.path.join instead of string concatenation.

emilio · 2016-10-05T19:52:44Z

components/style/binding_tools/check_bindings.py

+import re
+
+def build(objdir, verbose=False):
+    with open(objdir+"/bindings.rs", "r") as bindings:


You don't need the objdir here. you should be able to get the current script directory (with os.path.dirname(os.path.abspath(__file__)), or passing TOOLS_DIR as an argument), and arrive into the bindings.rs file.

I think you can test that with ./regen.py --target check_bindings /dummy/path/since/it/wont/be/used

regen_atoms.py just uses the local dir it seems.

Manishearth · 2016-10-06T04:11:32Z

components/style/gecko_bindings/mod.rs

@@ -2,24 +2,5 @@
 * License, v. 2.0. If a copy of the MPL was not distributed with this
 * file, You can obtain one at http://mozilla.org/MPL/2.0/. */



This file should be unchanged. check_bindings.rs is not actually going to be used in this crate, so you don't need a mod for it

Also, could you commit the generated file?

Moved the script back to the same folder bindings.rs is, added the generated file there, and removed the changes to files that should not have been changed.

Manishearth · 2016-10-06T04:11:32Z

components/style/binding_tools/check_bindings.py

+
+
+def build(objdir, verbose=False):
+    with open(objdir + "/bindings.rs", "r") as bindings:


You shouldn't be using the objdir here, you should be using local paths. The objdir is pointed to where gecko headers live.

When you say local paths, do you mean just open("bindings.rs", "r") as if they were in the same folder, or open("components/style/gecko_bindings/bindings.rs", "r")?

Yes. Assume it's being run from the same dir.

I think this should be ../gecko_bindings/bindings.rs?

I meant "Assume it is being run from the same dir that contains the script"

Manishearth · 2016-10-06T04:11:32Z

components/style/gecko_bindings/test_bindings.rs

+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+


We don't want to include it in gecko_bindings, we want to include (include!(), not pub mod) it in tests/unit/stylo. The glue functions don't get defined till later. I plan on doing this myself later.

Manishearth

Sorry about the confusion wrt including the mod.

Manishearth · 2016-10-06T04:14:30Z

As far as using this file, it should be used via a new module in tests/unit/stylo, which contains:

// license header

use geckolib::glue::*;
use style::gecko_bindings::bindings;

include!("../../../components/style/gecko_bindings/check_bindings.rs")

I didn't ask for this to be done in this PR because I suspect there will be some errors that need fixing and I didn't want to extend the scope of the issue to that. The changes I requested right now should be enough.

wafflespeanut · 2016-10-06T05:06:36Z

components/style/binding_tools/check_bindings.py

+            tests.write("/* This Source Code Form is subject to the terms of the Mozilla Public\n")
+            tests.write(" * License, v. 2.0. If a copy of the MPL was not distributed with this\n")
+            tests.write(" * file, You can obtain one at http://mozilla.org/MPL/2.0/. */\n\n")
+            tests.write("/* automatically generated by check_bindings.py. */\n\n")


This should be put into a global name (like, LICENSE) on the top of the file.

wafflespeanut · 2016-10-06T05:06:36Z

components/style/binding_tools/check_bindings.py

+
+def build(objdir, verbose=False):
+    with open(objdir + "/bindings.rs", "r") as bindings:
+        with open(objdir + "/check_bindings.rs", "w+") as tests:


Instead of nesting inside, this could be joined as

with open(...) as bindings, open(...) as tests:

Also, as @aneeshusa said, it's a better idea to join the paths instead of hardcoding them, and put the paths in the top of the file, like BINDINGS_FILE, BINDINGS_CHECK_FILE, etc.

Oh, I didn't knew you could do that. Fixed.

wafflespeanut · 2016-10-06T05:06:36Z

components/style/binding_tools/check_bindings.py

+                if match:
+                    tests.write("    [ Servo_" + match.group(1) + ", bindings::Servo_" + match.group(1) + " ];\n")
+
+            tests.write("}\n")


Instead, we could have a template for this above, like

TEMPLATE = ''' [ Servo_{name}, bindings::Servo_{name} ]; '''

and format it every time we find a match.

aneeshusa · 2016-10-06T13:52:10Z

components/style/gecko_bindings/check_bindings.py

+
+import re
+
+LICENSE = """/* This Source Code Form is subject to the terms of the Mozilla Public


Do

LICENSE = """\ /* This Source Code ... """

at the beginning instead, as it keeps the license block formatted as usual.

Wouldn't that add a \n at the start of the generated file? Or the \ removes the \n from the resulting string?

Yes, \ followed by a newline is an escape sequence that is ignored/skipped by Python, so the resulting string won't have a leading newline.

Try it out in a small test file!

Manishearth · 2016-10-06T14:15:48Z

components/style/gecko_bindings/check_bindings.py

+# This Source Code Form is subject to the terms of the Mozilla Public
+# License, v. 2.0. If a copy of the MPL was not distributed with this
+# file, You can obtain one at http://mozilla.org/MPL/2.0/.
+


Uh, sorry for the confusion again.

To be clear: check_bindings.py belongs in binding_tools. The generated file belongs in gecko_bindings.

Aside from this everything else looks good! r=me once this file is moved.

Oh, I see. I'm preparing a new commit soon anyway.

Manishearth · 2016-10-06T14:18:09Z

Once done, please squash your commits together.


        Issue #13377 Extract panic-catching for JS engine callbacks into a

separate function


        checking for metadata in htmlmediaelement::response_complete

updated test expectations reverting changes to webgl test


        adding macro to return string or url,

using that macro with the form action, making the form submit process use base url, adding tests.


        Add OSMesa headless mode, run WPT against Webrender, update tests.


        Properly keep track of our union typedef support in WebIDL.py


        Update the WebIDL parser


        Disable intermittently failing tests.


        Switch default renderer to webrender, and update webrender.


        layout: Use the fragment's writing mode when computing sizes relevant…

… to that fragment. Otherwise we might mix writing modes. Not totally sure this change is correct in the case we're mixing them, we might need to just not checking that operation.


        script: Prevent an integer overflow that was hitting us on htmlimagee…

…lement.rs


        Moved assertion to after lock release; tidied up code.


        Remove GlobalRef::get_worker_id


        Simplify GlobalRef::resource_threads


        Remove ScopeThings::pipeline_id

It can be retrieved through its init field.


        Use the fetch stack for stylesheets.

Fixes #13462.


        Minimal Win32 font platform implementation.

This uses a (very simple) Win32 API call to enumerate font families available, and load them as byte buffers. The font rasterization itself is done by freetype. This gets Servo + WR + Windows working, but should be improved by adding a proper implementation that matches fonts correctly and also uses DirectWrite (or GDI) to handle font rasterization.


        layout: Place inline absolute hypothetical boxes properly during block

fragment position assignment. Improves Rust documentation. Closes #13471.


        Remove some documentation that is no longer correct.


        Add to_css_single_value for serializing a single PropertyDeclarationB…

…lock (fixes #13423)


        Remove Borrowed<T>/BorrowedMut<T>, use Option<&T> instead.


        Squash some build warnings.


        Removed duplicate #![feature(plugin)]


        Update webrender (text run + transforms fix).


        Ensure that SERVO_ENABLE_DEBUG_ASSERTIONS is also used in build-cef


        Invert conditional and return early for AddEventListener/RemoveEventL…

…istener


        Correcting source tidyness

Rafagd · 2016-10-06T14:52:32Z

I'm closing this request and making a new one.

Manishearth · 2016-10-06T15:25:38Z

You can just squash the commits via rebase and force-push to the same branch, no need to close the PR 😄

Rafagd · 2016-10-06T15:29:07Z

I did something wrong during the rebase and merged a bunch of unrelated stuff to this PR.
I felt it was easier to start from a clean fork instead of trying to fix the one I had.


        Auto merge of #13617 - Rafagd:my_changes, r=Manishearth

Script-generated tests for Servo_* gecko bindings All changes done in #13603, but this time I understand why I should have created a branch instead of doing everything on master. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #13598 (github issue number if applicable).  - [ ] There are tests for these changes OR - [x] These changes do not require tests because `they are tests`.   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13617)


        Auto merge of #13617 - Rafagd:my_changes, r=Manishearth

Script-generated tests for Servo_* gecko bindings All changes done in #13603, but this time I understand why I should have created a branch instead of doing everything on master. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #13598 (github issue number if applicable).  - [ ] There are tests for these changes OR - [x] These changes do not require tests because `they are tests`.   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13617)


        Auto merge of #13617 - Rafagd:my_changes, r=Manishearth

Script-generated tests for Servo_* gecko bindings All changes done in #13603, but this time I understand why I should have created a branch instead of doing everything on master. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #13598 (github issue number if applicable).  - [ ] There are tests for these changes OR - [x] These changes do not require tests because `they are tests`.   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13617)


        servo: Merge #13617 - Script-generated tests for Servo_* gecko bindin…

…gs (from Rafagd:my_changes); r=Manishearth All changes done in servo/servo#13603, but this time I understand why I should have created a branch instead of doing everything on master. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #13598 (github issue number if applicable).  - [ ] There are tests for these changes OR - [x] These changes do not require tests because `they are tests`.  Source-Repo: https://github.com/servo/servo Source-Revision: 03cd936639905e4c980ee706f4ee8f0f35de5180


        servo: Merge #13617 - Script-generated tests for Servo_* gecko bindin…

…gs (from Rafagd:my_changes); r=Manishearth All changes done in servo/servo#13603, but this time I understand why I should have created a branch instead of doing everything on master. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #13598 (github issue number if applicable).  - [ ] There are tests for these changes OR - [x] These changes do not require tests because `they are tests`.  Source-Repo: https://github.com/servo/servo Source-Revision: 03cd936639905e4c980ee706f4ee8f0f35de5180 UltraBlame original commit: 1cf764acd99a05d75440bfa73b1a32b12c1cf4ee


        servo: Merge #13617 - Script-generated tests for Servo_* gecko bindin…

…gs (from Rafagd:my_changes); r=Manishearth All changes done in servo/servo#13603, but this time I understand why I should have created a branch instead of doing everything on master. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #13598 (github issue number if applicable).  - [ ] There are tests for these changes OR - [x] These changes do not require tests because `they are tests`.  Source-Repo: https://github.com/servo/servo Source-Revision: 03cd936639905e4c980ee706f4ee8f0f35de5180 UltraBlame original commit: 1cf764acd99a05d75440bfa73b1a32b12c1cf4ee


        servo: Merge #13617 - Script-generated tests for Servo_* gecko bindin…

…gs (from Rafagd:my_changes); r=Manishearth All changes done in servo/servo#13603, but this time I understand why I should have created a branch instead of doing everything on master. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #13598 (github issue number if applicable).  - [ ] There are tests for these changes OR - [x] These changes do not require tests because `they are tests`.  Source-Repo: https://github.com/servo/servo Source-Revision: 03cd936639905e4c980ee706f4ee8f0f35de5180 UltraBlame original commit: 1cf764acd99a05d75440bfa73b1a32b12c1cf4ee

Script that creates a test for Servo_* bindings

15aea7b

highfive assigned SimonSapin Oct 5, 2016

highfive added the S-awaiting-review label Oct 5, 2016

Rafagd mentioned this pull request Oct 5, 2016

Write a python script to generate type assertions for Servo_* functions #13598

Closed

highfive assigned wafflespeanut and unassigned SimonSapin Oct 5, 2016

aneeshusa reviewed Oct 5, 2016

View changes

emilio reviewed Oct 5, 2016

View changes

aneeshusa reviewed Oct 5, 2016

View changes

emilio reviewed Oct 5, 2016

View changes

Manishearth reviewed Oct 6, 2016

View changes

Manishearth requested changes Oct 6, 2016

View changes

wafflespeanut requested changes Oct 6, 2016

View changes

aneeshusa reviewed Oct 6, 2016

View changes

Manishearth reviewed Oct 6, 2016

View changes

mylainos and others added 8 commits Sep 22, 2016

Issue #13377 Extract panic-catching for JS engine callbacks into a

667ab88

separate function

checking for metadata in htmlmediaelement::response_complete

a84a57f

updated test expectations reverting changes to webgl test

adding macro to return string or url,

76fe44c

using that macro with the form action, making the form submit process use base url, adding tests.

Add OSMesa headless mode, run WPT against Webrender, update tests.

30004bb

Properly keep track of our union typedef support in WebIDL.py

261fee5

Update the WebIDL parser

d367aaa

Disable intermittently failing tests.

486d304

Switch default renderer to webrender, and update webrender.

5f39b22

emilio and others added 18 commits Sep 26, 2016

layout: Use the fragment's writing mode when computing sizes relevant…

4f99070

… to that fragment. Otherwise we might mix writing modes. Not totally sure this change is correct in the case we're mixing them, we might need to just not checking that operation.

script: Prevent an integer overflow that was hitting us on htmlimagee…

6ec7ae1

…lement.rs

Moved assertion to after lock release; tidied up code.

9163fdc

Remove GlobalRef::get_worker_id

885f002

Simplify GlobalRef::resource_threads

0c34856

Remove ScopeThings::pipeline_id

af8cffd

It can be retrieved through its init field.

Use the fetch stack for stylesheets.

e8b14c4

Fixes #13462.

layout: Place inline absolute hypothetical boxes properly during block

a55c9f2

fragment position assignment. Improves Rust documentation. Closes #13471.

Remove some documentation that is no longer correct.

3299b54

Add to_css_single_value for serializing a single PropertyDeclarationB…

eb2ff49

…lock (fixes #13423)

Remove Borrowed<T>/BorrowedMut<T>, use Option<&T> instead.

83fee0f

Squash some build warnings.

444326f

Removed duplicate #![feature(plugin)]

a7d643f

Update webrender (text run + transforms fix).

9861739

Ensure that SERVO_ENABLE_DEBUG_ASSERTIONS is also used in build-cef

08c24eb

Invert conditional and return early for AddEventListener/RemoveEventL…

119ab1d

…istener

Correcting source tidyness

Loading status checks…

07e1b5d

Rafagd closed this Oct 6, 2016

Rafagd mentioned this pull request Oct 6, 2016

Script-generated tests for Servo_* gecko bindings #13617

Merged

4 of 5 tasks complete

		@@ -2,24 +2,5 @@
		* License, v. 2.0. If a copy of the MPL was not distributed with this
		* file, You can obtain one at http://mozilla.org/MPL/2.0/. */



		def build(objdir, verbose=False):
		with open(objdir + "/bindings.rs", "r") as bindings:


		import re

		LICENSE = """/* This Source Code Form is subject to the terms of the Mozilla Public

servo / servo

Join GitHub today

Test generation for Servo_* bindings in `components/style/gecko_bindings/bindings.rs` #13603

Test generation for Servo_* bindings in `components/style/gecko_bindings/bindings.rs` #13603

Conversation

Rafagd commented Oct 5, 2016 • edited

highfive commented Oct 5, 2016

highfive commented Oct 5, 2016

highfive commented Oct 5, 2016

emilio commented Oct 5, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Manishearth left a comment

Manishearth commented Oct 6, 2016

This comment has been minimized.

This comment has been minimized.

wafflespeanut Oct 6, 2016 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Manishearth commented Oct 6, 2016

Rafagd commented Oct 6, 2016

Manishearth commented Oct 6, 2016

Rafagd commented Oct 6, 2016

Rafagd commented Oct 5, 2016 •

edited

wafflespeanut Oct 6, 2016 •

edited