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

RESOLVED FIXED in Firefox 42

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
mozilla42
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(5 attachments, 2 obsolete attachments)

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.)
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.
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)"
Attachment #8642635 - Flags: review?(cam)
Posted patch part 2: s/var/let/ (obsolete) — Splinter Review
...and this trivial patch replaces the remaining "var" decls with "let", to make scoping more explicit.
Attachment #8642680 - Flags: review?(cam)
(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.
Attachment #8642635 - Attachment is obsolete: true
Attachment #8642680 - Attachment is obsolete: true
Attachment #8642635 - Flags: review?(cam)
Attachment #8642680 - Flags: review?(cam)
Attachment #8642745 - Flags: review?(cam)
Attachment #8642746 - Flags: review?(cam)
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.
Attachment #8642752 - Flags: review?(cam)
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.)
Attachment #8642756 - Flags: review?(cam)
Blocks: 1168222
Attachment #8642745 - Flags: review?(cam) → review+
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.
Attachment #8642746 - Flags: review?(cam) → review+
Attachment #8642752 - Flags: review?(cam) → review+
Attachment #8642756 - Flags: review?(cam) → review+
(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!
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.
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.
Summary: Make test_extra_inherit_initial.html a little less intense → Make test_extra_inherit_initial.html a little less intense (and re-enable it on B2G debug)
You need to log in before you can comment on or make changes to this bug.