Bug 1005755 fixed 'view source in debugger' feature. Now we need a test for this. STR: 1) Load http://janodvarko.cz/firebug/tests/601/Issue601.htm 2) Open the Toolbox and select the Console panel 3) Click the POST button on the page 4) Expand the Network event in the Console 5) Select the 'Stack Trace' panel and click on a stack frame 6) The UI should switch to the Debugger panel Honza

Comment on attachment 8925570 [details] Bug 1408970 - Test for view source in debugger. https://reviewboard.mozilla.org/r/196702/#review201922 ::: devtools/client/netmonitor/test/browser_net_view-source-debugger.js:33 (Diff revision 2) > + document.querySelectorAll(".request-list-item")[0]); > + EventUtils.sendMouseEvent({ type: "click" }, > + document.querySelector("#stack-trace-tab")); > + yield wait; > + let link = document.querySelectorAll(".frame-link-source")[0]; > + link.click(); I tried using `EventUtils.sendMouseEvent({ type: "click" }, link);` but that didn't seem to generate a click. I'm not sure what I missed here. ::: devtools/client/netmonitor/test/browser_net_view-source-debugger.js:35 (Diff revision 2) > + document.querySelector("#stack-trace-tab")); > + yield wait; > + let link = document.querySelectorAll(".frame-link-source")[0]; > + link.click(); > + > + yield waitSomeTime(); Is there a cleaner way to do this by avoiding waitSomeTime()? I tried just having `yield waitUntil(() => toolbox.currentToolId == "jsdebugger");` without `waitSomeTime()` but although the test passes, it also passes if I change the assert to `yield waitUntil(() => toolbox.currentToolId == "netmonitor");`. However if I wait for some time and then have the above asserts, they yield correct results, ie passes with `toolbox.currentToolId == "jsdebugger")` and fails with `toolbox.currentToolId == "netmonitor")`

Hi Honza, I have written a test for this and in the reviewboard, have added few comments at some lines, where I wasn't sure of how it could be written in a better style. Can you check once?

Comment on attachment 8925570 [details] Bug 1408970 - Test for view source in debugger. https://reviewboard.mozilla.org/r/196702/#review201926 ::: devtools/client/netmonitor/test/browser_net_view-source-debugger.js:9 (Diff revision 2) > +"use strict"; > + > +/** > + * Tests if on clicking the stack frame, UI switches to the Debugger panel. > + */ > +add_task(function* () { can you use an async function ? We used to use generator because the test harness will allow us to have this async-y syntax, but now we can directly use async/await ::: devtools/client/netmonitor/test/browser_net_view-source-debugger.js:11 (Diff revision 2) > +/** > + * Tests if on clicking the stack frame, UI switches to the Debugger panel. > + */ > +add_task(function* () { > + // Set a higher panel height in order to get full CodeMirror content > + Services.prefs.setIntPref("devtools.toolbox.footer.height", 400); could you use `pushPref` like in http://searchfox.org/mozilla-central/source/devtools/client/netmonitor/test/browser_net_cached-status.js#12 ? It set the preference and will revert it when the test is done. Here, the next test will still have a 400px height footer, and even if it's minor, it may perturb other tests. ::: devtools/client/netmonitor/test/browser_net_view-source-debugger.js:21 (Diff revision 2) > + let { document, store, windowRequire } = monitor.panelWin; > + let Actions = windowRequire("devtools/client/netmonitor/src/actions/index"); > + store.dispatch(Actions.batchEnable(false)); > + > + let wait = waitForNetworkEvents(monitor, 0, 2); > + yield ContentTask.spawn(tab.linkedBrowser, {}, function* () { no need for the callback to be a generator function ::: devtools/client/netmonitor/test/browser_net_view-source-debugger.js:27 (Diff revision 2) > + EventUtils.sendMouseEvent({ type: "mousedown" }, > + document.querySelectorAll(".request-list-item")[0]); can you add some comments to explain what we are doing ? Here I guess we open a request detail panel ? ::: devtools/client/netmonitor/test/browser_net_view-source-debugger.js:29 (Diff revision 2) > + EventUtils.sendMouseEvent({ type: "click" }, > + document.querySelector("#stack-trace-tab")); same thing here, can we add a comment to explain what we do ? Also, shouldn't we wait until the #stack-trace-tab element is visible ? ::: devtools/client/netmonitor/test/browser_net_view-source-debugger.js:35 (Diff revision 2) > + yield waitSomeTime(); > + yield waitUntil(() => toolbox.currentToolId == "jsdebugger"); you can check how we do it in the console : http://searchfox.org/mozilla-central/rev/5a60492a53667fc61a62af1847d005a210b7a4f6/devtools/client/webconsole/new-console-output/test/mochitest/head.js#232-265

