Closed Bug 1273323 Opened 7 years ago Closed 7 years ago

Add integration tests for namespaced elements

Categories

(DevTools :: Inspector, enhancement)

Product:
DevTools
Component:
Inspector
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox49 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 49
Tracking Flags:
Tracking Status
firefox49 --- fixed

People

(Reporter: jdescottes, Assigned: nchevobbe)

Details

Attachments

(1 file)

MozReview Request: Bug 1273323 - Add integration tests for namespaced elements. r=jdescottes
58 bytes, text/x-review-board-request
jdescottes
: review+
Details 
Follow up to Bug 1270215. 

We should add tests to make sure that the tagnames of namespaced elements are properly handled and displayed in the devtools. 

They should always follow the pattern ${prefix}:${localName}
For instance "svg:svg" for a svg element embedded in a XHTML document.

data:application/xhtml+xml;charset=utf-8,<!DOCTYPE html><html xmlns="http://www.w3.org/1999/xhtml" xmlns:svg="http://www.w3.org/2000/svg"><body><svg:svg width="100" height="100"></svg:svg></body></html>

We lack tests for namespaced elements displayed in:
- the highlighter nodeinfobar
- the rule view
- the HTML search
- the markup view
Assignee: nobody → chevobbe.nicolas
Add tests for :
- markup display
- add rule
- breadcrumb
- highlighter info bar
- markup search
- webconsole element output

Review commit: https://reviewboard.mozilla.org/r/54756/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/54756/
Attachment #8755710 - Flags: review?(jdescottes)
Comment on attachment 8755710 [details]
MozReview Request: Bug 1273323 - Add integration tests for namespaced elements. r=jdescottes

https://reviewboard.mozilla.org/r/54756/#review51496

Thanks for adding the tests Nicolas!
No need for another round, but let me know if you want to discuss my comment about browser_webconsole_output_dom_elements_05.js

And of course needs a green try. Getting some average test running times for the new tests on Linux 32/64 debug would also be nice.

::: devtools/client/inspector/markup/test/browser.ini:105
(Diff revision 1)
>  [browser_markup_keybindings_04.js]
>  [browser_markup_keybindings_delete_attributes.js]
>  [browser_markup_keybindings_scrolltonode.js]
>  [browser_markup_mutation_01.js]
>  [browser_markup_mutation_02.js]
> -[browser_markup_node_names.js]
> +[browser_markup_namespaced_node_names.js]

nit: rename this test to browser_markup_node_names_namespaced to "regroup" it with browser_markup_node_names

::: devtools/client/inspector/rules/test/browser_rules_add-rule_06.js:8
(Diff revision 1)
> + http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";
> +
> +// Tests the behaviour of adding a new rule using the add rule button
> +// on namespaced elements

nit: punctuation, add a "." at the end of the sentence. (should be done for all test files)

