1187110 - Make test_extra_inherit_initial.html a little less intense (and re-enable it on B2G debug)

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Daniel Holbert [:dholbert]]

Right now, the mochitest test_extra_inherit_initial.html is among our longest-running mochitests. (It takes close to a minute on my local machine, and over 16 minutes on b2g debug, which resulted in it making the test-harness crap out (bug 1168222), which has prevented its mochitest job from being visible on TreeHerder until this test was recently disabled for that platform.)

It takes so long because it tests a variety of keywords, at the beginning & end of *every* test-value of *every* property that we have in property_database.js.

We don't really need to be that exhaustive.  Really, my goal in checking in the test (in bug 940229) was to cover shorthand properties.  The amount of time we're spending on all the other properties' piles of values is kinda overkill for this test.

So we should perhaps shorten the test to only focus on test the first N test-values of each longhand property, & still test all values of each shorthand property. (With N being something small, maybe 3.)

Comment 1

[Daniel Holbert [:dholbert]]

Actually, I came up with what I think is a better strategy here:
 - For each CSS property, only test the first "N" valid values robustly (with each of "inherit"/"initial"/"unset", at the beginning and then at the end of the value, to be sure that these modified values are treated as invalid.)  (This is what we currently do for all values.)

 - For the property's remaining values, *only* test *one of* inherit/initial/unset at the beginning & end.  This shouldn't really lose any robustness, since we use a common function to parse each of these special keywords (a call to "ParseVariant(tmpVal, VARIANT_INHERIT, nullptr)"), and it'll remove 2/3 of the [kinda-redundant] checks that we're doing on these values/keywords right now.

I described this strategy to dbaron last week and he said he'd be OK with it. I'll want the patch to be reviewed, since it involves some nontrivial tweaks to the test. I'm hoping heycam might be able to review, since dbaron [who's the original test author] is backlogged & will be traveling a lot soon.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: part 1: use "for...of" loops to iterate over arrays

First, 2 essentially-trivial patches here to clean up the test a bit.

This one converts some "for (var idx in array) { foo = array[idx] ... }" loops to "for (let foo of array)"

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: part 2: s/var/let/

...and this trivial patch replaces the remaining "var" decls with "let", to make scoping more explicit.

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: part 1 v2: use "for...of" for iteration over one array (where we don't care about array idx)

(sorry, disregard my previous part 1 & 2 patches -- I remembered that my later patch actually needs an integer-valued loop counter when we're looping over property-values, so I don't want to convert those loops to use for...of.)

So, here's a simplified version of "part 1" which just converts the one loop where I don't care about having an integer loop-counter.

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: part 2 v2: s/var/let/

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: part 3: Store the special keywords to test in an array, instead of hardcoding them in function-calls

This patch just refactors the existing logic in this test to be a bit more declarative (using an array of keywords which we walk over, instead of just making arbitrary function calls).

There's still no actual change in behavior yet (until the next patch) - just a few minor ordering changes.

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: part 4: After the first 3 values in a values-array, only test one keyword per vlaue

This is the real relevant tweak here. This reduces the test's number of checks and runtime to ~40% of current levels.

(And hopefully this will mean it'll now easily complete within the 1000 second hard-upper-limit that we're semi-accidentally imposing on all mochitests right now via bug 1187038.)

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: part 5: re-enable the test on b2g debug

Comment 9

[Cameron McCormack (:heycam)]

Comment on attachment 8642746 [details] [diff] [review]
part 2 v2: s/var/let/

Review of attachment 8642746 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/test/test_extra_inherit_initial.html
@@ +30,5 @@
>    "voice-family": true,
>  };
>  
> +let gElement = document.getElementById("testnode");
> +let gDeclaration = gElement.style;

Since let behaves the same as var at the top level, I wonder if using let here obscures the fact that these variables persist?  Either way.

Comment 10

[Daniel Holbert [:dholbert]]

(In reply to Cameron McCormack (:heycam) from comment #9)
> > +let gElement = document.getElementById("testnode");
> > +let gDeclaration = gElement.style;
> 
> Since let behaves the same as var at the top level, I wonder if using let
> here obscures the fact that these variables persist?  Either way.

Hmm, good question -- I think it's still clear enough that they're global, so I think I'll opt to leave the patch as-is.  (They have "g" at the beginning, and they're declared at the top level, so their scope is pretty-clearly global for this file, IMO.)

(I could have left these ones "var", as you suggest, since they're global regardless; but it felt cleaner to just convert everything, in the interests of consistency with my new code. Having let & var intermixed feels broken to me, since it's basically having directly-adjacent sections of code with different levels of strictness, with no clear reason for the difference.)

Thanks for the review!

Comment 11

[Daniel Holbert [:dholbert]]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=50fe0301b330

B2G ICS Emulator debug M-25 is the main test run that I'll be looking at there -- that's where this testcase gets batched on that platform, last I checked.

Comment 12

[Daniel Holbert [:dholbert]]

Try run looks good -- I triggered about 20 M-25 runs on B2G ICS Emulator debug, and they're all green.

The logs show that the duration for this test is well under the 1000-second harness timeout now, too -- it's in the 500-600 second range, in the 3 logs that I checked manually.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/de6004c72675
https://hg.mozilla.org/integration/mozilla-inbound/rev/5189e3db2bc2
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4e7b4e83365
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5b09e238cd8
https://hg.mozilla.org/integration/mozilla-inbound/rev/f763ac7bd0bd

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/de6004c72675
https://hg.mozilla.org/mozilla-central/rev/5189e3db2bc2
https://hg.mozilla.org/mozilla-central/rev/c4e7b4e83365
https://hg.mozilla.org/mozilla-central/rev/a5b09e238cd8
https://hg.mozilla.org/mozilla-central/rev/f763ac7bd0bd