test cleanup: conditional-breakpoint popup tests should wait on shown/hidden popup events

RESOLVED FIXED in Firefox 51

Status

()

Firefox
Developer Tools: Debugger
--
enhancement
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: jdescottes, Assigned: jdescottes)

Tracking

Trunk
Firefox 51
Points:
---

Firefox Tracking Flags

(firefox50 affected, firefox51 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

a year ago
Blocker for Bug 1206133. Mochitests testing the conditional breakpoint popup rely on the popupshowing and popuphiding event and expect the popup content to be interactive after the event fires. 

Showing the popup will be made asynchronous in Bug 1206133, so we should instead rely on popupshown and popuphidden events.
(Assignee)

Updated

a year ago
Blocks: 1206133
(Assignee)

Updated

a year ago
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
(Assignee)

Comment 1

a year ago
Created attachment 8771934 [details]
Bug 1287388 - wait for popupshown event in debugger conditional breakpoint tests;

Review commit: https://reviewboard.mozilla.org/r/64868/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64868/
Attachment #8771934 - Flags: review?(jlong)
https://reviewboard.mozilla.org/r/64868/#review63014

This looks mostly good, but I don't understand why this isn't just a mechanical change. It looks like there is actually new code that yields on this event. Were you running into problems?

::: devtools/client/debugger/test/mochitest/browser_dbg_conditional-breakpoints-01.js:189
(Diff revision 1)
> +      // Explicitely hide the conditional breakpoint popup which was displayed when adding
> +      // the breakpoints.
> +      let onPopupHidden = waitForDebuggerEvents(gPanel,
> +        gDebugger.EVENTS.CONDITIONAL_BREAKPOINT_POPUP_HIDDEN);
> +      gSources._hideConditionalPopup();
> +      yield onPopupHidden;

Why do you need to add extra code in the tests, instead of just converting the events?

::: devtools/client/debugger/test/mochitest/browser_dbg_conditional-breakpoints-02.js:57
(Diff revision 1)
> -    function addBreakpoint3() {
> +    function* addBreakpoint3() {
>        let finished = waitForDispatch(gPanel, constants.ADD_BREAKPOINT);
> +      let popupShown = waitForDebuggerEvents(gPanel, CONDITIONAL_POPUP_SHOWN);
>        setCaretPosition(20);
>        gSources._onCmdAddConditionalBreakpoint();
> -      return finished;
> +      yield finished;

I'd rather not add this code. The point of actions is that you simply wait on the action, and you don't need any of these manual events.

::: devtools/client/debugger/test/mochitest/browser_dbg_conditional-breakpoints-02.js:66
(Diff revision 1)
> -      let finished = waitForDispatch(gPanel, constants.SET_BREAKPOINT_CONDITION);
>        setCaretPosition(20);
> +
> +      let popupShown = waitForDebuggerEvents(gPanel, CONDITIONAL_POPUP_SHOWN);
>        gSources._onCmdAddConditionalBreakpoint();
> +      yield popupShown;

ditto
(Assignee)

Comment 3

a year ago
(In reply to James Long (:jlongster) from comment #2)
> https://reviewboard.mozilla.org/r/64868/#review63014
> 
> This looks mostly good, but I don't understand why this isn't just a
> mechanical change. It looks like there is actually new code that yields on
> this event. Were you running into problems?
> 

The point of this bug is to make the debugger tests updated here to be compatible with the XUL popup changes made in Bug 1206133.
Showing the popup will be asynchronous, which was breaking all the tests updated here. The reason it is not mechanical is that tests were expecting the conditional breakpoint popup to be displayed synchronously and were not waiting for the popup to be shown/hidden properly before interacting with it.

All the tests updated here were failing when applying the base patch from Bug 1206133. Feel free to retrieve the changeset at https://hg.mozilla.org/try/rev/2db186a57ea6812bfc91d84e8b42f145c538e22e if you want to give it a try.

> :::
> devtools/client/debugger/test/mochitest/browser_dbg_conditional-breakpoints-
> 01.js:189
> (Diff revision 1)
> > +      // Explicitely hide the conditional breakpoint popup which was displayed when adding
> > +      // the breakpoints.
> > +      let onPopupHidden = waitForDebuggerEvents(gPanel,
> > +        gDebugger.EVENTS.CONDITIONAL_BREAKPOINT_POPUP_HIDDEN);
> > +      gSources._hideConditionalPopup();
> > +      yield onPopupHidden;
> 
> Why do you need to add extra code in the tests, instead of just converting
> the events?
> 

Showing/hiding the popup is asynchronous

> :::
> devtools/client/debugger/test/mochitest/browser_dbg_conditional-breakpoints-
> 02.js:57
> (Diff revision 1)
> > -    function addBreakpoint3() {
> > +    function* addBreakpoint3() {
> >        let finished = waitForDispatch(gPanel, constants.ADD_BREAKPOINT);
> > +      let popupShown = waitForDebuggerEvents(gPanel, CONDITIONAL_POPUP_SHOWN);
> >        setCaretPosition(20);
> >        gSources._onCmdAddConditionalBreakpoint();
> > -      return finished;
> > +      yield finished;
> 
> I'd rather not add this code. The point of actions is that you simply wait
> on the action, and you don't need any of these manual events.
> 
> :::
> devtools/client/debugger/test/mochitest/browser_dbg_conditional-breakpoints-
> 02.js:66
> (Diff revision 1)
> > -      let finished = waitForDispatch(gPanel, constants.SET_BREAKPOINT_CONDITION);
> >        setCaretPosition(20);
> > +
> > +      let popupShown = waitForDebuggerEvents(gPanel, CONDITIONAL_POPUP_SHOWN);
> >        gSources._onCmdAddConditionalBreakpoint();
> > +      yield popupShown;
> 
> ditto
(Assignee)

Comment 4

a year ago
James: I could also move this patch to Bug 1206133, maybe it would be easier for you to review there? 
I thought it could be nice to land this patch independently, but it makes the patch harder to understand without context. Let me know how you want to proceed!
Flags: needinfo?(jlong)
Ah, I see. I dislike having this kind of async boilerplate in tests, but the root of the problem is how all of these tests are written in the first place. They practically encourage careful async coordination. If you need that code for the tests to run, that's fine with me.

We will be getting rid of many of these tests once we land and flesh out the new debugger anyway.
Flags: needinfo?(jlong)
Comment on attachment 8771934 [details]
Bug 1287388 - wait for popupshown event in debugger conditional breakpoint tests;

https://reviewboard.mozilla.org/r/64868/#review63170

If that extra async code is necessary, this looks fine!
Attachment #8771934 - Flags: review?(jlong) → review+
(Assignee)

Comment 7

a year ago
Comment on attachment 8771934 [details]
Bug 1287388 - wait for popupshown event in debugger conditional breakpoint tests;

I just tried the latest patch from Bug 1206133, and the impact on the debugger tests slightly changed. I'll re-submit for review when ready. Sorry for the mess!
Attachment #8771934 - Flags: review+
Comment hidden (mozreview-request)
(Assignee)

Comment 9

a year ago
Comment on attachment 8771934 [details]
Bug 1287388 - wait for popupshown event in debugger conditional breakpoint tests;

Carry over r+ from the previous review. The final patch is just a subset of what was reviewed before.

Some of the earlier changes were no longer needed after retrieving a newer version of the patch from Bug 1206133 

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ad4aaec99411
Attachment #8771934 - Flags: review+

Comment 10

a year ago
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/2edc35144ebf
wait for popupshown event in debugger conditional breakpoint tests;r=jlongster

Comment 11

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2edc35144ebf
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
You need to log in before you can comment on or make changes to this bug.