Closed Bug 1252099 Opened 9 years ago Closed 8 years ago

Stop using CPOWs in markup-view mochitests

Categories

(DevTools :: Inspector, defect, P1)

Product:
DevTools
Component:
Inspector
Type:
defect

Tracking

(firefox47 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 47
Tracking Flags:
Tracking Status
firefox47 --- fixed

People

(Reporter: pbro, Assigned: pbro)

References

Details

(Whiteboard: [btpp-fix-now])

Attachments

(4 files)

Bug_1252099_-_Do_not_use_CPOWs_in_browser_markup_c.diff
28.83 KB, patch
jdescottes
: review+
Details | Diff | Splinter Review
MozReview Request: Bug 1252099 - Main eslint cleanup of markupview tests; r=zer0
58 bytes, text/x-review-board-request
zer0
: review+
Details
MozReview Request: Bug 1252099 - Remove usage of getNode and content in markupview tests; r=ochameau
58 bytes, text/x-review-board-request
ochameau
: review+
Details
MozReview Request: Bug 1252099 - Final eslint cleanups in devtools/client/inspector/markup/test; r=miker
58 bytes, text/x-review-board-request
miker
: review+
Details 
Stop using CPOWs for accessing DOM elements in markup-view tests. We have everything we need to avoid this.
Assignee: nobody → pbrosset
Priority: -- → P1
Whiteboard: [btpp-fix-now]
Blocks: 1252345
Blocks: 1238022
A first commit just for one test: browser_markup_copy_image_data.js
Fixing this one first because it seems like the most urgent (see the 2 blocked bugs), and it's an easy fix.
Attachment #8725148 - Flags: review?(jdescottes)
Comment on attachment 8725148 [details] [diff] [review]
Bug_1252099_-_Do_not_use_CPOWs_in_browser_markup_c.diff

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

Looks good to me, thanks!

::: devtools/client/inspector/markup/test/doc_markup_image_and_canvas.html
@@ +4,5 @@
> +    <meta charset=utf-8 />
> +    <title>Image and Canvas markup-view test</title>
> +  </head>
> +  <body>
> +    <div></div>

ultra-nit: <img class="data" ...> the class "data" is not used in this test and should be removed.
Attachment #8725148 - Flags: review?(jdescottes) → review+
Keywords: leave-open
Many event-bubble tests had max-len issues that would have been really
awkward to fix by wrapping the lines. So I decided to disable eslint for
those lines instead.

This patch fixes the last remaining eslint issues and un-ignores the
directory in .eslintignore.

Review commit: https://reviewboard.mozilla.org/r/37595/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37595/
Attachment #8725688 - Flags: review?(mratcliffe)
(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #6)
> Created attachment 8725686 [details]
> MozReview Request: Bug 1252099 - Main eslint cleanup of markupview tests;
> r=zer0
> 
> Review commit: https://reviewboard.mozilla.org/r/37591/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/37591/
Sorry Matteo for the massive review, but these are only eslint fixes. Hopefully nothing too weird!
(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #7)
> Created attachment 8725687 [details]
> MozReview Request: Bug 1252099 - Remove usage of getNode and content in
> markupview tests; r=ochameau
> 
> Review commit: https://reviewboard.mozilla.org/r/37593/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/37593/
@Alex: as you will see, this is mostly a matter of using the TestActor everywhere in order to remove getNode and content.
Comment on attachment 8725687 [details]
MozReview Request: Bug 1252099 - Remove usage of getNode and content in markupview tests; r=ochameau

https://reviewboard.mozilla.org/r/37593/#review34147

I imagine try is going to catch more errors than me.

::: devtools/client/inspector/markup/test/browser_markup_html_edit_03.js:66
(Diff revision 1)
> +     "Escape cancels edits");

\o/ so easier to read!

::: devtools/client/inspector/markup/test/browser_markup_html_edit_03.js:100
(Diff revision 1)
> -  is(getNode("body").outerHTML, bodyHTML, "<body> HTML has been updated");
> +  let newBodyHTML = yield testActor.getProperty("body", "outerHTML");

You should re-query body.outerHTML instead of reusing currentBodyHTML!
Here you no longer assert if it changed or not.

::: devtools/client/inspector/markup/test/head.js:58
(Diff revision 1)
> -  content.location.reload();
> +  testActor.eval("content.location.reload()");

It might be worth to add a reload helper to testActor? Note that there already is a reloadFrame one to reload a given iframe.
We should be contributing very common patterns to TestActor instead of overusing eval().
Attachment #8725687 - Flags: review?(poirot.alex) → review+
Attachment #8725686 - Flags: review?(zer0)
Comment on attachment 8725686 [details]
MozReview Request: Bug 1252099 - Main eslint cleanup of markupview tests; r=zer0

https://reviewboard.mozilla.org/r/37591/#review34367

I didn't open as issue mostly any of the things I commented, due the fact that are basically a huge "use template string when possible" across the file. :) There is a typo, and a couple of suggestions, but they're mostly a kind of nice to have; so it's up to you.

::: devtools/client/inspector/markup/test/browser_markup_css_completion_style_attribute.js:66
(Diff revision 1)
> +  ["VK_RETURN", "style=\"display:  inherit; color :chartreuse !important;\"",

I have to say, I honestly find more readable the single quote in this specific case; I guess you've change that because eslint? What about using template string – even if there are no variables?

I'm not opening that as issue, I'm just thinking loud; there is nothing wrong with that, it's just that this changes is slightly less readable than the original code.

::: devtools/client/inspector/markup/test/browser_markup_css_completion_style_attribute.js:104
(Diff revision 1)
> -  info("Entering test data " + index + ": " + key + ", expecting: [" + TEST_DATA[index].slice(1) + "]");
> +  info("Entering test data " + index + ": " + key +

What about:
```js
let [key] = TEST_DATA[index];
let expected = TEST_DATA[index].slice(1);

info(`Entering test data ${index}: ${key}, expecting: [${expected}]`);
```

Also, not opened as issue, but I think when possible we should use template string now, to make them more readable, especially in unit test description.

::: devtools/client/inspector/markup/test/browser_markup_events.js:40
(Diff revision 1)
> -        handler: 'function mouseoverHandler(event) {\n' +
> +        handler: "function mouseoverHandler(event) {\n" +

If the string's indentation is not mandatory, use a template string for all the handlers in this file.

::: devtools/client/inspector/markup/test/browser_markup_events_jquery_1.0.js:227
(Diff revision 1)
>                   "  return returnValue;\n" +

We should definitely use template strings here too, but since you didn't change this part in the patch, and it's really a big part to change, maybe we can skip. :)

::: devtools/client/inspector/markup/test/browser_markup_keybindings_03.js:12
(Diff revision 1)
> +                 "<div class='test-class'></div>Text node";

Use a template string here.

::: devtools/client/inspector/markup/test/browser_markup_links_01.js:121
(Diff revision 1)
> -        is(linkEls[i].textContent, links[i].value, "Link " + i + " has the right value");
> +           "Link " + i + " has the right type");

I would use a template string here too, but since it's clear enough I don't mind; it's up to you.

::: devtools/client/inspector/markup/test/browser_markup_node_not_displayed_01.js:32
(Diff revision 1)
> -      "The container for " + selector + " is marked as displayed " + isDisplayed);
> +       "The container for " + selector + " is marked as displayed " +

Use template string here.

::: devtools/client/inspector/markup/test/browser_markup_tag_edit_01.js:26
(Diff revision 1)
> -  desc: 'Try changing an attribute to a quote (") - this should result ' +
> +  desc: "Try changing an attribute to a quote (\") - this should result " +

Not sure if it's make sense use template string, depends how `desc` is actually used.

::: devtools/client/inspector/markup/test/browser_markup_tag_edit_02.js:9
(Diff revision 1)
> -const TEST_URL = "data:text/html,<div id='test-div'>Test modifying my ID attribute</div>";
> +const TEST_URL = "data:text/html," +

Use template string here.

::: devtools/client/inspector/markup/test/browser_markup_tag_edit_03.js:9
(Diff revision 1)
> -const TEST_URL = "data:text/html;charset=utf-8,<div id='retag-me'><div id='retag-me-2'></div></div>";
> +const TEST_URL = "data:text/html;charset=utf-8," +

Ditto.

::: devtools/client/inspector/markup/test/browser_markup_tag_edit_06.js:74
(Diff revision 1)
> -  text: "onclick=\"javascript: throw new Error('wont fire');\" onload=\"alert('here');\"",
> +  text: "onclick=\"javascript: throw new Error('wont fire');\" " +

If possible, use template string here.

::: devtools/client/inspector/markup/test/browser_markup_tag_edit_07.js:34
(Diff revision 1)
> -  desc: 'Try add an attribute containing a quote (") attribute by ' +
> +  desc: "Try add an attribute containing a quote (\") attribute by " +

As before, if it's possible use template string it would be better; otherwise ignore this comment.

::: devtools/client/inspector/markup/test/browser_markup_tag_edit_07.js:44
(Diff revision 1)
> -  text: "style='"+DATA_URL_INLINE_STYLE+"'",
> +  text: "style='" + DATA_URL_INLINE_STYLE + "'",

use template string here.

::: devtools/client/inspector/markup/test/browser_markup_tag_edit_07.js:57
(Diff revision 1)
> -  text: 'data-long="'+LONG_ATTRIBUTE+'"',
> +  text: 'data-long="' + LONG_ATTRIBUTE + '"',

ditto.

::: devtools/client/inspector/markup/test/browser_markup_tag_edit_07.js:70
(Diff revision 1)
> -  text: 'src="'+DATA_URL_ATTRIBUTE+'"',
> +  text: 'src="' + DATA_URL_ATTRIBUTE + '"',

use template string here.

::: devtools/client/inspector/markup/test/browser_markup_tag_edit_08.js:45
(Diff revision 1)
> -  is (input.value, 'data-long="' + LONG_ATTRIBUTE + '"');
> +  is(input.value, 'data-long="' + LONG_ATTRIBUTE + '"');

use template string here

::: devtools/client/inspector/markup/test/browser_markup_tag_edit_10.js:34
(Diff revision 1)
> +     "The seelected node is now the SPAN");

