988314 - Clean up the inspector tests

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Patrick Brosset <:pbro>]

See bug 968727's first comment for a description.
This bug should focus on the /browser/devtools/inspector/test folder only

Comment 1

[Patrick Brosset <:pbro>]

Some minor clean up started in bug 825410.
Also, I believe some of the tests in devtools/inspector/test ought to be moved to either devtools/styleinspector/test or devtools/markupview/test. browser_inspector_pseudoclass_lock.js is an example.

Comment 2

[Patrick Brosset <:pbro>]

Attachment: bug988314-move-browser_inspector_changes.js-to-styleinspector.patch

Moving one of the test from inspector to styleinspector, and splitting it in 3 parts.

Comment 3

[Patrick Brosset <:pbro>]

Attachment: bug988314-move-browser_inspector_changes.js-to-styleinspector.patch

I removed a skip-if line in browser.ini by mistake in the previous patch. Restored.

Comment 4

[Patrick Brosset <:pbro>]

Try push for attachment 8408947 [details] [diff] [review] : https://tbpl.mozilla.org/?tree=Try&rev=8e81ae47e38c

Comment 5

[Patrick Brosset <:pbro>]

Attachment: bug988314-move-browser_inspector_changes.js-to-styleinspector v2.patch

Thanks Joe for the review.
Just changed the patch title in this one.
Fixed in fx-team: https://hg.mozilla.org/integration/fx-team/rev/f57788b66994

Comment 6

[Patrick Brosset <:pbro>]

Leaving this bug open because there are a lot of other clean-ups to be done on the inspector tests.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f57788b66994

Comment 8

[Sami Jaktholm]

Attachment: inspector-test-cleanup-part-1.patch

Here's a work-in-progress patch I've been working on. It makes five tests to use Task.jsm and generators instead of the continue-on-callback process. I'd like to get some feedback on the methods and coding style used in the patch to see if I'm going to the right direction.

The patch cleans up following tests:
browser_inspector_breadcrumbs.js
browser_inspector_bug_650804_search.js
browser_inspector_bug_665880.js
browser_inspector_bug_672902_keyboard_shortcuts.js
browser_inspector_bug_674871.js

To actually run these you'll also need attached 'inspector-test-cleanup-part-0-head.patch' that contains changes of head.js required by the patched tests. However the tests should not contain any functional changes.

If the patch looks fine, I plan to continue porting the inspector tests to the new format. When all tests are ported, I'll post a separate patch for renaming some of the files and a cleaned up version of head.js.

Comment 9

[Sami Jaktholm]

Attachment: inspector-test-cleanup-part-0-head.patch

A patch required to run tests changed in inspector-test-cleanup-part-1.patch.

Comment 10

[Patrick Brosset <:pbro>]

Thanks for your interest on this bug! This is much needed.
I spent quite a lot of time on doing the same for the markupview and styleinspector tests and I know it takes time and patience to go through the whole list of tests (prepare yourself for a few rebases too as these tests change on a regular basis).
I'll do a quick review of the 2 patches, but so far they seem to look really good from what I've seen.
Do you want me to assign the bug to you?

Comment 11

[Patrick Brosset <:pbro>]

Comment on attachment 8450772 [details] [diff] [review]
inspector-test-cleanup-part-0-head.patch

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

Very nice cleanup of head.js, very happy with it!
See my comments below.