@Abhinav, I can take a look a the patch after all Nicolas's comments are resolved. Thanks for working on this! Honza

Comment on attachment 8925570 [details] Bug 1408970 - Test for view source in debugger. https://reviewboard.mozilla.org/r/196702/#review203692 Just removing the review request for now to clean up my review queue. Honza

Comment on attachment 8925570 [details] Bug 1408970 - Test for view source in debugger. https://reviewboard.mozilla.org/r/196702/#review203998

Comment on attachment 8925570 [details] Bug 1408970 - Test for view source in debugger. https://reviewboard.mozilla.org/r/196702/#review201926 > same thing here, can we add a comment to explain what we do ? > Also, shouldn't we wait until the #stack-trace-tab element is visible ? I was drawing parallels from https://searchfox.org/mozilla-central/rev/5a60492a53667fc61a62af1847d005a210b7a4f6/devtools/client/netmonitor/test/browser_net_cause_source_map.js#38-42 https://searchfox.org/mozilla-central/rev/5a60492a53667fc61a62af1847d005a210b7a4f6/devtools/client/netmonitor/test/browser_net_complex-params.js#27-32 where we click on on the tab and wait for the panel to load.

Comment on attachment 8925570 [details] Bug 1408970 - Test for view source in debugger. https://reviewboard.mozilla.org/r/196702/#review201926 > you can check how we do it in the console : http://searchfox.org/mozilla-central/rev/5a60492a53667fc61a62af1847d005a210b7a4f6/devtools/client/webconsole/new-console-output/test/mochitest/head.js#232-265 Thanks for this snippet. When I debugged the code-flow, I found out that control ultimately reaches https://dxr.mozilla.org/mozilla-central/source/devtools/client/netmonitor/src/connector/index.js#98 > https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/view-source.js#53 but I couldn't find any event being emitted like https://dxr.mozilla.org/mozilla-central/source/devtools/client/webconsole/hudservice.js#487 So I couldn't wait for `source-in-debugger-opened` and instead had to waitSomeTime(). Let me know if there is a cleaner way to do this.

Thanks Nicolas for the review, I have addressed all the comments. @Honza, its fine, have asked Nicolas for a second round of review.

Comment on attachment 8925570 [details] Bug 1408970 - Test for view source in debugger. https://reviewboard.mozilla.org/r/196702/#review204072 The test looks good, we only need to figure something in order to wait in a consistent manner for the debugger to be ready. The issue with setting a fixed amount of time is that it can fail for various reason on various platforms on TRY, and gives us failures. ::: devtools/client/netmonitor/test/browser_net_view-source-debugger.js:20 (Diff revision 3) > + > + let { document, store, windowRequire } = monitor.panelWin; > + let Actions = windowRequire("devtools/client/netmonitor/src/actions/index"); > + store.dispatch(Actions.batchEnable(false)); > + > + let wait = waitForNetworkEvents(monitor, 0, 2); could this be renamed to something more explicit (waitForContentRequests ?) ::: devtools/client/netmonitor/test/browser_net_view-source-debugger.js:28 (Diff revision 3) > + EventUtils.sendMouseEvent({ type: "mousedown" }, > + document.querySelectorAll(".request-list-item")[0]); we can simplify this into ``` EventUtils.sendMouseEvent({ type: "mousedown" }, document.querySelector(".request-list-item")); ``` or even better (if it works) : ``` document.querySelector(".request-list-item").click(); ``` ::: devtools/client/netmonitor/test/browser_net_view-source-debugger.js:31 (Diff revision 3) > + EventUtils.sendMouseEvent({ type: "click" }, > + document.querySelector("#stack-trace-tab")); same here, we can use ``` EventUtils.sendMouseEvent({ type: "click" }, document.getElementById("stack-trace-tab")); ``` or ``` document.getElementById("stack-trace-tab").click(); ``` ::: devtools/client/netmonitor/test/browser_net_view-source-debugger.js:58 (Diff revision 3) > + EventUtils.sendMouseEvent({ type: "click" }, > + frameLinkNode.querySelector(".frame-link-source")); same here, can we try to use `.click()` directly ? ::: devtools/client/netmonitor/test/browser_net_view-source-debugger.js:61 (Diff revision 3) > + ok(line, `source line found ("${line}")`); > + > + EventUtils.sendMouseEvent({ type: "click" }, > + frameLinkNode.querySelector(".frame-link-source")); > + > + await waitSomeTime(); I still don't like this :) I think it would be better to emit an event from the viewSourceInDebugger function. Would you have another idea in mind Honza ?

