1063845 - Fix remaining style inspector e10s tests

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Brian Grinstead [:bgrins]]

I thought that these were all passing when I first opened Bug 1058875, but as it turns out there are still some failures (see https://tbpl.mozilla.org/?tree=Holly&rev=a9cc7f174c26, for instance).  Looking at errors like this (https://tbpl.mozilla.org/php/getParsedLog.php?id=47401370&tree=Holly&full=1#error0) makes me think that these are legitimate timeouts where the test is still running when it gets killed.  Also, the exact list of failing tests doesn't seem to be the same across platforms, which also makes me think it could be an issue of them just not having enough time to finish.

219 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/styleinspector/test/browser_ruleview_completion-existing-property_01.js | Test timed out
248 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/styleinspector/test/browser_ruleview_completion-existing-property_02.js | Test timed out
339 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/styleinspector/test/browser_ruleview_completion-new-property_01.js | Test timed out
431 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/styleinspector/test/browser_ruleview_completion-new-property_02.js | Test timed out
462 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/styleinspector/test/browser_ruleview_edit-selector_01.js | Test timed out
473 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/styleinspector/test/browser_ruleview_edit-selector_02.js | Test timed out
482 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/styleinspector/test/browser_ruleview_inherit.js | Test timed out
484 ERROR TEST-UNEXPECTED-TIMEOUT | chrome://mochitests/content/browser/browser/devtools/styleinspector/test/browser_ruleview_inherit.js | application timed out after 330 seconds with no output
TEST-UNEXPECTED-FAIL | ShutdownLeaks | process() called before end of test suite
485 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/styleinspector/test/browser_ruleview_inherit.js | application terminated with exit code 5
PROCESS-CRASH | chrome://mochitests/content/browser/browser/devtools/styleinspector/test/browser_ruleview_inherit.js | application crashed [@ libSystem.B.dylib + 0xd7a]
Return code: 1

Comment 1

[Mark Hammond [:markh] [:mhammond]]

One of the first problems I see here is a failure in browser_ruleview_livepreview.js:

65 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/styleinspector/test/browser_ruleview_livepreview.js | Element should be previewed as block - Got inline-block, expected block
Stack trace:
    chrome://mochikit/content/browser-test.js:test_is:793
    chrome://mochitests/content/browser/browser/devtools/styleinspector/test/browser_ruleview_livepreview.js:testLivePreviewData:63

Looking at line 60, I see a somewhat suspect 'wait(1)' - if I change this to wait(100) the test passes for me (note that 10 still fails).

Patrick, your name is all over this test - any suggestions for a better fix (or failing that, if you'd r+ a patch that bumps the wait to 100)?

Comment 2

[Mark Hammond [:markh] [:mhammond]]

The last failing styleinspector test I see is browser_ruleview_pseudo-element.js, and this can be worked-around in a similar way.  The failure is:

410 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/styleinspector/test/browser_ruleview_pseudo-element.js | Added property should have been used. - Got rgb(221, 221, 221), expected rgb(0, 0, 255)
Stack trace:
    chrome://mochikit/content/browser-test.js:test_is:793
    chrome://mochitests/content/browser/browser/devtools/styleinspector/test/browser_ruleview_pseudo-element.js:testTopLeft:116
    Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:865:23
    this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:744:7

If at line 116 I add the line:

     yield wait(100);

The test passes - so hopefully there is a single fix we can use in both places.

Comment 3

[Patrick Brosset <:pbro>]

I believe |yield wait(1);| was introduced merely as a replacement to |executeSoon(cb);| when I refactored the styleinspector tests in bug 988313.
This was used to make sure the css property changes done in the style-inspector UI would be reflected in the page before continuing the test.
Now, with e10s, the style-inspector UI and the page running in 2 different processes, using an equivalent of |executeSoon| isn't enough anymore.

I think these 2 failing tests should be fixed using the message manager. There are already a few tests that use it in this directory, and there are helper functions in head.js.
See browser_ruleview_edit-property_01.js for example:

  let propValue = yield executeInContent("Test:GetRulePropertyValue", {
    styleSheetIndex: 0,
    ruleIndex: 0,
    name
  });
  is(propValue, value, name + " should have been set.");

Here, the test goes through the MM to retrieve the value of a given css property name, in a given stylesheet rule. When the response is known, it compares it to the expected value.

Comment 4

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0001-use-messages-to-fetch-computed-style.patch

thanks for the pointer - something like this?

Comment 5

[Patrick Brosset <:pbro>]

Comment on attachment 8491304 [details] [diff] [review]
0001-use-messages-to-fetch-computed-style.patch

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

Yes, something like this would be perfect.

::: browser/devtools/styleinspector/test/browser_ruleview_pseudo-element.js
@@ +272,5 @@
>    return gutters;
>  }
> +
> +// get a computed style property by messaging the content process.
> +function* getComputedStyleProperty(element, pseudo, propName) {

Worth moving into head.js as this is something common enough that we want to have a simple helper function there.

::: browser/devtools/styleinspector/test/doc_frame_script.js
@@ +82,5 @@
> + * @return {String} The value, if found, null otherwise
> + */
> +addMessageListener("Test:GetComputedStylePropertyValue", function(msg) {
> +  let {id, pseudo, name} = msg.data;
> +  let element = content.document.getElementById(id, pseudo);

Why do you pass pseudo here?
In fact, what about turning id into selector and using content.document.querySelector instead? That would make this function more generic.

Comment 6

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0001-Bug-1063845-Fix-remaining-style-inspector-e10s-tests.patch

Thanks!  With this patch the style inspector tests all pass locally both with and without e10s enabled.

Comment 7

[Patrick Brosset <:pbro>]

Comment on attachment 8491338 [details] [diff] [review]
0001-Bug-1063845-Fix-remaining-style-inspector-e10s-tests.patch

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

Looks good. Thanks!

Comment 8

[Mark Hammond [:markh] [:mhammond]]

Thanks!

https://hg.mozilla.org/integration/fx-team/rev/849a1b3074c3

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/849a1b3074c3