Remove style/testing feature #17984

Manishearth · 2017-08-06T07:13:24Z

We added this because a year ago we had no reliable Gecko CI. This meant that Gecko-only properties needed to be tested somehow, and we solved that by making it so that for unit tests we compile all properties, not just the servo ones.

This was useful back then, but I don't think we need this anymore. We have reliable Gecko CI, and all the gecko-only stuff we tested is adequately handled by the properties-database parsing mochitests. It's a bit of annoying cruft that just complicates things; we probably should remove it.

r? @emilio or @SimonSapin

This change is

highfive · 2017-08-06T07:13:28Z

Heads up! This PR modifies the following files:

@bholley: components/style/properties/longhand/position.mako.rs, components/style/Cargo.toml, components/style/properties/data.py, components/style/properties/shorthand/text.mako.rs, components/style/properties/build.py and 6 more
@canaltinova: components/style/properties/longhand/position.mako.rs, components/style/Cargo.toml, components/style/properties/data.py, components/style/properties/shorthand/text.mako.rs, components/style/properties/build.py and 6 more
@wafflespeanut: python/servo/testing_commands.py
@emilio: components/style/properties/longhand/position.mako.rs, components/style/Cargo.toml, components/style/properties/data.py, components/style/properties/shorthand/text.mako.rs, ports/geckolib/Cargo.toml and 7 more

Manishearth · 2017-08-06T07:28:22Z

Gah, there's some ruletree stuff that still needs this feature. Kept it around for testing stuff, but removed the whole complexity wrt conditional compilation of properties.

emilio

That's just to avoid asserting thread stuff right? If so, it may be worth to just mock the thread state module or something...

Losing the testing of font-feature-values is kinda unfortunate, since we don't test that right now yet...

SimonSapin · 2017-08-06T14:01:13Z

I didn’t realize this would involve removing tests. Can we move them to tests/units/stylo instead? Or have a runtime flag instead of a compilation flag?

emilio

Looks good... I agree with @SimonSapin that losing test coverage is not great, but I think it's not a huge deal either...

But perhaps it can be moved to tests/unit/stylo as he suggests.

emilio · 2017-08-06T22:14:07Z

tests/unit/style/rule_tree/bench.rs

@@ -148,6 +152,7 @@ fn bench_expensive_insertion(b: &mut Bencher) {
 #[bench]
 fn bench_insertion_basic_parallel(b: &mut Bencher) {
    let r = RuleTree::new();
+    thread_state::initialize(thread_state::SCRIPT);


I think this would also be fine with thread_state::LAYOUT, and would be preferable.

Manishearth · 2017-08-06T22:42:27Z

I didn’t realize this would involve removing tests.

This is all stuff well tested on the m-c side. Most of these tests are simple tests added by new contributors back when we didn't have mochitests.

Runtime flag won't work, though.

SimonSapin · 2017-08-06T23:38:11Z

Alright. r=me with the commits squashed or reordered. It a previous push the feature was removed before its usage was removed. Now it’s removed, added back, and removed again.

Reviewed 17 of 17 files at r1, 14 of 21 files at r2, 9 of 9 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from Reviewable

Manishearth · 2017-08-07T20:37:57Z

@bors-servo r=SimonSapin

bors-servo · 2017-08-07T20:37:57Z

📌 Commit 3c9fa22 has been approved by SimonSapin

bors-servo · 2017-08-07T20:38:02Z

⌛ Testing commit 3c9fa22 with merge c2299e3...

@emilio

Remove style/testing feature We added this because a year ago we had no reliable Gecko CI. This meant that Gecko-only properties needed to be tested *somehow*, and we solved that by making it so that for unit tests we compile all properties, not just the servo ones. This was useful back then, but I don't think we need this anymore. We have reliable Gecko CI, and all the gecko-only stuff we tested is adequately handled by the properties-database parsing mochitests. It's a bit of annoying cruft that just complicates things; we probably should remove it. r? @emilio or @SimonSapin  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17984)

Manishearth · 2017-08-07T20:48:25Z

@bors-servo r=SimonSapin

push didn't happen. gah.

bors-servo · 2017-08-07T20:48:26Z

📌 Commit e1fbf5a has been approved by SimonSapin

bors-servo · 2017-08-07T20:48:30Z

⌛ Testing commit e1fbf5a with merge 41fc0d8...

@emilio

Remove style/testing feature We added this because a year ago we had no reliable Gecko CI. This meant that Gecko-only properties needed to be tested *somehow*, and we solved that by making it so that for unit tests we compile all properties, not just the servo ones. This was useful back then, but I don't think we need this anymore. We have reliable Gecko CI, and all the gecko-only stuff we tested is adequately handled by the properties-database parsing mochitests. It's a bit of annoying cruft that just complicates things; we probably should remove it. r? @emilio or @SimonSapin  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17984)

