Closed Bug 1181837 Opened 5 years ago Closed 4 years ago

Migrate browser/devtools/inspector tests to shared-head.js

Categories

(DevTools :: Inspector, defect)

defect
Not set

Tracking

(firefox42 affected, firefox46 fixed)

RESOLVED FIXED
Firefox 46
Tracking Status
firefox42 --- affected
firefox46 --- fixed

People

(Reporter: bgrins, Assigned: pbro)

References

(Blocks 1 open bug)

Details

Attachments

(8 files)

58 bytes, text/x-review-board-request
bgrins
: review+
pbro
: checkin+
Details
58 bytes, text/x-review-board-request
bgrins
: review+
pbro
: checkin+
Details
58 bytes, text/x-review-board-request
bgrins
: review+
pbro
: checkin+
Details
58 bytes, text/x-review-board-request
bgrins
: review+
pbro
: checkin+
Details
58 bytes, text/x-review-board-request
bgrins
: review+
pbro
: checkin+
Details
76.27 KB, patch
bgrins
: review+
pbro
: checkin+
Details | Diff | Splinter Review
13.51 KB, patch
bgrins
: review+
pbro
: checkin+
Details | Diff | Splinter Review
150.26 KB, patch
bgrins
: review+
pbro
: checkin+
Details | Diff | Splinter Review
We should import shared-head.js in inspector/test/head.js and remove any newly unnecessary code after that.
Blocks: 1181843
Blocks: 1181844
No longer blocks: 1181843
No longer blocks: 1181844
There are also several common functionalities lying in individual test files which need to be migrated to inspector/test/head.js; Should that work be done along with this bug or may be a new bug should be filed for that job- I guess this is going to be true for most of the other devtools module.
Flags: needinfo?(bgrinstead)
(In reply to Avik Pal [:avikpal] from comment #1)
> There are also several common functionalities lying in individual test files
> which need to be migrated to inspector/test/head.js; Should that work be
> done along with this bug or may be a new bug should be filed for that job- I
> guess this is going to be true for most of the other devtools module.

We can do that work in a separate bug.
Flags: needinfo?(bgrinstead)
Assignee: nobody → pbrosset
Progress update for Brian:

- I've slightly cleaned up shared-head.js to remove some code duplication.
- I've also changed a couple of its function names so it's easier to reuse those from inspector tests.
- I've imported it in devtools/client/inspector/test/head.js and made sure the test from this folder still passed.
- I've then imported devtools/client/inspector/test/head.js into devtools/client/inspector/layout/test/head.js so that inspector's sub-panel tests can reuse stuff like getNode, selectNode, etc. This made me realize that paths needed to be absolute (chrome://mochitests/content/browser/devtools/...) when you use loadSubScript.

Both inspector test and layout test's head.js files are now much smaller. There's still a bit of cleanup to do in the layout head.js file, which will help bring it down to about 100 lines long only.

After this, I want to take care of the remaining sub-panels: rules, computed, fonts, markup. But I reckoned these patches could already get reviewed now.
Comment on attachment 8707371 [details]
MozReview Request: Bug 1181837 - 1 - Minor shared-head.js cleanup and code duplication removal

https://reviewboard.mozilla.org/r/30659/#review27775
Attachment #8707371 - Flags: review?(bgrinstead) → review+
Comment on attachment 8707372 [details]
MozReview Request: Bug 1181837 - 2 - Rename shared-head's open/close toolbox functions

https://reviewboard.mozilla.org/r/30661/#review27777
Attachment #8707372 - Flags: review?(bgrinstead) → review+
Comment on attachment 8707373 [details]
MozReview Request: Bug 1181837 - 3 - Introduce openToolboxForTab in shared-head.js to let tests open toolboxes in existing tabs

https://reviewboard.mozilla.org/r/30663/#review27779
Attachment #8707373 - Flags: review?(bgrinstead) → review+
Comment on attachment 8707374 [details]
MozReview Request: Bug 1181837 - 4 - Includes shared-head.js in inspector's tests and removes code duplication

https://reviewboard.mozilla.org/r/30665/#review27781

nice
Attachment #8707374 - Flags: review?(bgrinstead) → review+
Attachment #8707375 - Flags: review?(bgrinstead) → review+
Comment on attachment 8707375 [details]
MozReview Request: Bug 1181837 - 5 - Include inspector's head.js in layout-view's tests and remove code duplication

https://reviewboard.mozilla.org/r/30667/#review27783

::: devtools/client/inspector/layout/test/browser_layout_update-in-iframes.js:46
(Diff revision 1)
> -  yield selectNode(node, inspector);
> +  yield selectNodeInIframe2("p", inspector);

This doesn't read as what the `info` above says ("Selecting the test node in iframe1").  Should this be selecting it in iframe 1?
Keywords: leave-open
(In reply to Brian Grinstead [:bgrins] from comment #14)
> devtools/client/inspector/layout/test/browser_layout_update-in-iframes.js:46
> (Diff revision 1)
> > -  yield selectNode(node, inspector);
> > +  yield selectNodeInIframe2("p", inspector);
> 
> This doesn't read as what the `info` above says ("Selecting the test node in
> iframe1").  Should this be selecting it in iframe 1?
It turns out this test was broken in all sorts of ways, and in fact was disabled, which is why I didn't see it fail while running on try.
I made sure it now passes locally, but kept it disabled, it looks like it was disabled in the past for some reasons unrelated to my changes (bug 1020038)
Attachment #8707371 - Flags: checkin+
Attachment #8707372 - Flags: checkin+
Attachment #8707373 - Flags: checkin+
Attachment #8707374 - Flags: checkin+
Attachment #8707375 - Flags: checkin+
Part 6 - Include inspector's head.js in rule and computed view tests to remove duplication
Attachment #8709024 - Flags: review?(bgrinstead)
Part 7 - Include inspector's head.js in font inspector to remove duplication
Attachment #8709025 - Flags: review?(bgrinstead)
Part 8 - Include inspector's head.js in markup view to remove duplication

(for info, the last 3 parts were attached here rather than pushed to mozreview because mozreview doesn't yet deal well with this sort of workflow, see bug 1096768).
Attachment #8709026 - Flags: review?(bgrinstead)
Comment on attachment 8709024 [details] [diff] [review]
Bug_1181837_-_6_-_Include_inspector_s_head.js_in_r.diff

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

Nice!

::: devtools/client/inspector/computed/test/head.js
@@ +4,3 @@
>  "use strict";
>  
> +// Import the inspector's head.js first (which itself imports shared-head.js).

So this ended up working after all?  I remember we were seeing some issues that appeared to be due to nested loadSubScript calls.

::: devtools/client/inspector/test/browser_inspector_keyboard-shortcuts-copy-outerhtml.js
@@ +33,5 @@
>  }
>  
>  function* checkClipboard(expectedText, node) {
>    let deferred = promise.defer();
> +  SimpleTest.waitForClipboard(

This changed to SimpleTest.waitForClipboard but browser_inspector_menu-02-copy-items.js is still using waitForClipboard directly.  What gives?
Attachment #8709024 - Flags: review?(bgrinstead) → review+
Status: NEW → ASSIGNED
Attachment #8709025 - Flags: review?(bgrinstead) → review+
Comment on attachment 8709026 [details] [diff] [review]
Bug_1181837_-_8_-_Include_inspector_s_head.js_in_m.diff

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

::: devtools/client/inspector/markup/test/browser_markup_anonymous_01.js
@@ +9,3 @@
>  
>  add_task(function*() {
> +  let {inspector} = yield addTab(TEST_URL).then(() => openInspector());

Isn't this what openInspectorForURL(TEST_URL) is for?  Either way works for me but I think that's a little shorter / simpler
Attachment #8709026 - Flags: review?(bgrinstead) → review+
Duplicate of this bug: 1181858
Duplicate of this bug: 1181843
Duplicate of this bug: 1181842
(In reply to Brian Grinstead [:bgrins] from comment #24)
> ::: devtools/client/inspector/computed/test/head.js
> @@ +4,3 @@
> >  "use strict";
> >  
> > +// Import the inspector's head.js first (which itself imports shared-head.js).
> 
> So this ended up working after all?  I remember we were seeing some issues
> that appeared to be due to nested loadSubScript calls.
Yeah, so I ended up being able to fix it by using absolute paths in loadSubScript. It made perfect sense after I found out the issue, but the error wasn't self-explanatory, so it took some time. Basically, if you loadSubScript something that contains another call to loadSubScript with a relative path, then that path becomes relative to the parent, which might be wrong.

> devtools/client/inspector/test/browser_inspector_keyboard-shortcuts-copy-
> outerhtml.js
> @@ +33,5 @@
> >  }
> >  
> >  function* checkClipboard(expectedText, node) {
> >    let deferred = promise.defer();
> > +  SimpleTest.waitForClipboard(
> 
> This changed to SimpleTest.waitForClipboard but
> browser_inspector_menu-02-copy-items.js is still using waitForClipboard
> directly.  What gives?
Good catch, I guess I should clean this up. In fact we have 2 functions: waitForClipboard inside head.js, and SimpleTest.waitForClipboard provided by the test harness. They have almost identical signatures, and the one in head.js is just a convenient wrapper for the other. The fact that some tests use one and others use the other is just historical. I'll try to have something more consistent.

(In reply to Brian Grinstead [:bgrins] from comment #25)
> ::: devtools/client/inspector/markup/test/browser_markup_anonymous_01.js
> @@ +9,3 @@
> >  
> >  add_task(function*() {
> > +  let {inspector} = yield addTab(TEST_URL).then(() => openInspector());
> 
> Isn't this what openInspectorForURL(TEST_URL) is for?  Either way works for
> me but I think that's a little shorter / simpler
You're right, I can make these shorter.
Most (all?) markupview tests used to do 'yield addTab(TEST_URL).then(openInspector)'
but this didn't work anymore with the new shared openInspector function, so I opted for a mass find/replace 'openInspector' with '() => openInspector()'.
But openInspectorForURL would be simpler indeed.
I have addressed the review comments and pushed to try one last time:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f0e06849beda&group_state=expanded
Keywords: leave-open
Attachment #8709024 - Flags: checkin+
Attachment #8709025 - Flags: checkin+
Attachment #8709026 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/86e366d70585
https://hg.mozilla.org/mozilla-central/rev/74646bfb670f
https://hg.mozilla.org/mozilla-central/rev/173ae55dbac1
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
[bugday-20160323]

Status: RESOLVED,FIXED -> UNVERIFIED

Comments:
STR: Not clear.
Developer specific testing

Component: 
Name 			Firefox
Version 		46.0b4
Build ID 		20160322075646
Update Channel 	        beta
User Agent 		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS                      Windows 7 SP1 x86_64

Expected Results: 
As per developer specified requirement

Actual Results: 
As expected
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.