585043 - Improve Inspector DOM panel unittests

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Rob Campbell [:rc] (:robcee)]

following up from review comment bug 561782c#18, we need to track the observed properties of an inspected object in a unittest to verify that the contents are correct and not missing anything.

This may already be implemented as part of the PropertyPanel.jsm tests Julian added, but wanted to write this to capture it.

Comment 1

[Mihai Sucan [:msucan]]

Attachment: proposed fix

Proposed fix. As discussed, this patch is on top of the tree panel patches.

Comment 2

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 467813 [details] [diff] [review]
proposed fix

     gBrowser.tabContainer.addEventListener("TabSelect", this, false);
-    this.toolsInspectCmd.setAttribute("checked", true);
+    this.inspectCmd.setAttribute("checked", true);

unrelated to this bug?

-    toolsInspectCmd.setAttribute("checked", "false");
+    this.inspectCmd.setAttribute("checked", "false");

also.

-  let closing = doc.getElementById("#closing");
+
+  let closing = doc.getElementById("closing");
+  newProperty = "bazbaz" + Date.now();

ouch. That may have been fixed elsewhere, but good to have it just in case.

     testGen.next();
-  },
+  }
+};

customary to leave trailing commas in object method declarations.

In this case, I'd prefer to see the observe: method removed entirely and runDOMTests converted to a function. We can do that now for notification observers.

rest looks good. feedback- because of the above.

Comment 3

[Mihai Sucan [:msucan]]

(In reply to comment #2)
> Comment on attachment 467813 [details] [diff] [review]
> proposed fix
> 
>      gBrowser.tabContainer.addEventListener("TabSelect", this, false);
> -    this.toolsInspectCmd.setAttribute("checked", true);
> +    this.inspectCmd.setAttribute("checked", true);
> 
> unrelated to this bug?
> 
> -    toolsInspectCmd.setAttribute("checked", "false");
> +    this.inspectCmd.setAttribute("checked", "false");
> 
> also.

Hehe, both fixed the failures I was having with the older tree panel code. These two little buggers caused problems for me.

> -  let closing = doc.getElementById("#closing");
> +
> +  let closing = doc.getElementById("closing");
> +  newProperty = "bazbaz" + Date.now();
> 
> ouch. That may have been fixed elsewhere, but good to have it just in case.

Yes, same with this.

>      testGen.next();
> -  },
> +  }
> +};
> 
> customary to leave trailing commas in object method declarations.
> 
> In this case, I'd prefer to see the observe: method removed entirely and
> runDOMTests converted to a function. We can do that now for notification
> observers.

Indeed. When I wrote the patch I didn't know we can do this.

Will update and rebase the patch. Thanks for your feedback!

Comment 4

[Mihai Sucan [:msucan]]

Attachment: [checked-in] rebased patch

Updated and rebased patch, as requested. Thanks Robert!

Comment 5

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 475914 [details] [diff] [review]
[checked-in] rebased patch

looks good

Comment 6

[Mihai Sucan [:msucan]]

Comment on attachment 475914 [details] [diff] [review]
[checked-in] rebased patch

Thanks for your feedback+ Robert! Asking for review.

Comment 7

[Mihai Sucan [:msucan]]

Comment on attachment 475914 [details] [diff] [review]
[checked-in] rebased patch

Thanks Dietrich for your review+! Asking for approval2.0+ to include this test in mozilla-central.

Comment 8

[Rob Campbell [:rc] (:robcee)]

we can't block on this, but would appreciate approval to land these test enhancements. Moar tests!

Comment 9

[Dietrich Ayala (:dietrich)]

Comment on attachment 475914 [details] [diff] [review]
[checked-in] rebased patch

a hearty a+ on improving tests.

Comment 10

[Rob Campbell [:rc] (:robcee)]

checked into devtools
changeset 42e06e5d97c8

Comment 11

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 475914 [details] [diff] [review]
[checked-in] rebased patch

http://hg.mozilla.org/mozilla-central/rev/b829cf5debce