SimonSapin · 2017-08-07T21:08:26Z

@bors-servo r-

There’s still stuff like cfg!(feature = "testing") being added in the second commit. Maybe easier to squash everything?

SimonSapin · 2017-08-08T11:57:58Z

@bors-servo r+

bors-servo · 2017-08-08T11:57:58Z

📌 Commit 2ebce54 has been approved by SimonSapin

bors-servo · 2017-08-08T12:35:30Z

⌛ Testing commit 2ebce54 with merge 32f8352...

@emilio

Remove style/testing feature We added this because a year ago we had no reliable Gecko CI. This meant that Gecko-only properties needed to be tested *somehow*, and we solved that by making it so that for unit tests we compile all properties, not just the servo ones. This was useful back then, but I don't think we need this anymore. We have reliable Gecko CI, and all the gecko-only stuff we tested is adequately handled by the properties-database parsing mochitests. It's a bit of annoying cruft that just complicates things; we probably should remove it. r? @emilio or @SimonSapin  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17984)

bors-servo · 2017-08-08T13:27:14Z

☀️ 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: SimonSapin
Pushing 32f8352 to master...

highfive assigned emilio Aug 6, 2017

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Aug 6, 2017

Manishearth force-pushed the rm-testing branch from ddf8292 to 29debb9 Compare August 6, 2017 07:33

emilio reviewed Aug 6, 2017

View reviewed changes

emilio removed the S-awaiting-review There is new code that needs to be reviewed. label Aug 7, 2017

highfive assigned SimonSapin and unassigned emilio Aug 7, 2017

highfive added the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Aug 7, 2017

Manishearth force-pushed the rm-testing branch from 3c9fa22 to e1fbf5a Compare August 7, 2017 20:48

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Aug 7, 2017

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Aug 7, 2017

Remove testing feature from style crate

2ebce54

Manishearth force-pushed the rm-testing branch from e1fbf5a to 2ebce54 Compare August 7, 2017 21:30

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Aug 7, 2017

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Aug 8, 2017

bors-servo merged commit 2ebce54 into servo:master Aug 8, 2017

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Aug 8, 2017

Manishearth deleted the rm-testing branch May 7, 2019 22:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove style/testing feature #17984

Remove style/testing feature #17984

Manishearth commented Aug 6, 2017 •

edited by SimonSapin

highfive commented Aug 6, 2017

Manishearth commented Aug 6, 2017

emilio left a comment

SimonSapin commented Aug 6, 2017

emilio left a comment

emilio Aug 6, 2017

Manishearth commented Aug 6, 2017

SimonSapin commented Aug 6, 2017

Manishearth commented Aug 7, 2017

bors-servo commented Aug 7, 2017

bors-servo commented Aug 7, 2017

Manishearth commented Aug 7, 2017

bors-servo commented Aug 7, 2017

bors-servo commented Aug 7, 2017

SimonSapin commented Aug 7, 2017

SimonSapin commented Aug 8, 2017

bors-servo commented Aug 8, 2017

bors-servo commented Aug 8, 2017

bors-servo commented Aug 8, 2017

Remove style/testing feature #17984

Remove style/testing feature #17984

Conversation

Manishearth commented Aug 6, 2017 • edited by SimonSapin

highfive commented Aug 6, 2017

Manishearth commented Aug 6, 2017

emilio left a comment

Choose a reason for hiding this comment

SimonSapin commented Aug 6, 2017

emilio left a comment

Choose a reason for hiding this comment

emilio Aug 6, 2017

Choose a reason for hiding this comment

Manishearth commented Aug 6, 2017

SimonSapin commented Aug 6, 2017

Manishearth commented Aug 7, 2017

bors-servo commented Aug 7, 2017

bors-servo commented Aug 7, 2017

Manishearth commented Aug 7, 2017

bors-servo commented Aug 7, 2017

bors-servo commented Aug 7, 2017

SimonSapin commented Aug 7, 2017

SimonSapin commented Aug 8, 2017

bors-servo commented Aug 8, 2017

bors-servo commented Aug 8, 2017

bors-servo commented Aug 8, 2017

Manishearth commented Aug 6, 2017 •

edited by SimonSapin