Clean up the inspector tests

RESOLVED FIXED in Firefox 33

Status

defect
RESOLVED FIXED
5 years ago
Last year

People

(Reporter: pbro, Assigned: sjakthol)

Tracking

(Blocks 1 bug)

unspecified
Firefox 33
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 26 obsolete attachments)

17.85 KB, patch
pbro
: review+
pbro
: checkin+
Details | Diff | Splinter Review
159.44 KB, patch
pbro
: review+
sjakthol
: checkin+
Details | Diff | Splinter Review
30.93 KB, patch
pbro
: review+
pbro
: feedback+
Details | Diff | Splinter Review
Reporter

Description

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

Updated

5 years ago
See Also: → 968727
Reporter

Updated

5 years ago
See Also: → 988313
Reporter

Comment 1

5 years ago
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.
Reporter

Comment 2

5 years ago
Moving one of the test from inspector to styleinspector, and splitting it in 3 parts.
Assignee: nobody → pbrosset
Reporter

Comment 3

5 years ago
I removed a skip-if line in browser.ini by mistake in the previous patch. Restored.
Attachment #8408945 - Attachment is obsolete: true
Reporter

Updated

5 years ago
Attachment #8408947 - Flags: review?(jwalker)
Attachment #8408947 - Flags: review?(jwalker) → review+
Reporter

Comment 5

5 years ago
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
Attachment #8408947 - Attachment is obsolete: true
Attachment #8410106 - Flags: review+
Reporter

Comment 6

5 years ago
Leaving this bug open because there are a lot of other clean-ups to be done on the inspector tests.
Keywords: leave-open
Whiteboard: [fixed-in-fx-team]
Assignee

Comment 8

5 years ago
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.
Attachment #8450770 - Flags: feedback?(pbrosset)
Assignee

Comment 9

5 years ago
A patch required to run tests changed in inspector-test-cleanup-part-1.patch.
Reporter

Comment 10

5 years ago
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?
Assignee: pbrosset → nobody
Reporter

Comment 11

5 years ago
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!
Attachment #8450772 - Flags: feedback+
Reporter

Comment 12

5 years ago
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.
Attachment #8450770 - Flags: feedback?(pbrosset) → feedback+
Reporter

Comment 13

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

This has already landed.
Attachment #8410106 - Flags: checkin+
Assignee

Comment 14

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

Comment 15

