Stop using CPOWs in rule-view mochitests

RESOLVED FIXED in Firefox 47

Status

defect
RESOLVED FIXED
3 years ago
11 months ago

People

(Reporter: pbro, Assigned: pbro)

Tracking

unspecified
Firefox 47
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(10 attachments)

58 bytes, text/x-review-board-request
miker
: review+
Details
58 bytes, text/x-review-board-request
miker
: review+
Details
58 bytes, text/x-review-board-request
tromey
: review+
Details
58 bytes, text/x-review-board-request
miker
: review+
Details
58 bytes, text/x-review-board-request
jdescottes
: review+
Details
58 bytes, text/x-review-board-request
tromey
: review+
Details
58 bytes, text/x-review-board-request
jdescottes
: review+
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
ochameau
: review+
Details
58 bytes, text/x-review-board-request
Details
Assignee

Description

3 years ago
Search for 'getNode(' in devtools/client/inspector/rules/test and you'll see where CPOWs are explicitly used still.

We have many options to avoid relying on CPOWs now, so let's use them.
Assignee

Updated

3 years ago
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
Assignee

Comment 1

3 years ago
There are also many occurrences of content.getComputedStyle in these tests.
Assignee

Comment 2

3 years ago
Depending on bug 1244120 which removes a CPOW usage in browser_rules_content_02.js
Depends on: 1244120
Assignee

Updated

3 years ago
Blocks: 1247263
Assignee

Comment 13

3 years ago
Sorry for the massive review request for some of these commits.
Hopefully they make sense. If not, please ping me. I can explain the reason behind these changes.
Comment on attachment 8718820 [details]
MozReview Request: Bug 1246677 - 1 - Make waitForSuccess work with async functions

https://reviewboard.mozilla.org/r/34759/#review31383
Attachment #8718820 - Flags: review?(mratcliffe) → review+
Comment on attachment 8718821 [details]
MozReview Request: Bug 1246677 - 2 - Stop using CPOWs in simulateColorPickerChange

https://reviewboard.mozilla.org/r/34761/#review31385
Attachment #8718821 - Flags: review?(mratcliffe) → review+
Comment on attachment 8718823 [details]
MozReview Request: Bug 1246677 - 4 - Stop using content.getComputedStyle in ruleview tests

https://reviewboard.mozilla.org/r/34765/#review31391
Attachment #8718823 - Flags: review?(mratcliffe) → review+
Comment on attachment 8718824 [details]
MozReview Request: Bug 1246677 - 5 - Get rid of 'content' in ruleview test files

https://reviewboard.mozilla.org/r/34767/#review31397

Nice!

::: devtools/client/inspector/rules/test/browser_rules_content_01.js:21
(Diff revision 1)
> -    "  background-color: green;" +
> +  <div id='testid2'>Styled Node</div>

nit : attributes could use double quotes rather than single quotes now.

::: devtools/client/inspector/rules/test/browser_rules_edit-property-increments.js:15
(Diff revision 1)
> +      color:#000000;

nit : spaces after colon 
    
    margin-top: 0px;
    [...]
    color: #000000;

::: devtools/client/inspector/rules/test/browser_rules_style-editor-link.js:144
(Diff revision 1)
> +    `content.document.styleSheets[${expectedSheetIndex}]`);

I have a test failure here. "sheet" is an object, but sheet.href is always undefined.

FWIW, using 

    let sheetHref = yield testActor.eval(
      `content.document.styleSheets[${expectedSheetIndex}].href`);
    is(editor.styleSheet.href, sheetHref,
      "loaded stylesheet href matches document stylesheet");
      
fixes the issue for me.
Attachment #8718824 - Flags: review?(jdescottes) → review+
Attachment #8718826 - Flags: review?(jdescottes) → review+
Comment on attachment 8718826 [details]
MozReview Request: Bug 1246677 - 7 - Clean remaining ruleview and tests eslint warnings

https://reviewboard.mozilla.org/r/34771/#review31403

Can't wait for the rebase after this lands :)

::: devtools/client/inspector/rules/test/browser_rules_edit-property-increments.js:202
(Diff revision 1)
> +    key = key;

Should remove the `else { key = key }` .

