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

Status: RESOLVED FIXED

(DevTools :: Debugger, enhancement)

Description

[Julian Descottes [:jdescottes]]

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.

Comment 1

[Julian Descottes [:jdescottes]]

Attachment: 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/

Comment 2

[James Long (:jlongster)]

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

Comment 3

[Julian Descottes [:jdescottes]]

(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

Comment 4

[Julian Descottes [:jdescottes]]

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!

Comment 5

[James Long (:jlongster)]

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.

Comment 6

[James Long (:jlongster)]

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!

Comment 7

[Julian Descottes [:jdescottes]]

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!

Comment 8

[Julian Descottes [:jdescottes]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64868/diff/1-2/

Comment 9

[Julian Descottes [:jdescottes]]

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

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/2edc35144ebf
wait for popupshown event in debugger conditional breakpoint tests;r=jlongster

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2edc35144ebf