::: browser/devtools/inspector/test/head.js
@@ +47,5 @@
>    Services.prefs.clearUserPref("devtools.dump.emit");
>    Services.prefs.clearUserPref("devtools.inspector.activeSidebar");
>  });
>  
> +registerCleanupFunction(() => {

I'm pretty sure you know that, but just in case: doing this is great but means that you'll need to have 1 big patch ready at the end with all tests migrated.

@@ +48,5 @@
>    Services.prefs.clearUserPref("devtools.inspector.activeSidebar");
>  });
>  
> +registerCleanupFunction(() => {
> +  let TargetFactory = TargetFactory || devtools.TargetFactory;

Why not just 'TargetFactory'? Are there cases when it can be undefined?

@@ +63,5 @@
> +
> +  while (gBrowser.tabs.length > 1) {
> +    gBrowser.removeCurrentTab();
> +  }
> +

nit: empty line here not needed

@@ +141,5 @@
> +  let tab = yield addTab(url);
> +  let { inspector, toolbox } = yield openInspector();
> +  return { tab: tab,
> +           inspector: inspector,
> +           toolbox: toolbox };

It seems that shorthand properties in object literals have landed, so you could simply do:
return {tab, inspector, toolbox};

@@ +535,5 @@
> + * @param {String} eventName
> + * @param {Boolean} useCapture Optional, for addEventListener/removeEventListener
> + * @return A promise that resolves when the event has been handled
> + */
> +function once(target, eventName, useCapture=false) {

For information, :miker is about to add this method too in bug 1032721 which, from what it looks like, may land before this one.

@@ -514,5 @@
>  
> -/**
> - * Define an async test based on a generator function
> - */
> -function asyncTest(generator) {

omg this was defined twice! :|

@@ -523,5 @@
> - * Add a new test tab in the browser and load the given url.
> - * @param {String} url The url to be loaded in the new tab
> - * @return a promise that resolves to the tab object when the url is loaded
> - */
> -function addTab(url) {

This too!

Comment 12

[Patrick Brosset <:pbro>]

Comment on attachment 8450770 [details] [diff] [review]
inspector-test-cleanup-part-1.patch

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

It feels so good seeing lines of code being removed and tests being simplified. Thanks for working on this!

I don't have any particular remark at this stage, the changes look very good and the test functionalities seem to have been preserved.
My only comment is that I'd prefer test constants to be defined outside the test function (urls, data, ...).

If these tests pass, I'm fine with the changes so far.
Let me know when you want me to review other refactored tests.

I agree with submitting a separate patch for renaming tests. This can even be done later, after this one lands.

::: browser/devtools/inspector/test/browser_inspector_breadcrumbs.js
@@ +7,3 @@
>  
> +let test = asyncTest(function*() {
> +  const TEST_URI = TEST_URL_ROOT + "browser_inspector_breadcrumbs.html";

Although it's not a problem in this test, I think it makes sense to define test-global const in the global context (outside the test function) as we may end up needing them in other functions too.

Comment 13

[Patrick Brosset <:pbro>]

Comment on attachment 8410106 [details] [diff] [review]
bug988314-move-browser_inspector_changes.js-to-styleinspector v2.patch

This has already landed.

Comment 14

[Sami Jaktholm]

Thanks for the feedback. I'll start working on the rest of the tests and keep the comments in mind.

Comment 15

[Sami Jaktholm]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #11)
> ::: browser/devtools/inspector/test/head.js
> @@ +47,5 @@
> >    Services.prefs.clearUserPref("devtools.dump.emit");
> >    Services.prefs.clearUserPref("devtools.inspector.activeSidebar");
> >  });
> >  
> > +registerCleanupFunction(() => {
> 
> I'm pretty sure you know that, but just in case: doing this is great but
> means that you'll need to have 1 big patch ready at the end with all tests
> migrated.

The plan I had in mind is to port the tests in chunks of five tests per patch. This should ease the review process as already ported tests can be reviewed while rest of the tests are being ported. Once all tests are ported and reviews done the patches can be easily combined into a big one as the patches should not conflict with each other. How does that sound?

> @@ +48,5 @@
> >    Services.prefs.clearUserPref("devtools.inspector.activeSidebar");
> >  });
> >  
> > +registerCleanupFunction(() => {
> > +  let TargetFactory = TargetFactory || devtools.TargetFactory;
>
> Why not just 'TargetFactory'? Are there cases when it can be undefined?

Currently some tests nullify TargetFactory at the end causing issues. Once the tests are fixed I'll remove that.

Patch for head.js is picking up the pieces on the go and there's a lot of unnecessary glue to hold things together and keep all the tests passing the whole time.

Comment 16

[Patrick Brosset <:pbro>]

(In reply to Sami Jaktholm from comment #15)
> (In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #11)
> > ::: browser/devtools/inspector/test/head.js
> > @@ +47,5 @@
> > >    Services.prefs.clearUserPref("devtools.dump.emit");
> > >    Services.prefs.clearUserPref("devtools.inspector.activeSidebar");
> > >  });
> > >  
> > > +registerCleanupFunction(() => {
> > 
> > I'm pretty sure you know that, but just in case: doing this is great but
> > means that you'll need to have 1 big patch ready at the end with all tests
> > migrated.
> 
> The plan I had in mind is to port the tests in chunks of five tests per
> patch. This should ease the review process as already ported tests can be
> reviewed while rest of the tests are being ported. Once all tests are ported
> and reviews done the patches can be easily combined into a big one as the
> patches should not conflict with each other. How does that sound?
Perfect.

> > @@ +48,5 @@
> > >    Services.prefs.clearUserPref("devtools.inspector.activeSidebar");
> > >  });
> > >  
> > > +registerCleanupFunction(() => {
> > > +  let TargetFactory = TargetFactory || devtools.TargetFactory;
> >
> > Why not just 'TargetFactory'? Are there cases when it can be undefined?
> 
> Currently some tests nullify TargetFactory at the end causing issues. Once
> the tests are fixed I'll remove that.
> 
> Patch for head.js is picking up the pieces on the go and there's a lot of
> unnecessary glue to hold things together and keep all the tests passing the
> whole time.
Ok got it.

Comment 17

[Sami Jaktholm]

Attachment: inspector-test-cleanup-part-0-head.patch

Comment 18

[Sami Jaktholm]

Attachment: inspector-test-cleanup-part-1.patch

Here's four patches cleaning up 20 tests ready for review. Also the intermediate patch for head.js has been updated.

This patch cleans up following tests:
- browser_inspector_breadcrumbs.js
- browser_inspector_bug_650804_search.js
- browser_inspector_bug_665880.js
- browser_inspector_bug_672902_keyboard_shortcuts.js
- browser_inspector_bug_674871.js

Try run: https://tbpl.mozilla.org/?tree=Try&rev=c2b2c40592d4

Comment 19

[Sami Jaktholm]

Attachment: inspector-test-cleanup-part-2.patch

Here's a patch that cleans up following tests:
- browser_inspector_bug_699308_iframe_navigation.js
- browser_inspector_bug_817558_delete_node.js
- browser_inspector_bug_831693_combinator_suggestions.js
- browser_inspector_bug_831693_input_suggestion.js
- browser_inspector_bug_840156_destroy_after_navigation.js

Try run: https://tbpl.mozilla.org/?tree=Try&rev=c2b2c40592d4

Comment 20

[Sami Jaktholm]

Attachment: inspector-test-cleanup-part-3.patch

Here's a patch that cleans up following tests:
- browser_inspector_bug_848731_reset_selection_on_delete.js
- browser_inspector_bug_922125_destroy_on_navigate.js
- browser_inspector_bug_958169_switch_to_inspector_on_pick.js
- browser_inspector_bug_958456_highlight_comments.js
- browser_inspector_bug_961771_picker_stops_on_tool_select.js

Try run: https://tbpl.mozilla.org/?tree=Try&rev=c2b2c40592d4

Comment 21

[Sami Jaktholm]

Attachment: inspector-test-cleanup-part-4.patch

Here's a patch that cleans up following tests:
- browser_inspector_bug_962478_picker_stops_on_destroy.js
- browser_inspector_cmd_inspect.js
- browser_inspector_dead_node_exception.js
- browser_inspector_destroyselection.js
- browser_inspector_highlighter.js

Try run: https://tbpl.mozilla.org/?tree=Try&rev=c2b2c40592d4

Comment 22

[Sami Jaktholm]

Attachment: inspector-test-cleanup-part-0-head.patch

Here's a patch that cleans up head.js ready for review.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=17f7699ffe10

Comment 23

[Sami Jaktholm]

Attachment: inspector-test-cleanup-part-2.patch

Here's a revised version of inspector-test-cleanup-part-2.patch. It cleans up following tests:
- browser_inspector_bug_699308_iframe_navigation.js
- browser_inspector_bug_817558_delete_node.js
- browser_inspector_bug_831693_combinator_suggestions.js
- browser_inspector_bug_831693_input_suggestion.js
- browser_inspector_bug_840156_destroy_after_navigation.js

Try run: https://tbpl.mozilla.org/?tree=Try&rev=17f7699ffe10

Comment 24

[Sami Jaktholm]

Attachment: inspector-test-cleanup-part-4.patch

Here's a revised version of inspector-test-cleanup-part-4.patch. It cleans up following tests:
- browser_inspector_bug_962478_picker_stops_on_destroy.js
- browser_inspector_cmd_inspect.js
- browser_inspector_dead_node_exception.js
- browser_inspector_destroyselection.js
- browser_inspector_highlighter.js

Try run: https://tbpl.mozilla.org/?tree=Try&rev=17f7699ffe10

Comment 25

[Sami Jaktholm]

Attachment: inspector-test-cleanup-part-5.patch

Here's a patch that cleans up following tests:
- browser_inspector_iframeTest.js
- browser_inspector_initialization.js
- browser_inspector_invalidate.js
- browser_inspector_menu.js
- browser_inspector_navigation.js

Try run: https://tbpl.mozilla.org/?tree=Try&rev=17f7699ffe10

Comment 26

[Sami Jaktholm]

Attachment: inspector-test-cleanup-part-6.patch

Here's a patch that cleans up following tests:
- browser_inspector_pseudoclass_lock.js
- browser_inspector_pseudoClass_menu.js
- browser_inspector_reload.js
- browser_inspector_scrolling.js
- browser_inspector_select_last_selected.js
- browser_inspector_sidebarstate.js

Try run: https://tbpl.mozilla.org/?tree=Try&rev=17f7699ffe10

Comment 27

[Sami Jaktholm]

The seven patches that are marked for review clean up all but following test cases:
- browser_inspector_basic_highlighter.js: already in great shape, no need for clean up.
- browser_inspector_infobar.js: about to be cleaned up in bug 1032721.
- browser_inspector_bug_831693_searchbox_panel_navigation.js: about to be cleaned up in bug 851349.
- browser_inspector_tree_height.js: seems to be legacy test, doesn't run and I don't know what it is supposed to test. Should probably be removed completely.

Once the changes to the mentioned tests land I will refresh required patches of this bug but for now the patches are ready for review and feedback.

Comment 28

[Patrick Brosset <:pbro>]

Comment on attachment 8451334 [details] [diff] [review]
inspector-test-cleanup-part-0-head.patch

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

These changes to head.js look fine to me.
I was a bit surprised that you removed the openRuleView and openComputedView functions, but it looks like we don't really need them here anyway.

Comment 29

[Patrick Brosset <:pbro>]

Comment on attachment 8451238 [details] [diff] [review]
inspector-test-cleanup-part-1.patch

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

Very nice work. Thank you.
I've made a few comments, but R+ing this anyway, cause it looks good and these are mostly minor comments.
No need to ask for a new review for this after you've made changes to the patch.

::: browser/devtools/inspector/test/browser_inspector_breadcrumbs.js
@@ +5,2 @@
>  
> +// Test that breadcrumbs are working correctly.

Can I suggest that, while we're at it, we take the opportunity to add a little more details to the test description comments?
For this test, something like this would probably be better: "Test that the breadcrumbs widget content is correct".

@@ +35,3 @@
>    }
>  
> +  function performTest(node)

Not a big deal, but when a test only tests one thing (as it should), is short enough and doesn't require many helper functions, I think it makes sense to move the whole code in the test function, without defining other functions. Especially that this one is inside the test function scope and relies on its scoped variables.
What I'm saying is: remove the performTest function and move the code directly in the for ... of loop.

For small test cases, it usually helps with reading the code and somewhat prevents it from growing in the future.

::: browser/devtools/inspector/test/browser_inspector_bug_650804_search.js
@@ +5,2 @@
>  
> +// Test that searching works.

More something like: "Test that searching for nodes in the search field actually selects those nodes"

@@ +74,3 @@
>    }
>  
> +  function performChecks(state, id, isValid) {

Same comment as for the previous file, if it's just a short assertions function that's called from only one place, let's just move its code where it's called and get rid of the function.

::: browser/devtools/inspector/test/browser_inspector_bug_665880.js
@@ +8,2 @@
>  
> +const TEST_URI = "data:text/html," +

Please use data:text/html;charset=utf-8,

::: browser/devtools/inspector/test/browser_inspector_bug_672902_keyboard_shortcuts.js
@@ +5,5 @@
>  
>  // Tests that the keybindings for highlighting different elements work as
>  // intended.
>  
> +const TEST_URI = "data:text/html," +

data:text/html;charset=utf-8, here too.
I know this was here before, but since we're revisiting these tests anyway, it's probably a good time to do this.

@@ +13,3 @@
>  
> +const TEST_DATA = [
> +  { key: "VK_RIGHT",

nit: the formatting of this array is a little different to how we normally format array/object literals in our codebase. 
const TEST_DATA = [
  { key: "VK_RIGHT", selectedNode: "h1" }
  ...
];
or
const TEST_DATA = [
  {
    key: "VK_RIGHT",
    selectedNode: "h1"
  }
  ...
];

@@ +29,3 @@
>  
> +  let bc = inspector.breadcrumbs;
> +  bc.nodeHierarchy[bc.currentIndex].button.focus();

Reading the description, it's not obvious that this test is about the breadcrumbs widget in fact. Can you change the description comment?
Maybe add an 'info("Focusing the currently active breadcrumb button")', this would help reading the code.

::: browser/devtools/inspector/test/browser_inspector_bug_674871.js
@@ +29,2 @@
>  
> +const TEST_URI = "data:text/html;utf-8," + DOCUMENT_SRC;

data:text/html;charset=utf-8

Comment 30

[Patrick Brosset <:pbro>]

Comment on attachment 8451335 [details] [diff] [review]
inspector-test-cleanup-part-2.patch

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

Same as previous patch, R+ and no need to re-ask for review. Just a few minor comments.

::: browser/devtools/inspector/test/browser_inspector_bug_699308_iframe_navigation.js
@@ +5,2 @@
>  
> +// Test that highlighter doesn't break when iframe navigates.

Something like "Test that the highlighter element picker still works through iframe navigations" would be more appropriate.

::: browser/devtools/inspector/test/browser_inspector_bug_831693_combinator_suggestions.js
@@ +5,2 @@
>  
> +// Test to ensure combining selectors produce correct suggestions.

"Testing that searching for combining selectors using the inspector search field produces correct suggestions"

@@ +11,5 @@
> +// suggestions is an array of suggestions that should be shown in the popup.
> +// Suggestion is an object with label of the entry and optional count
> +// (defaults to 1)
> +const TEST_DATA = [
> +  { key: "d",

Same formatting comment as one I made in an earlier patch: 
{
  key: "d",
  suggestions: [{label: "div", count: 4}]
}

::: browser/devtools/inspector/test/browser_inspector_bug_831693_input_suggestion.js
@@ +5,2 @@
>  
> +// Test to ensure searching produces correct suggestions.

Same minor comments as for the previous file

::: browser/devtools/inspector/test/browser_inspector_bug_840156_destroy_after_navigation.js
@@ +5,1 @@
>  

Missing test description comment: "Testing that closing the inspector after navigating to a page doesn't fail".

Comment 31

[Patrick Brosset <:pbro>]

Comment on attachment 8451240 [details] [diff] [review]
inspector-test-cleanup-part-3.patch

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

Nice work here too.

::: browser/devtools/inspector/test/browser_inspector_bug_848731_reset_selection_on_delete.js
@@ +17,4 @@
>  
> +  yield testManuallyDeleteSelectedNode();
> +  yield testAutomaticallyDeleteSelectedNode();
> +  yield testDeleteSelectedNodeContainerFrame();

This test is a perfect candidate for splitting into 3 separate tests.
Not in this patch though, you've got enough on your plate as it is.
Can you make a note of it for later though? Maybe by filing a new bug, that depends on this one.

@@ +21,3 @@
>  
>    // 1 - select a node, right click, hit delete, verify that its parent is selected
>    // and that all other tools are fine

I think these "N - blah blah blah ..." comments and the following 'info(...)' statements should be merged into one 'info()' only.
My usual approach to comments in tests is:
- almost everytime we feel like adding a comment, it should be in a log statement, so it helps with debugging tests that fail (especially on other platforms, when all we have it logs)
- only when the comment is purely technical, or references a bug number or a file as a way to explain why something is done the way it is should the comment be a code comment '// ...'

@@ +29,2 @@
>  
> +    // Get the node container in the markup view

As per what I just said, I think we should revisit this patch to convert all these functional test step comments into 'info(...)' logs.

::: browser/devtools/inspector/test/browser_inspector_bug_922125_destroy_on_navigate.js
@@ +5,2 @@
>  
> +// Test that inspector handles page navigation correctly.

Well looking at the code, it looks like this is more of a markup-view test instead. So that comment should be changed. And furthermore, that test should be moved to the browser/devtools/markupview/test folder instead. Not in this patch though. Let's file another bug to do so later.

@@ +14,3 @@
>  
> +  let firstNode = getNode("#one");
> +  ok(firstNode, "We have the test node on page 1");

nit: I think this is an unrelated assertion that shouldn't really be in this test. It's not a problem if it is, but I think it would be better directly inside the 'getNode' helper function.

::: browser/devtools/inspector/test/browser_inspector_bug_958169_switch_to_inspector_on_pick.js
@@ +1,3 @@
>  /* vim: set ts=2 et sw=2 tw=80: */
>  /* Any copyright is dedicated to the Public Domain.
>   http://creativecommons.org/publicdomain/zero/1.0/ */

Missing test description comment: "Testing that clicking the pick button switches the toolbox to the inspector panel"

@@ +18,5 @@
> +
> +function openToolbox(tab) {
> +  info("Opening webconsole.");
> +  let target = TargetFactory.forTab(tab);
> +  return gDevTools.showToolbox(target, "webconsole");

If showToolbox returns a promise, then you can rewrite the function as:

function* openToolbox(tab) {
  info("Opening webconsole.");
  let target = TargetFactory.forTab(tab);
  yield gDevTools.showToolbox(target, "webconsole");
}

::: browser/devtools/inspector/test/browser_inspector_bug_958456_highlight_comments.js
@@ +26,2 @@
>  
> +  // It should be shown again when coming back to the same element tough

Can you change these comments into info() statements instead?

Comment 32

[Patrick Brosset <:pbro>]

Comment on attachment 8451336 [details] [diff] [review]
inspector-test-cleanup-part-4.patch

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

::: browser/devtools/inspector/test/browser_inspector_bug_962478_picker_stops_on_destroy.js
@@ +6,5 @@
>  
>  // Test that the highlighter's picker should be stopped when the toolbox is
>  // closed
>  
> +const TEST_URI = "data:text/html,<p>testing the highlighter goes away on destroy</p>";

data:text/html;charset=utf-8

@@ +15,4 @@
>  
> +  // Selecting a node and waiting for inspector-updated event gives the
> +  // inspector a chance to fully update its side-panels and therefore
> +  // avoids errors when removing the tab (due to ongoing requests)

We now use 'selectNode' consistently through the tests, and it always waits for that event, so this whole comment is unnecessary at best, misleading otherwise, cause it may make the reader feel something's missing here.

::: browser/devtools/inspector/test/browser_inspector_cmd_inspect.js
@@ +5,2 @@
>  
>  // Tests that the inspect command works as it should

Something more specific would help: "Testing that the gcli 'inspect' command works as it should"

::: browser/devtools/inspector/test/browser_inspector_dead_node_exception.js
@@ +5,1 @@
>  

Missing test description comment: "Testing that the inspector doesn't go blank when navigating to a page that deletes an iframe while loading"

::: browser/devtools/inspector/test/browser_inspector_destroyselection.js
@@ +17,3 @@
>  
> +  info("Removing iframe.");
> +  iframe.parentNode.removeChild(iframe);

iframe.remove() is enough

Comment 33

[Patrick Brosset <:pbro>]

(In reply to Sami Jaktholm from comment #27)
> - browser_inspector_tree_height.js: seems to be legacy test, doesn't run and
> I don't know what it is supposed to test. Should probably be removed
> completely.
Yeah, please go ahead and delete the file.

Comment 34

[Sami Jaktholm]

Attachment: inspector-test-cleanup-part-1.patch

A revised version of inspector-test-cleanup-part-1.patch that addresses the review comments.

Comment 35

[Sami Jaktholm]

Attachment: inspector-test-cleanup-part-2.patch

A revised version of inspector-test-cleanup-part-2.patch that addresses the review comments.

Comment 36

[Sami Jaktholm]

Attachment: inspector-test-cleanup-part-3.patch

A revised version of inspector-test-cleanup-part-3.patch that addresses the review comments.

Comment 37

[Sami Jaktholm]

Attachment: inspector-test-cleanup-part-4.patch

A revised version of inspector-test-cleanup-part-4.patch that addresses the review comments.

Comment 38

[Patrick Brosset <:pbro>]

Comment on attachment 8451337 [details] [diff] [review]
inspector-test-cleanup-part-5.patch

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

::: browser/devtools/inspector/test/browser_inspector_iframeTest.js
@@ +7,1 @@
>  

Missing test description comment: "Testing that moving the mouse over the document with the element picker started highlights nodes"

@@ +7,2 @@
>  
> +const NESTED_FRAME_SRC = "data:text/html,nested iframe<div>nested div</div>";

utf8

@@ +7,3 @@
>  
> +const NESTED_FRAME_SRC = "data:text/html,nested iframe<div>nested div</div>";
> +const OUTER_FRAME_SRC = "data:text/html,little frame<div>little div</div>" +

utf8

@@ +7,5 @@
>  
> +const NESTED_FRAME_SRC = "data:text/html,nested iframe<div>nested div</div>";
> +const OUTER_FRAME_SRC = "data:text/html,little frame<div>little div</div>" +
> +  "<iframe src='" + NESTED_FRAME_SRC + "' />";
> +const TEST_URI = "data:text/html,iframe tests for inspector" +

utf8

@@ +13,5 @@
>  
> +let test = asyncTest(function*() {
> +  let { inspector } = yield openInspectorForURL(TEST_URI);
> +  let outerFrame = getNode("iframe");
> +  let outerFrameDiv = outerFrame.contentDocument.querySelector("div");

I think we should add a second argument to 'getNode' which would be the parent document.
By default it do what it does today, but it would be possible to pass in other documents.

::: browser/devtools/inspector/test/browser_inspector_initialization.js
@@ +109,5 @@
>    ok(button, "A crumbs is checked=true");
>    is(button.getAttribute("tooltiptext"), expectedText, "Crumb refers to the right node");
>  }
>  
> +function* _clickOnInspectMenuItem(node) {

We don't really need to differentiate between public and private functions in test, I guess we can remove the _ prefix here.

::: browser/devtools/inspector/test/browser_inspector_menu.js
@@ +5,1 @@
>  

Missing description comment, having said this, this test does so many things that coming up with one short comment would be hard. A good sign that this one should be split up too! Could you log a bug for doing this?
I think we should have a test that only checks for existence and activate of menu items, and at least another one that checks that the items actually do what they say they do.

::: browser/devtools/inspector/test/browser_inspector_navigation.js
@@ +7,1 @@
>  

Missing description comment.

Comment 39

[Sami Jaktholm]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #31)
> Comment on attachment 8451240 [details] [diff] [review]
> inspector-test-cleanup-part-3.patch
> 
> Review of attachment 8451240 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice work here too.
> 
> :::
> browser/devtools/inspector/test/
> browser_inspector_bug_848731_reset_selection_on_delete.js
> @@ +17,4 @@
> >  
> > +  yield testManuallyDeleteSelectedNode();
> > +  yield testAutomaticallyDeleteSelectedNode();
> > +  yield testDeleteSelectedNodeContainerFrame();
> 
> This test is a perfect candidate for splitting into 3 separate tests.
> Not in this patch though, you've got enough on your plate as it is.
> Can you make a note of it for later though? Maybe by filing a new bug, that
> depends on this one.

Filed bug 1035121.

> browser/devtools/inspector/test/
> browser_inspector_bug_922125_destroy_on_navigate.js
> @@ +5,2 @@
> >  
> > +// Test that inspector handles page navigation correctly.
> 
> Well looking at the code, it looks like this is more of a markup-view test
> instead. So that comment should be changed. And furthermore, that test
> should be moved to the browser/devtools/markupview/test folder instead. Not
> in this patch though. Let's file another bug to do so later.

Filed bug 1035124.

> @@ +14,3 @@
> >  
> > +  let firstNode = getNode("#one");
> > +  ok(firstNode, "We have the test node on page 1");
> 
> nit: I think this is an unrelated assertion that shouldn't really be in this
> test. It's not a problem if it is, but I think it would be better directly
> inside the 'getNode' helper function.

Some tests use the getNode to check that a node with given selector doesn't exist so adding a check into getNode would make it a lot more complicated.

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #33)
> (In reply to Sami Jaktholm from comment #27)
> > - browser_inspector_tree_height.js: seems to be legacy test, doesn't run and
> > I don't know what it is supposed to test. Should probably be removed
> > completely.
> Yeah, please go ahead and delete the file.

Created a new bug that contains a patch and a try run for removing that file. See bug 1035126.

Comment 40

[Sami Jaktholm]

Attachment: inspector-test-cleanup-part-0-head.patch

(In reply to Sami Jaktholm from comment #39)
> (In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #31)
> > @@ +14,3 @@
> > >  
> > > +  let firstNode = getNode("#one");
> > > +  ok(firstNode, "We have the test node on page 1");
> > 
> > nit: I think this is an unrelated assertion that shouldn't really be in this
> > test. It's not a problem if it is, but I think it would be better directly
> > inside the 'getNode' helper function.
> 
> Some tests use the getNode to check that a node with given selector doesn't
> exist so adding a check into getNode would make it a lot more complicated.

Scratch that. Here's a version of head.js that has a getNode method that takes a second parameter "options" which is a object { exceptNoMatches: false, document: content.document } by default. document specifies the HTMLDocument object that should be queried. If exceptNoMatches is false, a failure occurs if nothing matches the selector. If exceptNoMatches is true, test fails if a match is found.

Comment 41

[Sami Jaktholm]

Attachment: inspector-test-cleanup-part-1.patch

Here's a version adapted to the new functionality of getNode.

Comment 42

[Sami Jaktholm]

Attachment: inspector-test-cleanup-part-2.patch

Here's a version adapted to the new functionality of getNode.

Comment 43

[Sami Jaktholm]

Attachment: inspector-test-cleanup-part-3.patch

Here's a version adapted to the new functionality of getNode.

Comment 44

[Sami Jaktholm]

Attachment: inspector-test-cleanup-part-4.patch

Here's a version adapted to the new functionality of getNode.

Comment 45

[Sami Jaktholm]

Attachment: inspector-test-cleanup-part-5.patch

A revised version of inspector-test-cleanup-part-5.patch that addresses the comments in the review and adapts to the new getNode functionality of head.js.

Comment 46

[Patrick Brosset <:pbro>]

Comment on attachment 8451574 [details] [diff] [review]
inspector-test-cleanup-part-0-head.patch

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

::: browser/devtools/inspector/test/head.js
@@ +104,5 @@
> + *        An object containing any of the following options:
> + *        - document: HTMLDocument that should be queried for the selector.
> + *                    Default: content.document.
> + *        - expectNoMatch: If false and node is not found, a failure is logged.
> + *                         Default: true.

Didn't you mean the opposite? If true, we expect no node to match the selector and so we will log an failure if we do find one. And this defaults to false, not to true.

@@ +109,3 @@
>   * @return {DOMNode}
>   */
> +function getNode(nodeOrSelector, options) {

function getNode(nodeOrSelector, options={}) {...}
This avoids the following line.

Comment 47

[Patrick Brosset <:pbro>]

(In reply to Sami Jaktholm from comment #41)
> Created attachment 8451575 [details] [diff] [review]
> inspector-test-cleanup-part-1.patch
> 
> Here's a version adapted to the new functionality of getNode.
You know you can R+ the new versions of part 1/2/3/4/5 since I've R+'d them already.
As long as what you change is just addressing my comments (which were minor btw), it's fine to carry the R+ over to the new version.

Comment 48

[Sami Jaktholm]

Attachment: inspector-test-cleanup-part-0-head.patch

Here's a patch with a fixed and hopefully a little bit clearer docstring for getNode.

Comment 49

[Patrick Brosset <:pbro>]

Comment on attachment 8451735 [details] [diff] [review]
inspector-test-cleanup-part-0-head.patch

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

I like the new docstring. Thanks

Comment 50

[Patrick Brosset <:pbro>]

Comment on attachment 8451338 [details] [diff] [review]
inspector-test-cleanup-part-6.patch

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

These changes look good too! Nice work again.

::: browser/devtools/inspector/test/browser_inspector_scrolling.js
@@ +7,1 @@
>  

Is it me or is this test completely useless? I seem to remember it used to actually test something useful, but it looks like now it only scrolls an iframe and then asserts that the iframe has scrolled. It doesn't check that the highlighter has scrolled too, which is what I used to do I think.

Can you add a comment about this here? So we can address this later in another bug.

@@ +19,5 @@
>  
> +  let iframe = getNode("iframe");
> +  let div = iframe.contentDocument.querySelector("div");
> +  ok(iframe, "<iframe> found.");
> +  ok(div, "<div> found from <iframe>.");

With the latest head.js changes you made, I think you can simplify these 4 lines.

::: browser/devtools/inspector/test/browser_inspector_select_last_selected.js
@@ +2,5 @@
>  /* vim: set ts=2 et sw=2 tw=80: */
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  

Description comment: "Checks that the expected default node is selected after a page navigation or a reload"

@@ +12,5 @@
> +// - nodeToSelect: a selector for a node to select before navigation. If null,
> +//                 whatever is selected stays selected.
> +// - selectedNode: a selector for a node that is selected after navigation.
> +let TEST_DATA = [
> +  { url: PAGE_1,

nit: formatting of this object literal

Comment 51

[Sami Jaktholm]

Attachment: inspector-test-cleanup-part-6.patch

A patch with review comments addressed.

Comment 52

[Sami Jaktholm]

Attachment: inspector-test-cleanup-combined.patch

Here's a patch that combines the changes from inspector-test-cleanup-part-[0-6] patches.

A try run with the combined patch: https://tbpl.mozilla.org/?tree=Try&rev=cc5d60019b5b

I'll run the devtools tests couple of times on try to see how are things going.

Comment 53

[Patrick Brosset <:pbro>]

The try build is still pending but is already pretty green. I think we have enough retriggers already to be pretty confident that your refactor didn't introduce any new intermittent bug. Let's wait until we have green dt test results on all platforms and then land this.

Comment 54

[Sami Jaktholm]

Try run is green: https://tbpl.mozilla.org/?tree=Try&rev=cc5d60019b5b

inspector-test-cleanup-combined.patch is ready to land.

Comment 55

[Patrick Brosset <:pbro>]

Comment on attachment 8452212 [details] [diff] [review]
inspector-test-cleanup-combined.patch

R+ this folded patch since all sub-patches were R+

Comment 56

[Carsten Book [:Tomcat]]

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

Comment 57

[Patrick Brosset <:pbro>]

Oops, let's leave-open for now since we want to use this bug for renaming files too.

Comment 58

[Sami Jaktholm]

Attachment: inspector-test-cleanup-rename.patch

Here's a patch renaming files in inspector/test/. It does the following:
- renames supporting documents browser_inspector_*.html --> doc_inspector_*.html
- removes bug numbers from file names (should they be added to the files instead?)
- renames some tests to better reflect their functionality
- update references to renamed files in tests and browser.ini

Comment 59

[Patrick Brosset <:pbro>]

(In reply to Sami Jaktholm from comment #58)
> Created attachment 8453030 [details] [diff] [review]
> inspector-test-cleanup-rename.patch
> 
> Here's a patch renaming files in inspector/test/. It does the following:
> - renames supporting documents browser_inspector_*.html -->
> doc_inspector_*.html
Good, this way it's consistent with the other devtools test that have been refactored
> - removes bug numbers from file names (should they be added to the files
> instead?)
It depends ... some test titles have the number of the bug a particular feature was implemented in. For these ones, I don't think we need to worry about just removing the number completely. Other tests were added as a result of a regression and the number of that regression bug was used as the file name. For these ones, I think it makes sense to add the number in the first description comment in the file and get rid of it in the filename.
> - renames some tests to better reflect their functionality
Good!
> - update references to renamed files in tests and browser.ini

Comment 60

[Patrick Brosset <:pbro>]

Comment on attachment 8453030 [details] [diff] [review]
inspector-test-cleanup-rename.patch

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

These changes look good.
Let's not worry about bug numbers at all, even for tests that were added after regressions. Indeed, hard to know which ones have, and I don't think it adds much benefit anyway.

Comment 61

[Sami Jaktholm]

Comment on attachment 8453030 [details] [diff] [review]
inspector-test-cleanup-rename.patch

Thanks for the feedback. This is be ready to be reviewed then.

Try: https://tbpl.mozilla.org/?tree=Try&rev=8d219c60d235

Comment 62

[Patrick Brosset <:pbro>]

Comment on attachment 8453030 [details] [diff] [review]
inspector-test-cleanup-rename.patch

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

R+ assuming a green try push.

Comment 63

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d5a606989ab9

Comment 64

[Sami Jaktholm]

Try run mostly green, only few unrelated known timeouts. Patch inspector-test-cleanup-rename.patch is ready to be checked in.

Comment 65

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/68d12ca186a7

Comment 66

[Patrick Brosset <:pbro>]

Nice work Sami. Thanks for that!

Comment 67

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/68d12ca186a7