NI Honza on the last comment by Nicolas

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #13) > > + await waitSomeTime(); > > I still don't like this :) > I think it would be better to emit an event from the viewSourceInDebugger > function. > > Would you have another idea in mind Honza ? Yeah totally agree, timeouts are always evil. Could we use DOM (mutation) observer to wait till the "jsdebugger" panel and its content is ready? Or could we wait for 'ready' event sent by the "jsdebugger" panel? Honza

Comment on attachment 8925570 [details] Bug 1408970 - Test for view source in debugger. https://reviewboard.mozilla.org/r/196702/#review204072 > we can simplify this into > ``` > EventUtils.sendMouseEvent({ type: "mousedown" }, > document.querySelector(".request-list-item")); > ``` > > or even better (if it works) : > ``` > document.querySelector(".request-list-item").click(); > ``` It listens for mousedown, so had to use EventUtils only

Comment on attachment 8925570 [details] Bug 1408970 - Test for view source in debugger. https://reviewboard.mozilla.org/r/196702/#review204072 Hi Nicolas, Thanks for the feedback. I have fixed the issue by waiting for the event - `jsdebugger-selected`

Comment on attachment 8925570 [details] Bug 1408970 - Test for view source in debugger. https://reviewboard.mozilla.org/r/196702/#review205942 @Abhinav, thanks for working on this! The pach looks good to me, but please put Nicolas in the commit message and ask him for the final review. He's done the review here. Honza

Comment on attachment 8925570 [details] Bug 1408970 - Test for view source in debugger. https://reviewboard.mozilla.org/r/196702/#review205964 We are almost done, thanks Abhinav ! I have a few comment, but overall the patch is good and we'll land it when the comments are adressed. ::: commit-message-e7fee:1 (Diff revision 5) > +Bug 1408970 - Test for view source in debugger. r=Honza change Hona to nchevobbe ::: devtools/client/netmonitor/test/browser_net_view-source-debugger.js:29 (Diff revision 5) > + > + info("Clicking stack-trace tab and waiting for stack-trace panel to open"); > + let wait = waitForDOM(document, "#stack-trace-panel"); > + // Click on the first request > + EventUtils.sendMouseEvent({ type: "mousedown" }, > + document.querySelectorAll(".request-list-item")[0]); replace with document.querySelector(".request-list-item")); ::: devtools/client/netmonitor/test/browser_net_view-source-debugger.js:40 (Diff revision 5) > +async function waitOnToolbox(toolbox, event) { > + return new Promise(resolve => toolbox.on(event, resolve)); > +} I think this could be removed. The toolbox as a `once` property that sets the event listener, returns a promise, and remove the event listeners when resolving the promise. So, in the checkClickOnNode function, instead of doing ``` waitOnToolbox(toolbox, "jsdebugger-selected"); ``` you can do ``` toolbox.once("jsdebugger-selected"); ``` ::: devtools/client/netmonitor/test/browser_net_view-source-debugger.js:56 (Diff revision 5) > + frameLinkNode.querySelector(".frame-link-source").click(); > + > + // wait for the event `jsdebugger-selected` > + await waitOnToolbox(toolbox, "jsdebugger-selected"); Here we are still vulnerable to a race condition if the debugger is very very fast to open and emit the event before we are waiting for it :) Usually, if we are going to trigger a click that will fire an event, we do this like : ``` // create the promise const onJsDebuggerSelected = toolbox.once("jsdebugger-selected"); // Fire the click event frameLinkNode.querySelector(".frame-link-source").click(); // wait for the promise to resolve await onJSDebuggerSelected; ```

Comment on attachment 8925570 [details] Bug 1408970 - Test for view source in debugger. https://reviewboard.mozilla.org/r/196702/#review205964 Thanks for the review. I have addressed the comments.

Comment on attachment 8925570 [details] Bug 1408970 - Test for view source in debugger. https://reviewboard.mozilla.org/r/196702/#review206000 Excellent, thanks a lot for your patience Abhinav. Let's push this to TRY

Pushed by nchevobbe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/02305cc3d49b Test for view source in debugger. r=nchevobbe