::: devtools/client/inspector/rules/test/browser_rules_pseudo-element_01.js:30
(Diff revision 1)
> -  let {rules} = yield assertPseudoElementRulesNumbers(selector,
> +  let {rules} = yield assertPseudoElementRulesNumbers(el,

nit : I feel like el is misleading. I understand selector was pushing some lines of the function over 80 chars. How about `id` ? Not totally accurate either, but a bit closer.
Comment on attachment 8718822 [details]
MozReview Request: Bug 1246677 - 3 - Remove all usages of getNode in ruleview tests

https://reviewboard.mozilla.org/r/34763/#review31411

This looks good, and thanks very much for doing this.  I have a few nits; nothing serious.

::: devtools/client/inspector/rules/test/browser_rules_keyframes-rule_02.js:89
(Diff revision 1)
> -  return {rules, element, elementStyle};
> +  return {rules};

It would be clearer for future readers not to box this, but to just "return rules" and update the callers not to unbox.

::: devtools/client/inspector/rules/test/browser_rules_pseudo-element_01.js:197
(Diff revision 1)
> -  return {element: element, elementStyle: elementStyle};
> +  return {elementStyle};

Another spot where boxing isn't needed.

::: devtools/client/inspector/rules/test/browser_rules_pseudo-element_01.js:222
(Diff revision 1)
> -  return {rules, element, elementStyle};
> +  return {rules};

Here too.
Attachment #8718822 - Flags: review?(ttromey) → review+
Comment on attachment 8718825 [details]
MozReview Request: Bug 1246677 - 6 - Get rid of 'content' in ruleview head.js

https://reviewboard.mozilla.org/r/34769/#review31413

Looks good, just a comment change needed.

::: devtools/client/inspector/rules/test/head.js:742
(Diff revision 1)
> -function reloadPage(inspector) {
> +function* reloadPage(inspector, testActor) {

This needs a small jsdoc update for the new parameter; and because this function no longer returns a promise.
Attachment #8718825 - Flags: review?(ttromey) → review+
Comment on attachment 8718828 [details]
MozReview Request: Bug 1246677 - 9 - Get rid of all remaining _applyingModifications usage in tests

https://reviewboard.mozilla.org/r/34775/#review31531

Looks good to me.
I imagine you didn't introduced ruleview-changed wait where we used to yield on null applyModifications?

::: devtools/client/inspector/rules/test/browser_rules_add-property-cancel_01.js
(Diff revision 1)
> -    "Shouldn't have an outstanding request after a cancel.");

Was applyingModifications null?
Shouldn't we wait for ruleview-changed?

::: devtools/client/inspector/rules/test/browser_rules_add-property-cancel_03.js
(Diff revision 1)
> -    "Shouldn't have an outstanding modification request after a cancel.");

Same.

::: devtools/client/inspector/rules/test/browser_rules_edit-property-cancel.js:29
(Diff revision 1)
>      ["VK_DELETE", "VK_ESCAPE"]);

sendCharsAndWaitForFocus doesn't wait for ruleview-changed, shouldn't we wait for it here or from this helper function?
Attachment #8718828 - Flags: review?(poirot.alex) → review+
Assignee

Comment 22

3 years ago
(In reply to Alexandre Poirot [:ochameau] from comment #21)
> Looks good to me.
> I imagine you didn't introduced ruleview-changed wait where we used to yield
> on null applyModifications?
Yeah, I was very cautious about checking what did and did not trigger server-side requests in the tests. We used to have _applyingModifications in sort of random places because they were added not knowing exactly how everything worked most of the times.
> :::
> devtools/client/inspector/rules/test/browser_rules_add-property-cancel_01.js
> (Diff revision 1)
> > -    "Shouldn't have an outstanding request after a cancel.");
> 
> Was applyingModifications null?
> Shouldn't we wait for ruleview-changed?
_applyingModifications is always null here, escaping out of property name field doesn't cause any preview or server requests. So there's no need to wait for ruleview-changed here.
> :::
> devtools/client/inspector/rules/test/browser_rules_add-property-cancel_03.js
> (Diff revision 1)
> > -    "Shouldn't have an outstanding modification request after a cancel.");
> 
> Same.
Same reason here.
> :::
> devtools/client/inspector/rules/test/browser_rules_edit-property-cancel.js:29
> (Diff revision 1)
> >      ["VK_DELETE", "VK_ESCAPE"]);
> 
> sendCharsAndWaitForFocus doesn't wait for ruleview-changed, shouldn't we
> wait for it here or from this helper function?
I can't wait for the ruleview-changed event in the helper because it's used both for changing the property name and property value. And only changing the property value triggers server-side requests that we need to wait for.
Comment on attachment 8718827 [details]
MozReview Request: Bug 1246677 - 8 - Use addPRoperty and remove code duplication

https://reviewboard.mozilla.org/r/34773/#review32043

Commit msg addPRoperty is miscapitalized - "Use addProperty and remove code duplication"

::: devtools/client/inspector/rules/test/head.js:601
(Diff revision 1)
> +  let nbOfProps = ruleEditor.rule.textProps.length;

I prefer numOfProps
Attachment #8718827 - Flags: review?(gl) → review+
Comment on attachment 8718829 [details]
MozReview Request: Bug 1246677 - 10 - Removed addStyle from head.js since it was unused

https://reviewboard.mozilla.org/r/34777/#review32045
Attachment #8718829 - Flags: review?(gl) → review+
Assignee

Comment 25

3 years ago
Bug 1244120 landed again, and should stick this time, so I rebased my patches, and pushed to try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=898af952fc11&group_state=expanded
Assignee

Comment 26

3 years ago
Bug 1244120 made it to m-c. Try is mostly green apart from some remaining eslint failures which I've now fixed and some weird looking leaks on windows debug e10s which I sure hope aren't related to my changes.
I'll spend some more time investigating and then will land these patches shortly after (rebasing them these past few weeks has been hard).
Backed out for causing frequent OSX browser_rules_completion-existing-property_01.js failures (10.6 and 10.10).
https://hg.mozilla.org/integration/fx-team/rev/8d2eef5f7249

https://treeherder.mozilla.org/logviewer.html#?job_id=7513162&repo=fx-team
Assignee

Comment 30

3 years ago
I have a potential fix for this test which I'm running on try at the moment. Of course I can't reproduce this issue locally. So, until I do, pushing potential fixes to try is my best option.
Assignee

Comment 31

3 years ago
The test that was failing was changed in my part 6. There used to a `yield wait(1)` in this test, which I had replaced with `yield waitForTick()`. It looks like that's what was causing the test to fail.
Looking at the logs, I decided to remove this altogether and instead explicitly wait for the completion popup to hide when needed.
Pushed parts 1 to 6 + this fix to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1bd83a9b3948&group_state=expanded
This seems to have worked, so I'll merge this in, rebase, etc... and land again my patches.
Assignee

Updated

3 years ago
Duplicate of this bug: 1246458
Assignee

Updated

3 years ago
Duplicate of this bug: 1246463

Updated

11 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.