5 years ago
(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.
Assignee: nobody → sjakthol
Reporter

Comment 16

5 years ago
(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.
Assignee

Comment 17

5 years ago
Attachment #8450772 - Attachment is obsolete: true
Assignee

Comment 18

5 years ago
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
Attachment #8450770 - Attachment is obsolete: true
Attachment #8451238 - Flags: review?(pbrosset)
Assignee

Comment 19

5 years ago
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
Attachment #8451239 - Flags: review?(pbrosset)
Assignee

Comment 20

5 years ago
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
Attachment #8451240 - Flags: review?(pbrosset)
Assignee

Comment 21

5 years ago
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
Attachment #8451241 - Flags: review?(pbrosset)
Assignee

Comment 22

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

Try run: https://tbpl.mozilla.org/?tree=Try&rev=17f7699ffe10
Attachment #8451237 - Attachment is obsolete: true
Attachment #8451334 - Flags: review?(pbrosset)
Assignee

Comment 23

5 years ago
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
Attachment #8451239 - Attachment is obsolete: true
Attachment #8451239 - Flags: review?(pbrosset)
Attachment #8451335 - Flags: review?(pbrosset)
Assignee

Comment 24

5 years ago
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
Attachment #8451241 - Attachment is obsolete: true
Attachment #8451241 - Flags: review?(pbrosset)
Attachment #8451336 - Flags: review?(pbrosset)
Assignee

Comment 25

5 years ago
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
Attachment #8451337 - Flags: review?(pbrosset)
Assignee

Comment 26

5 years ago
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
Attachment #8451338 - Flags: review?(pbrosset)
Assignee

Comment 27

5 years ago
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.
Reporter

Comment 28

5 years ago
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.
Attachment #8451334 - Flags: review?(pbrosset) → review+
Reporter

Comment 29

5 years ago
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
Attachment #8451238 - Flags: review?(pbrosset) → review+
Reporter

Comment 30

5 years ago
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".
Attachment #8451335 - Flags: review?(pbrosset) → review+
Reporter

Comment 31

5 years ago
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?
Attachment #8451240 - Flags: review?(pbrosset) → review+
Reporter

Comment 32

5 years ago
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
Attachment #8451336 - Flags: review?(pbrosset) → review+
Reporter

Comment 33

5 years ago
(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.
Assignee

Comment 34

5 years ago
A revised version of inspector-test-cleanup-part-1.patch that addresses the review comments.
Attachment #8451238 - Attachment is obsolete: true
Assignee

Comment 35

5 years ago
A revised version of inspector-test-cleanup-part-2.patch that addresses the review comments.
Attachment #8451335 - Attachment is obsolete: true
Assignee

Comment 36

5 years ago
A revised version of inspector-test-cleanup-part-3.patch that addresses the review comments.
Attachment #8451240 - Attachment is obsolete: true
Assignee

Comment 37

5 years ago
A revised version of inspector-test-cleanup-part-4.patch that addresses the review comments.
Attachment #8451336 - Attachment is obsolete: true
Reporter

Updated

5 years ago
Attachment #8451516 - Flags: review+
Reporter

Updated

5 years ago
Attachment #8451517 - Flags: review+
Reporter

Updated

5 years ago
Attachment #8451518 - Flags: review+
Reporter

Updated

5 years ago
Attachment #8451519 - Flags: review+
Assignee

Updated

5 years ago
Blocks: 1035121
Assignee

Updated

5 years ago
Blocks: 1035124
Reporter

Comment 38

5 years ago
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.
Attachment #8451337 - Flags: review?(pbrosset) → review+
Assignee

Comment 39

5 years ago
(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.
Assignee

Comment 40

5 years ago
(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.
Attachment #8451334 - Attachment is obsolete: true
Attachment #8451574 - Flags: review?(pbrosset)
Assignee

Comment 41

5 years ago
Here's a version adapted to the new functionality of getNode.
Attachment #8451516 - Attachment is obsolete: true
Assignee

Comment 42

5 years ago
Here's a version adapted to the new functionality of getNode.
Attachment #8451517 - Attachment is obsolete: true
Assignee

Comment 43

5 years ago
Here's a version adapted to the new functionality of getNode.
Attachment #8451518 - Attachment is obsolete: true
Assignee

Comment 44

5 years ago
Here's a version adapted to the new functionality of getNode.
Attachment #8451519 - Attachment is obsolete: true
Assignee

Comment 45

5 years ago
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.
Attachment #8451337 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Blocks: 1035140
Reporter

Comment 46

5 years ago
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.
Attachment #8451574 - Flags: review?(pbrosset)
Reporter

Comment 47

5 years ago
(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.
Assignee

Updated

5 years ago
Attachment #8451575 - Flags: review+
Assignee

Updated

5 years ago
Attachment #8451576 - Flags: review+
Assignee

Updated

5 years ago
Attachment #8451577 - Flags: review+
Assignee

Updated

5 years ago
Attachment #8451578 - Flags: review+
Assignee

Updated

5 years ago
Attachment #8451581 - Flags: review+
Assignee

Comment 48

5 years ago
Here's a patch with a fixed and hopefully a little bit clearer docstring for getNode.
Attachment #8451574 - Attachment is obsolete: true
Attachment #8451735 - Flags: review?(pbrosset)
Reporter

Comment 49

5 years ago
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
Attachment #8451735 - Flags: review?(pbrosset) → review+
Reporter

Comment 50

5 years ago
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
Attachment #8451338 - Flags: review?(pbrosset) → review+
Reporter

Updated

5 years ago
Blocks: 1035661
Assignee

Comment 51

5 years ago
A patch with review comments addressed.
Attachment #8451338 - Attachment is obsolete: true
Attachment #8452199 - Flags: review+
Assignee

Comment 52

5 years ago
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.
Attachment #8451575 - Attachment is obsolete: true
Attachment #8451576 - Attachment is obsolete: true
Attachment #8451577 - Attachment is obsolete: true
Attachment #8451578 - Attachment is obsolete: true
Attachment #8451581 - Attachment is obsolete: true
Attachment #8451735 - Attachment is obsolete: true
Attachment #8452199 - Attachment is obsolete: true
Reporter

Comment 53

5 years ago
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.
Assignee

Comment 54

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

inspector-test-cleanup-combined.patch is ready to land.
Keywords: checkin-needed
Reporter

Comment 55

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

R+ this folded patch since all sub-patches were R+
Attachment #8452212 - Flags: review+
Reporter

Updated

5 years ago
Keywords: leave-open
Reporter

Comment 57

5 years ago
Oops, let's leave-open for now since we want to use this bug for renaming files too.
Keywords: leave-open
Assignee

Comment 58

5 years ago
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
Attachment #8453030 - Flags: feedback?(pbrosset)
Reporter

Comment 59

5 years ago
(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
Reporter

Comment 60

5 years ago
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.
Attachment #8453030 - Flags: feedback?(pbrosset) → feedback+
Assignee

Comment 61

5 years ago
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
Attachment #8453030 - Flags: review?(pbrosset)
Reporter

Comment 62

5 years ago
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.
Attachment #8453030 - Flags: review?(pbrosset) → review+
https://hg.mozilla.org/mozilla-central/rev/d5a606989ab9
Flags: in-testsuite-
Whiteboard: [fixed-in-fx-team]
Assignee

Comment 64

5 years ago
Try run mostly green, only few unrelated known timeouts. Patch inspector-test-cleanup-rename.patch is ready to be checked in.
Keywords: checkin-needed
Assignee

Updated

5 years ago
Attachment #8452212 - Flags: checkin+
Reporter

Comment 66

5 years ago
Nice work Sami. Thanks for that!
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/68d12ca186a7
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 33

Updated

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