Closed Bug 585043 Opened 14 years ago Closed 14 years ago

Improve Inspector DOM panel unittests

Categories

(DevTools :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: rcampbell, Assigned: msucan)

References

Details

(Whiteboard: [patchclean:0929][fixed-in-devtools])

Attachments

(1 file, 1 obsolete file)

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.
Attached patch proposed fix (obsolete) — Splinter Review
Proposed fix. As discussed, this patch is on top of the tree panel patches.
Assignee: nobody → mihai.sucan
Status: NEW → ASSIGNED
Attachment #467813 - Flags: feedback?(rcampbell)
Depends on: 572038
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.
Attachment #467813 - Flags: feedback?(rcampbell) → feedback-
(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!
Updated and rebased patch, as requested. Thanks Robert!
Attachment #467813 - Attachment is obsolete: true
Attachment #475914 - Flags: feedback?(rcampbell)
Blocks: 592320
Whiteboard: [patchclean:0916]
Comment on attachment 475914 [details] [diff] [review] [checked-in] rebased patch looks good
Attachment #475914 - Flags: feedback?(rcampbell) → feedback+
Comment on attachment 475914 [details] [diff] [review] [checked-in] rebased patch Thanks for your feedback+ Robert! Asking for review.
Attachment #475914 - Flags: review?(dietrich)
Attachment #475914 - Flags: review?(dietrich) → review+
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.
Attachment #475914 - Flags: approval2.0?
we can't block on this, but would appreciate approval to land these test enhancements. Moar tests!
Comment on attachment 475914 [details] [diff] [review] [checked-in] rebased patch a hearty a+ on improving tests.
Attachment #475914 - Flags: approval2.0? → approval2.0+
Keywords: checkin-needed
Whiteboard: [patchclean:0916] → [patchclean:0929]
checked into devtools changeset 42e06e5d97c8
Whiteboard: [patchclean:0929] → [patchclean:0929][fixed-in-devtools]
Attachment #475914 - Attachment description: rebased patch → [checked-in] rebased patch
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Keywords: checkin-needed
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: