1264626 - Write a test against the browser toolbox debugger

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Description

[Alexandre Poirot [:ochameau]]

I started writing a test in bug 1263629, but it is challenging to get it green on try and don't want to block this bug on getting this test green.

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: patch v1

Comment 2

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bbceaba39e5b

Comment 3

[Alexandre Poirot [:ochameau]]

Attachment: patch v2

Rebased. Looks like this test is now green on try?!

This test check that overall breakpoint works in the browser debugger,
but also that we don't step into promise world.
This test used to fail without my recent promise patch.

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8742350 [details] [diff] [review]
patch v2

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

Well, it seems reasonable to me, but I haven't reviewed many debugger tests.  I would ask r? from :ejpbruel or :jlong.

::: devtools/client/framework/test/browser_browser_toolbox_debugger.js
@@ +15,5 @@
> +      ["devtools.chrome.enabled", true],
> +      // Test-only pref to allow passing `testScript` argument to the browser
> +      // toolbox
> +      ["devtools.browser-toolbox.allow-unsafe-script", true],
> +      // On debug test slave, it takes more than the default time (20s)

Nit: Let's say "debug machines" or "debug test runner" or something instead of "slave"

@@ +36,5 @@
> +
> +  // Execute the function every second in order to trigger the breakpoint
> +  let interval = setInterval(s.plop, 1000);
> +
> +  // Be careful, this JS function is going to be executed in the addon toolbox,

Nit: browser toolbox

@@ +40,5 @@
> +  // Be careful, this JS function is going to be executed in the addon toolbox,
> +  // which lives in another process. So do not try to use any scope variable!
> +  let env = Components.classes["@mozilla.org/process/environment;1"].getService(Components.interfaces.nsIEnvironment);
> +  let testScript = function () {
> +    dump("Opening the browser toolbox and debugger panel\n");

Can you use Task.jsm here?  I know it's going to the other process, but it would be nice if it's possible, so that it uses a format like other modern tests.

Comment 5

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d7ba4c29df32

Comment 6

[Alexandre Poirot [:ochameau]]

Attachment: patch v3

James, this test is special as it targets the browser toolbox.
It craft the "testScript" function which is stringified and passed to the
browser toolbox process. It means that we can't use any head helper,
not any test harness function. We just execute code.
That may explain why this test is quite simple and may be more high level than regular one.

In parallel, I'm asking jryans to review all this special magic.

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8743204 [details] [diff] [review]
patch v3

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

Does it make sense for such a test to be in the debugger folder, since it's mostly testing that tool?  It seems like more of test is debugger specific than about the browser toolbox generally.

General approach looks good!

::: devtools/client/framework/test/browser_browser_toolbox_debugger.js
@@ +38,5 @@
> +  let interval = setInterval(s.plop, 1000);
> +
> +  // Be careful, this JS function is going to be executed in the browser toolbox,
> +  // which lives in another process. So do not try to use any scope variable!
> +  let env = Components.classes["@mozilla.org/process/environment;1"].getService(Components.interfaces.nsIEnvironment);

Nit: Wrap if more than 80 char

@@ +101,5 @@
> +      toolbox.destroy();
> +
> +    }).catch(error => {
> +      dump("Error while running code in the browser toolbox process:\n");
> +      dump(error+"\n");

Nit: error + "

Comment 8

[James Long (:jlongster)]

Comment on attachment 8743204 [details] [diff] [review]
patch v3

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

(sorry, was off most of last week)

What changed to allow this? I thought there was a technical limitation that make testing the browser toolbox hard.

I know jryans suggested moving this into the debugger, but I don't think we should do that, and it's for a stupid reason: the debugger tests are already too slow. You will most likely hit random timeouts on try if you move it to the debugger. We've already have to disable debugger tests on linux32 debug because they kept timing out, for no reason other than just taking too long. It sucks, but with our debugger rewrite we plan on coming in with a *far* better test suite. Until then this probably should live in a different folder to appease the timeout gods...

::: devtools/client/framework/test/browser_browser_toolbox_debugger.js
@@ +112,5 @@
> +  });
> +
> +  let { BrowserToolboxProcess } = Cu.import("resource://devtools/client/framework/ToolboxProcess.jsm", {});
> +  let closePromise;
> +  yield new Promise(onRun => {

Is there much point in promises here? It makes the code a lot more confusing. Instead you could do:

BrowserToolboxProcess.init(
  () => {
    ok(true, "Browser toolbox process just closed");
    clearInterval(interval);
  },
  () => ok(true, "Browser toolbox started\n")
);

Comment 9

[Alexandre Poirot [:ochameau]]

(In reply to James Long (:jlongster) from comment #8)
> Comment on attachment 8743204 [details] [diff] [review]
> patch v3
> 
> Review of attachment 8743204 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (sorry, was off most of last week)
> 
> What changed to allow this? I thought there was a technical limitation that
> make testing the browser toolbox hard.

Nothing changed. I just made this non-obvious test case which pipes JS through a process environment variable! We are not going to run all tests against the browser toolbox, but just going to have a set of meaningful test that check for the typical browser toolbox regressions.
Like this one, to check that we do not end in debugger internals while stepping.

Comment 10

[Alexandre Poirot [:ochameau]]

(In reply to James Long (:jlongster) from comment #8)
> ::: devtools/client/framework/test/browser_browser_toolbox_debugger.js
> @@ +112,5 @@
> > +  });
> > +
> > +  let { BrowserToolboxProcess } = Cu.import("resource://devtools/client/framework/ToolboxProcess.jsm", {});
> > +  let closePromise;
> > +  yield new Promise(onRun => {
> 
> Is there much point in promises here? It makes the code a lot more
> confusing. Instead you could do:
> 
> BrowserToolboxProcess.init(
>   () => {
>     ok(true, "Browser toolbox process just closed");
>     clearInterval(interval);
>   },
>   () => ok(true, "Browser toolbox started\n")
> );

Now that I re-read this code, I also find this code confusing.
But you suggestion doesn't work as I do have to wait for callbacks to be called before finishing the tests. So I need to yield on some promises. It would certainly look less indented and may be easier to read if I use devtools promises instead of DOM one, but I thought it was better to use DOM one in new code?

Comment 11

[James Long (:jlongster)]

(In reply to Alexandre Poirot [:ochameau] from comment #10)
> 
> Now that I re-read this code, I also find this code confusing.
> But you suggestion doesn't work as I do have to wait for callbacks to be
> called before finishing the tests. So I need to yield on some promises. It
> would certainly look less indented and may be easier to read if I use
> devtools promises instead of DOM one, but I thought it was better to use DOM
> one in new code?

I think you're right; the only reason I can think of to use devtools promises is if we still don't make tests fail for unhandled DOM promise rejections. Here you probably should use native promises.

I'll leave it up to you to decide how you want to clean it up; at least a comment describing the workflow would be nice.

In general I don't have much to say about the test because how this works goes over my head :) But it's great to have a test to make sure it works!

Comment 12

[Alexandre Poirot [:ochameau]]

Attachment: patch v4

With a comment about the DOM promises and nit addressed.

Comment 13

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d13685e393b9

Comment 14

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to James Long (:jlongster) from comment #11)
> (In reply to Alexandre Poirot [:ochameau] from comment #10)
> > 
> > Now that I re-read this code, I also find this code confusing.
> > But you suggestion doesn't work as I do have to wait for callbacks to be
> > called before finishing the tests. So I need to yield on some promises. It
> > would certainly look less indented and may be easier to read if I use
> > devtools promises instead of DOM one, but I thought it was better to use DOM
> > one in new code?
> 
> I think you're right; the only reason I can think of to use devtools
> promises is if we still don't make tests fail for unhandled DOM promise
> rejections. Here you probably should use native promises.

Bug 1242505 tracks the unhandled rejection tracking in the test harness for DOM Promises.  I typically use Promise.jsm still, since this bug is not yet fixed.

Comment 15

[Alexandre Poirot [:ochameau]]

https://hg.mozilla.org/integration/fx-team/rev/ed934827bebaeee8a1aeb1726a3af1021d2a8364
Bug 1264626 - Test the browser toolbox debugger. r=jryans,jlongster

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ed934827beba