::: devtools/client/inspector/rules/test/browser_rules_add-rule_06.js:47
(Diff revision 1)
> +});
> +
> +function* testNewRule(view, expected, index) {
> +  let idRuleEditor = getRuleViewRuleEditor(view, index);
> +  let editor = idRuleEditor.selectorText.ownerDocument.activeElement;
> +  is(editor.value, expected,

I am just wondering if we should not trigger an ESCAPE here and wait for the blur event of "editor"?
In this case we are not changing the value, so leaving the editor should not trigger any request, so it's up to you.

::: devtools/client/inspector/test/browser_inspector_breadcrumbs_namespaced.js:8
(Diff revision 1)
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +"use strict";
> +
> +// Test that the breadcrumbs widget content for namespaced elements is correct.
> +
> +const XHTML = `

Since we have 3 tests relying on a XHTML document here, we could have it as a support file ?

::: devtools/client/webconsole/test/browser.ini:362
(Diff revision 1)
>  [browser_webconsole_output_dom_elements_01.js]
>  [browser_webconsole_output_dom_elements_02.js]
>  skip-if = e10s # Bug 1042253 - webconsole e10s tests (Linux debug timeout)
>  [browser_webconsole_output_dom_elements_03.js]
>  [browser_webconsole_output_dom_elements_04.js]
> +[browser_webconsole_output_dom_elements_05.js]

The "skip-if" statements target the test above (not really intuitive!). So here the skip-if was targetting dom_elements_04.js, please move it accordingly.

Now concerning your new test, since it's heavily inspired from dom_elements_02 which has a similar skip-if, let's see how the test behaves on try to see if we have to add a skip as well.

::: devtools/client/webconsole/test/browser_webconsole_output_dom_elements_05.js:41
(Diff revision 1)
> +    output: '<svg:circle cx="0" cy="0" r="5">',
> +    displayName: "svg:circle"
> +  },
> +];
> +
> +function test() {

This test is not trivial and since it is almost entirely duplicated with browser_webconsole_output_dom_elements_02.js I think we should either:
- merge the 2 tests (might make it slower, so will need to pay attention to the test time on try on the slowest platforms)
- move the common methods to head.js or to a common helper? The core of the test (clicking on the icon, checking the highlighting) could also be extracted to a separate method
Attachment #8755710 - Flags: review?(jdescottes) → review+
https://reviewboard.mozilla.org/r/54756/#review51496

FYI, I already pushed it to TRY on only 1 platform (https://treeherder.mozilla.org/#/jobs?repo=try&revision=3dc5a0419c4f) to make sure everything was okay

> nit: rename this test to browser_markup_node_names_namespaced to "regroup" it with browser_markup_node_names

Oh okay, I though it was supposed to be alphabetically sorted. I'll changed it

> I am just wondering if we should not trigger an ESCAPE here and wait for the blur event of "editor"?
> In this case we are not changing the value, so leaving the editor should not trigger any request, so it's up to you.

I checked that they weren't any pending requests done. But maybe it would be more future-proof if I do it your way.

> Since we have 3 tests relying on a XHTML document here, we could have it as a support file ?

is there any advantage to have an html file ? I find it more difficult to understand a test at first glance if you have to go back and forth between the js test file and the associated html file. 
Also, having it like this makes it easier to edit the markup. If you have a support file, editing the markup could lead to some tests to fail. 
But maybe I'm missing something ?

> The "skip-if" statements target the test above (not really intuitive!). So here the skip-if was targetting dom_elements_04.js, please move it accordingly.
> 
> Now concerning your new test, since it's heavily inspired from dom_elements_02 which has a similar skip-if, let's see how the test behaves on try to see if we have to add a skip as well.

above ? wow, I thought it was only for the next line, not the previous one :) Thanks !

BTW, I don't know if you know this, but in reviewboard you can add a comment on multiple lines (click and drag from start line to end line )

> This test is not trivial and since it is almost entirely duplicated with browser_webconsole_output_dom_elements_02.js I think we should either:
> - merge the 2 tests (might make it slower, so will need to pay attention to the test time on try on the slowest platforms)
> - move the common methods to head.js or to a common helper? The core of the test (clicking on the icon, checking the highlighting) could also be extracted to a separate method

Like you said, we should pay attention to test time, and because this one and 02 are not really fast, I'm reluctant to merge them. But maybe I should just measure it.
https://reviewboard.mozilla.org/r/54756/#review51496

> Oh okay, I though it was supposed to be alphabetically sorted. I'll changed it

nevermind my comment, I misread it
Comment on attachment 8755710 [details]
MozReview Request: Bug 1273323 - Add integration tests for namespaced elements. r=jdescottes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54756/diff/1-2/
Comment on attachment 8755710 [details]
MozReview Request: Bug 1273323 - Add integration tests for namespaced elements. r=jdescottes

Pushed to TRY https://treeherder.mozilla.org/#/jobs?repo=try&revision=da6b8551a16e 

Julian, could you just quickly check that what I did for browser_webconsole_output_dom_elements_02.js and browser_webconsole_output_dom_elements_05.js was what you meant ? Thanks !
Attachment #8755710 - Flags: review+ → review?(jdescottes)
Comment on attachment 8755710 [details]
MozReview Request: Bug 1273323 - Add integration tests for namespaced elements. r=jdescottes

https://reviewboard.mozilla.org/r/54756/#review51704

Looks good, thanks Nicolas!

Only one small additional comment. I think we now have a lot of room for improvements in webconsole's head.js. 
The new helper we are adding here shares some similarities with checkOutputForInputs. But no need to tackle that in this bug.

::: devtools/client/webconsole/test/head.js:1606
(Diff revision 2)
> + *
> + *        - displayName: string, expected NodeFront's displayName.
> + *
> + *        - attr: Array, expected NodeFront's attributes
> + */
> +function checkDomElementsOutputForInputs(hud, inputs) {

nit: Most of this helper is testing the link to the inspector and the highlight of the node. The method name should hint at that.

checkDomElementHighlightingForInputs ? (I am notoriously bad at naming things, feel free to diverge!)
Attachment #8755710 - Flags: review?(jdescottes) → review+
Comment on attachment 8755710 [details]
MozReview Request: Bug 1273323 - Add integration tests for namespaced elements. r=jdescottes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54756/diff/2-3/
TRY https://treeherder.mozilla.org/#/jobs?repo=try&revision=da6b8551a16e  is over.
There are many failures, but none of theme involve one of the tests added or edited in my patch (there are mainly clipboard issues)

Pushed a new, rebased version (https://reviewboard.mozilla.org/r/54756/diff/2-3/), which only contains a function name change.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5040cc90f9fc
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox49: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
No longer depends on: 1276349
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.