typo: "The selected node"

::: devtools/client/inspector/markup/test/browser_markup_tag_edit_12.js:30
(Diff revision 1)
> -  info("Editing this attribute, keeping the same name, and tabbing to the next");
> +  info("Editing this attribute, keeping the same name, " +

Due the fact that `info` is dumped, I guess using template string would add weird spaces. For testing, it would be useful having a generic tag like:
```js
info(flat`Editing this attribute, keeping the same name,
          and tabbing to the next`);
```

Or something like that.

::: devtools/client/inspector/markup/test/browser_markup_tag_edit_13-other.js:9
(Diff revision 1)
> -const TEST_URL = "data:text/html;charset=utf8,<div a b id='order' c class></div>";
> +const TEST_URL = "data:text/html;charset=utf8," +

Use template string here.

::: devtools/client/inspector/markup/test/browser_markup_toggle_01.js:37
(Diff revision 1)
>        "li:nth-child(" + (i + 1) + ")", inspector);

use a template string here.

::: devtools/client/inspector/markup/test/browser_markup_toggle_02.js:28
(Diff revision 1)
>        "li:nth-child(" + (i + 1) + ")", inspector);

use a template string here.

::: devtools/client/inspector/markup/test/head.js:221
(Diff revision 1)
> -  info("Entering text '" + text + "' in node '" + selector + "''s new attribute field");
> +  info("Entering text '" + text + "' in node '" +

if possible, use a template string here.

::: devtools/client/inspector/markup/test/head.js:444
(Diff revision 1)
> -  EventUtils.sendKey("tab", inspector.panelWin); // next element
> +  EventUtils.sendKey("tab", inspector.panelWin);

since `inspector.panelWin` seems used a lot, maybe we could store it in a variable.

::: devtools/client/inspector/markup/test/head.js:544
(Diff revision 1)
> -  let button = 2;  // right click
> +  let button = 2;

What about calling it `buttonRight` or something similar, so it doesn't need the comment, 'cause it's more explict. It could be a `const`ant too.

::: devtools/client/inspector/markup/test/head.js:585
(Diff revision 1)
> -      return client.getTab().then(response => {
> +      return client.getTab().then(tabResponse => {

That's OK, but we could also do:
```js
return client.getTab().then(tabResponse => ({
  registrar: registrar,
  form: tabResponse.tab
}));
```
Comment on attachment 8725686 [details]
MozReview Request: Bug 1252099 - Main eslint cleanup of markupview tests; r=zer0

https://reviewboard.mozilla.org/r/37591/#review34419
Attachment #8725686 - Flags: review+
(In reply to Matteo Ferretti [:matteo][:zer0] from comment #13)
Thanks Matteo. I've changed many strings to template strings now. Much nicer in many places.

> ::: devtools/client/inspector/markup/test/browser_markup_events.js:40
> (Diff revision 1)
> > -        handler: 'function mouseoverHandler(event) {\n' +
> > +        handler: "function mouseoverHandler(event) {\n" +
> 
> If the string's indentation is not mandatory, use a template string for all
> the handlers in this file.
I originally did, and then realized that the test was failing because it actually checks for the handler's source as a string, so indentation/whitespaces are important here.

> :::
> devtools/client/inspector/markup/test/browser_markup_events_jquery_1.0.js:227
> (Diff revision 1)
> >                   "  return returnValue;\n" +
> 
> We should definitely use template strings here too, but since you didn't
> change this part in the patch, and it's really a big part to change, maybe
> we can skip. :)
Yeah, it's the same issue as just above though, if I do use a template string to make it more readable, then the test will fail.
I updated the 2 first patches to address the review comments. Here's a new try push with this:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc7aaec9c2aa&group_state=expanded&selectedJob=17547560
Comment on attachment 8725688 [details]
MozReview Request: Bug 1252099 - Final eslint cleanups in devtools/client/inspector/markup/test; r=miker

https://reviewboard.mozilla.org/r/37595/#review34535
Attachment #8725688 - Flags: review?(mratcliffe) → review+
Keywords: leave-open
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: