Closed
Bug 1252099
Opened 9 years ago
Closed 9 years ago
Stop using CPOWs in markup-view mochitests
Categories
(DevTools :: Inspector, defect, P1)
DevTools
Inspector
Tracking
(firefox47 fixed)
RESOLVED
FIXED
Firefox 47
| Tracking | Status | |
|---|---|---|
| firefox47 | --- | fixed |
People
(Reporter: pbro, Assigned: pbro)
References
Details
(Whiteboard: [btpp-fix-now])
Attachments
(4 files)
|
(deleted),
patch
|
jdescottes
:
review+
|
Details | Diff | Splinter Review |
|
(deleted),
text/x-review-board-request
|
zer0
:
review+
|
Details |
|
MozReview Request: Bug 1252099 - Remove usage of getNode and content in markupview tests; r=ochameau
(deleted),
text/x-review-board-request
|
ochameau
:
review+
|
Details |
|
(deleted),
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 | |
Updated•9 years ago
|
Assignee: nobody → pbrosset
Priority: -- → P1
|
|
Assignee | |
Updated•9 years ago
|
Whiteboard: [btpp-fix-now]
|
|
Assignee | |
Comment 1•9 years ago
|
||
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 2•9 years ago
|
||
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+
|
|
Assignee | |
Comment 3•9 years ago
|
||
|
|
Assignee | |
Updated•9 years ago
|
Keywords: leave-open
|
||
Comment 5•9 years ago
|
||
| bugherder | ||
|
|
Assignee | |
Comment 6•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/37591/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37591/
Attachment #8725686 -
Flags: review?(zer0)
|
|
Assignee | |
Comment 7•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/37593/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37593/
Attachment #8725687 -
Flags: review?(poirot.alex)
|
|
Assignee | |
Comment 8•9 years ago
|
||
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)
|
|
Assignee | |
Comment 9•9 years ago
|
||
(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!
|
|
Assignee | |
Comment 10•9 years ago
|
||
(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 11•9 years ago
|
||
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+
|
|
Assignee | |
Comment 12•9 years ago
|
||
|
||
Updated•9 years ago
|
Attachment #8725686 -
Flags: review?(zer0)
|
|
||
Comment 13•9 years ago
|
||
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 14•9 years ago
|
||
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+
|
|
Assignee | |
Comment 15•9 years ago
|
||
(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.
|
|
Assignee | |
Comment 16•9 years ago
|
||
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+
|
||
Comment 18•9 years ago
|
||
|
|
Assignee | |
Updated•9 years ago
|
Keywords: leave-open
|
||
Comment 19•9 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/6623a3cc2457
https://hg.mozilla.org/mozilla-central/rev/ea1e990cbed0
https://hg.mozilla.org/mozilla-central/rev/34c795c24d31
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
|
||
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•